media: uvcvideo: fix division by zero at stream start

PATCH In review Johan Hovold 3 participants 9 messages Oct 26, 2021 – Dec 3, 2021

Subsystems (4)

Stable branch Media (V4L/DVB) USB USB video class

lore

Share & tools

lore

Johan Hovold

author

2021-10-26 09:56 UTC #0 lore raw

PATCH 1/1 media: uvcvideo: fix division by zero at stream start

Add the missing bulk-endpoint max-packet sanity check to probe() to avoid division by zero in uvc_alloc_urb_buffers() in case a malicious device has broken descriptors (or when doing descriptor fuzz testing).

Note that USB core will reject URBs submitted for endpoints with zero wMaxPacketSize but that drivers doing packet-size calculations still need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0")).

Fixes c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")

Cc [email protected] # 2.6.26

Signed-off-by Johan Hovold <[email protected]>

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

drivers/media/usb/uvc/uvc_video.c +4 -0

index e16464606b14..85ac5c1081b6 100644
@@ -1958,6 +1958,10 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
 		if (ep == NULL)
 			return -EIO;
 
+		/* Reject broken descriptors. */
+		if (usb_endpoint_maxp(&ep->desc) == 0)
+			return -EIO;
+
 		ret = uvc_init_video_bulk(stream, ep, gfp_flags);
 	}

Signature · 1 line

2.32.0

Kieran Bingham

Reviewed-by on v1 1/1

2021-10-26 10:55 UTC #1 lore raw

Quoting Johan Hovold (2021-10-26 10:55:11)

Quoted 21 lines

Add the missing bulk-endpoint max-packet sanity check to probe() to avoid division by zero in uvc_alloc_urb_buffers() in case a malicious device has broken descriptors (or when doing descriptor fuzz testing).
Note that USB core will reject URBs submitted for endpoints with zero wMaxPacketSize but that drivers doing packet-size calculations still need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0")).
Fixes c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Cc [email protected] # 2.6.26
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/media/usb/uvc/uvc_video.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/usb/uvc/uvc_video.c +0 -0
index e16464606b14..85ac5c1081b6 100644

drivers/media/usb/uvc/uvc_video.c static int uvc_video_start_transfer(struct uvc_streaming *stream, · line 1963

Quoted 6 lines

drivers/media/usb/uvc/uvc_video.c +3 -0

@@ -1958,6 +1958,10 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
                if (ep == NULL)
                        return -EIO;
 
+               /* Reject broken descriptors. */
+               if (usb_endpoint_maxp(&ep->desc) == 0)
+                       return -EIO;

Is there any value in identifying this with a specific return code like -ENODATA?

But either way, this seems sane.

Reviewed-by Kieran Bingham <[email protected]>

Quoted 7 lines

drivers/media/usb/uvc/uvc_video.c +1 -0

+
                ret = uvc_init_video_bulk(stream, ep, gfp_flags);
        }

Signature · 1 line

2.32.0

Laurent Pinchart

maintainer

2021-10-26 11:15 UTC #2 lore raw

Quoted 25 lines · Kieran Bingham

Quoting Johan Hovold (2021-10-26 10:55:11)
Quoted 24 lines
Add the missing bulk-endpoint max-packet sanity check to probe() to avoid division by zero in uvc_alloc_urb_buffers() in case a malicious device has broken descriptors (or when doing descriptor fuzz testing).
Note that USB core will reject URBs submitted for endpoints with zero wMaxPacketSize but that drivers doing packet-size calculations still need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0")).
Fixes c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Cc [email protected] # 2.6.26
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/media/usb/uvc/uvc_video.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/usb/uvc/uvc_video.c +0 -0
index e16464606b14..85ac5c1081b6 100644
@@ -1958,6 +1958,10 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
                if (ep == NULL)
                        return -EIO;
 

Quoted 6 lines

