[PATCH 1/1] uvc: Avoid NULL pointer dereference at the end of streaming

[PATCH 1/1] uvc: Avoid NULL pointer dereference at the end of streaming

[Sakari Ailus]

The UVC video driver converts the timestamp from hardware specific unit to
one known by the kernel at the time when the buffer is dequeued. This is
fine in general, but the streamoff operation consists of the following
steps (among other things):

1. uvc_video_clock_cleanup --- the hardware clock sample array is
   released and the pointer to the array is set to NULL,

2. buffers in active state are returned to the user and

3. buf_finish callback is called on buffers that are prepared. buf_finish
   includes calling uvc_video_clock_update that accesses the hardware
   clock sample array.

The above is serialised by a queue specific mutex. Address the problem by
skipping the clock conversion if the hardware clock sample array is
already released.

Reported-by: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>
Tested-by: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Laurent,

This seems like something that's been out there for a while... I'll figure
out soon which stable kernels should receive it, if any.

 drivers/media/usb/uvc/uvc_video.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 84525ff047450..a30c5e1893e72 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -676,6 +676,13 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
 	if (!uvc_hw_timestamps_param)
 		return;
 
+	/*
+	 * We may get called if there are buffers done but not dequeued by the
+	 * user. Just bail out in that case.
+	 */
+	if (!clock->samples)
+		return;
+
 	spin_lock_irqsave(&clock->lock, flags);
 
 	if (clock->count < clock->size)
-- 
2.11.0

Re: [PATCH 1/1] uvc: Avoid NULL pointer dereference at the end of streaming

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tue, Jan 29, 2019 at 11:49:44PM +0200, Sakari Ailus wrote:
> The UVC video driver converts the timestamp from hardware specific unit to
> one known by the kernel at the time when the buffer is dequeued. This is
> fine in general, but the streamoff operation consists of the following
> steps (among other things):
> 
> 1. uvc_video_clock_cleanup --- the hardware clock sample array is
>    released and the pointer to the array is set to NULL,
> 
> 2. buffers in active state are returned to the user and
> 
> 3. buf_finish callback is called on buffers that are prepared. buf_finish
>    includes calling uvc_video_clock_update that accesses the hardware
>    clock sample array.
> 
> The above is serialised by a queue specific mutex. Address the problem by
> skipping the clock conversion if the hardware clock sample array is
> already released.
> 
> Reported-by: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>
> Tested-by: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

The analysis looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Hi Laurent,
> 
> This seems like something that's been out there for a while... I'll figure
> out soon which stable kernels should receive it, if any.

Should I wait for the proper Fixes: and Cc:stable tags before queuing
this patch then ?

>  drivers/media/usb/uvc/uvc_video.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 84525ff047450..a30c5e1893e72 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -676,6 +676,13 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
>  	if (!uvc_hw_timestamps_param)
>  		return;
>  
> +	/*
> +	 * We may get called if there are buffers done but not dequeued by the
> +	 * user. Just bail out in that case.
> +	 */
> +	if (!clock->samples)
> +		return;
> +
>  	spin_lock_irqsave(&clock->lock, flags);
>  
>  	if (clock->count < clock->size)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] uvc: Avoid NULL pointer dereference at the end of streaming

[Sakari Ailus]

Hi Laurent,

On Wed, Jan 30, 2019 at 11:17:37AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Tue, Jan 29, 2019 at 11:49:44PM +0200, Sakari Ailus wrote:
> > The UVC video driver converts the timestamp from hardware specific unit to
> > one known by the kernel at the time when the buffer is dequeued. This is
> > fine in general, but the streamoff operation consists of the following
> > steps (among other things):
> > 
> > 1. uvc_video_clock_cleanup --- the hardware clock sample array is
> >    released and the pointer to the array is set to NULL,
> > 
> > 2. buffers in active state are returned to the user and
> > 
> > 3. buf_finish callback is called on buffers that are prepared. buf_finish
> >    includes calling uvc_video_clock_update that accesses the hardware
> >    clock sample array.
> > 
> > The above is serialised by a queue specific mutex. Address the problem by
> > skipping the clock conversion if the hardware clock sample array is
> > already released.
> > 
> > Reported-by: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>
> > Tested-by: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> The analysis looks good to me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> > Hi Laurent,
> > 
> > This seems like something that's been out there for a while... I'll figure
> > out soon which stable kernels should receive it, if any.
> 
> Should I wait for the proper Fixes: and Cc:stable tags before queuing
> this patch then ?

Please do. I'll figure these out in a moment...

> 
> >  drivers/media/usb/uvc/uvc_video.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 84525ff047450..a30c5e1893e72 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -676,6 +676,13 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
> >  	if (!uvc_hw_timestamps_param)
> >  		return;
> >  
> > +	/*
> > +	 * We may get called if there are buffers done but not dequeued by the
> > +	 * user. Just bail out in that case.
> > +	 */
> > +	if (!clock->samples)
> > +		return;
> > +
> >  	spin_lock_irqsave(&clock->lock, flags);
> >  
> >  	if (clock->count < clock->size)

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com