Skip to contentSkip to conversation

uvc: Avoid NULL pointer dereference at the end of streaming

PATCH In review Sakari Ailus 3 participants 4 messages Jan 30, 2019 – Feb 1, 2019

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

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

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.

Fixes 9c0863b1cc48 ("[media] vb2: call buf_finish from __queue_cancel")

---
since v1:

- Improve the comment in uvc_video_clock_update().

- Add appropriate Cc: stable, Reviewed-by: and Fixes: tags.

 drivers/media/usb/uvc/uvc_video.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/media/usb/uvc/uvc_video.c +8 -0
index 84525ff047450..e314657a1843a 100644
@@ -676,6 +676,14 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
 	if (!uvc_hw_timestamps_param)
 		return;
 
+	/*
+	 * We will get called from __vb2_queue_cancel() if there are buffers
+	 * done but not dequeued by the user, but the sample array has already
+	 * been released at that time. Just bail out in that case.
+	 */
+	if (!clock->samples)
+		return;
+
 	spin_lock_irqsave(&clock->lock, flags);
 
 	if (clock->count < clock->size)
Signature · 1 line
2.11.0
maintainer
#1 lore raw

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag, fixing commit: 9c0863b1cc48 [media] vb2: call buf_finish from __queue_cancel.

The bot has tested the following trees: v4.20.5, v4.19.18, v4.14.96, v4.9.153, v4.4.172, v3.18.133.

v4.20.5: Build OK!
v4.19.18: Build OK!
v4.14.96: Build OK!
v4.9.153: Build OK!
v4.4.172: Build OK!
v3.18.133: Failed to apply! Possible dependencies:
5d0fd3c806b9 ("[media] uvcvideo: Disable hardware timestamps by default")

How should we proceed with this patch?

--
Thanks,
Sasha

#2 lore raw

Hi Sasha,

Quoted 18 lines · Sasha Levin

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag, fixing commit: 9c0863b1cc48 [media] vb2: call buf_finish from __queue_cancel.

The bot has tested the following trees: v4.20.5, v4.19.18, v4.14.96, v4.9.153, v4.4.172, v3.18.133.

v4.20.5: Build OK!
v4.19.18: Build OK!
v4.14.96: Build OK!
v4.9.153: Build OK!
v4.4.172: Build OK!
v3.18.133: Failed to apply! Possible dependencies:
5d0fd3c806b9 ("[media] uvcvideo: Disable hardware timestamps by default")

Quoted 1 line

How should we proceed with this patch?

IMO 5d0fd3c806b9 should be applied as well. It's effectively a bugfix as well (but which also, for most users, covered the problem fixed by 9c0863b1cc48).

Laurent, could you confirm?

Signature · 4 lines
Regards,

Sakari Ailus
[email protected]
maintainer
#3 lore raw

Hello,

Quoted 19 lines · Sakari Ailus
Quoted 18 lines · Sasha Levin

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag, fixing commit: 9c0863b1cc48 [media] vb2: call buf_finish from __queue_cancel.

The bot has tested the following trees: v4.20.5, v4.19.18, v4.14.96, v4.9.153, v4.4.172, v3.18.133.

v4.20.5: Build OK!
v4.19.18: Build OK!
v4.14.96: Build OK!
v4.9.153: Build OK!
v4.4.172: Build OK!
v3.18.133: Failed to apply! Possible dependencies:
5d0fd3c806b9 ("[media] uvcvideo: Disable hardware timestamps by default")

Quoted 7 lines
Quoted 1 line

How should we proceed with this patch?

IMO 5d0fd3c806b9 should be applied as well. It's effectively a bugfix as well (but which also, for most users, covered the problem fixed by 9c0863b1cc48).

Laurent, could you confirm?

That seems good to me.

Signature · 3 lines
Regards,

Laurent Pinchart