drivers/media/usb/uvc/uvc_video.c static int uvc_video_start_transfer(struct uvc_streaming *stream, · line 1963
Quoted 3 lines
drivers/media/usb/uvc/uvc_video.c +3 -0
+               /* Reject broken descriptors. */
+               if (usb_endpoint_maxp(&ep->desc) == 0)
+                       return -EIO;
Is there any value in identifying this with a specific return code like -ENODATA?

Going one step further, wouldn't it be better to fail probe() for those devices ?

Quoted 8 lines

But either way, this seems sane.
Reviewed-by Kieran Bingham <[email protected]>
Quoted 4 lines
drivers/media/usb/uvc/uvc_video.c +1 -0
+
                ret = uvc_init_video_bulk(stream, ep, gfp_flags);
        }
 

Signature · 3 lines

Regards,

Laurent Pinchart

Johan Hovold

author

2021-10-26 12:07 UTC #3 lore raw

Quoted 33 lines · Laurent Pinchart

Quoted 25 lines · Kieran Bingham
Quoting Johan Hovold (2021-10-26 10:55:11)
Quoted 24 lines
Add the missing bulk-endpoint max-packet sanity check to probe() to avoid division by zero in uvc_alloc_urb_buffers() in case a malicious device has broken descriptors (or when doing descriptor fuzz testing).
Note that USB core will reject URBs submitted for endpoints with zero wMaxPacketSize but that drivers doing packet-size calculations still need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0")).
Fixes c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Cc [email protected] # 2.6.26
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/media/usb/uvc/uvc_video.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/usb/uvc/uvc_video.c +0 -0
index e16464606b14..85ac5c1081b6 100644
@@ -1958,6 +1958,10 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
                if (ep == NULL)
                        return -EIO;
 
Quoted 6 lines
drivers/media/usb/uvc/uvc_video.c static int uvc_video_start_transfer(struct uvc_streaming *stream, · line 1963
Quoted 3 lines
drivers/media/usb/uvc/uvc_video.c +3 -0
+               /* Reject broken descriptors. */
+               if (usb_endpoint_maxp(&ep->desc) == 0)
+                       return -EIO;
Is there any value in identifying this with a specific return code like -ENODATA?

Quoted 2 lines

Going one step further, wouldn't it be better to fail probe() for those devices ?

This is not how the driver works today. Look at the "return -EIO" just above in case the expected endpoint is missing. And similarly for the isochronous case for which zero wMaxPacket isn't handled until uvc_video_start_transfer() either (a few lines further up still).

Note however the copy-paste error in the commit message mentioning probe(), which is indeed where this would typically be handled.

Do you want me to resend or can you change

	s/probe()/uvc_video_start_transfer()/

in the commit message when applying if you think this is acceptable as is?

Johan

Laurent Pinchart

maintainer Reviewed-by

2021-10-26 21:51 UTC #4 lore raw

Hi Johan,

Quoted 34 lines · Johan Hovold

Quoted 33 lines · Laurent Pinchart
Quoted 31 lines · Kieran Bingham
Quoting Johan Hovold (2021-10-26 10:55:11)
Quoted 21 lines
Add the missing bulk-endpoint max-packet sanity check to probe() to avoid division by zero in uvc_alloc_urb_buffers() in case a malicious device has broken descriptors (or when doing descriptor fuzz testing).
Note that USB core will reject URBs submitted for endpoints with zero wMaxPacketSize but that drivers doing packet-size calculations still need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0")).
Fixes c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Cc [email protected] # 2.6.26
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/media/usb/uvc/uvc_video.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/usb/uvc/uvc_video.c +0 -0
index e16464606b14..85ac5c1081b6 100644
drivers/media/usb/uvc/uvc_video.c static int uvc_video_start_transfer(struct uvc_streaming *stream, · line 1963
Quoted 6 lines
drivers/media/usb/uvc/uvc_video.c +3 -0
@@ -1958,6 +1958,10 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
                if (ep == NULL)
                        return -EIO;
 
+               /* Reject broken descriptors. */
+               if (usb_endpoint_maxp(&ep->desc) == 0)
+                       return -EIO;
Is there any value in identifying this with a specific return code like -ENODATA?

Quoted 7 lines

Quoted 2 lines
Going one step further, wouldn't it be better to fail probe() for those devices ?
This is not how the driver works today. Look at the "return -EIO" just above in case the expected endpoint is missing. And similarly for the isochronous case for which zero wMaxPacket isn't handled until uvc_video_start_transfer() either (a few lines further up still).

That's a good point, it wouldn't be reasonable to ask you to fix all that :-)

Quoted 9 lines

Note however the copy-paste error in the commit message mentioning probe(), which is indeed where this would typically be handled.
Do you want me to resend or can you change
	s/probe()/uvc_video_start_transfer()/
in the commit message when applying if you think this is acceptable as is?

I can fix this when applying.

Reviewed-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Johan Hovold

author

2021-10-27 06:37 UTC #5 lore raw

Quoted 4 lines · Laurent Pinchart

Hi Johan,
On Tue, Oct 26, 2021 at 02:06:55PM +0200, Johan Hovold wrote:

Quoted 8 lines

Quoted 6 lines
Do you want me to resend or can you change
	s/probe()/uvc_video_start_transfer()/
in the commit message when applying if you think this is acceptable as is?
I can fix this when applying.

Perfect, thanks.

Quoted 1 line

Reviewed-by Laurent Pinchart <[email protected]>

Johan

Johan Hovold

author

2021-12-03 10:29 UTC #6 lore raw

In reply to Laurent Pinchart

Hi Laurent,

Quoted 24 lines · Laurent Pinchart

Hi Johan,
Quoted 21 lines · Johan Hovold
Quoted 6 lines · Laurent Pinchart
Quoted 5 lines · Kieran Bingham
Quoting Johan Hovold (2021-10-26 10:55:11)
Quoted 4 lines
Add the missing bulk-endpoint max-packet sanity check to probe() to avoid division by zero in uvc_alloc_urb_buffers() in case a malicious device has broken descriptors (or when doing descriptor fuzz testing).
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Note that USB core will reject URBs submitted for endpoints with zero wMaxPacketSize but that drivers doing packet-size calculations still need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0")).
Fixes c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Cc [email protected] # 2.6.26
Signed-off-by Johan Hovold <[email protected]>
Note however the copy-paste error in the commit message mentioning probe(), which is indeed where this would typically be handled.
Do you want me to resend or can you change

Quoted 8 lines

Quoted 4 lines
	s/probe()/uvc_video_start_transfer()/
in the commit message when applying if you think this is acceptable as is?
I can fix this when applying.
Reviewed-by Laurent Pinchart <[email protected]>

I noticed that this one hasn't showed up in linux-next yet. Do you still have it in your queue or do you want me to resend?

Johan

Laurent Pinchart

maintainer

2021-12-03 10:52 UTC #7 lore raw

Hi Johan,

Quoted 28 lines · Johan Hovold

Quoted 27 lines · Laurent Pinchart
Quoted 25 lines · Johan Hovold
Quoted 6 lines · Laurent Pinchart
Quoted 5 lines · Kieran Bingham
Quoting Johan Hovold (2021-10-26 10:55:11)
Quoted 4 lines
Add the missing bulk-endpoint max-packet sanity check to probe() to avoid division by zero in uvc_alloc_urb_buffers() in case a malicious device has broken descriptors (or when doing descriptor fuzz testing).
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Note that USB core will reject URBs submitted for endpoints with zero wMaxPacketSize but that drivers doing packet-size calculations still need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0")).
Fixes c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Cc [email protected] # 2.6.26
Signed-off-by Johan Hovold <[email protected]>
Note however the copy-paste error in the commit message mentioning probe(), which is indeed where this would typically be handled.
Do you want me to resend or can you change
	s/probe()/uvc_video_start_transfer()/
in the commit message when applying if you think this is acceptable as is?

Quoted 6 lines

Quoted 3 lines
I can fix this when applying.
Reviewed-by Laurent Pinchart <[email protected]>
I noticed that this one hasn't showed up in linux-next yet. Do you still have it in your queue or do you want me to resend?

It should be in Mauro's queue now:

https://lore.kernel.org/all/[email protected]/

Signature · 3 lines

Regards,

Laurent Pinchart

Johan Hovold

author

2021-12-03 10:56 UTC #8 lore raw

Quoted 6 lines · Laurent Pinchart

Quoted 2 lines
I noticed that this one hasn't showed up in linux-next yet. Do you still have it in your queue or do you want me to resend?
It should be in Mauro's queue now:
https://lore.kernel.org/all/[email protected]/

Perfect, thanks!

Johan