* [PATCH] Make sure we parse really received data.
@ 2021-12-22 10:46 Hans Petter Selasky
2021-12-23 19:09 ` Ricardo Ribalda
0 siblings, 1 reply; 5+ messages in thread
From: Hans Petter Selasky @ 2021-12-22 10:46 UTC (permalink / raw)
To: linux-media
Hi,
USB control requests may return less data than we ask for.
Found using valgrind and webcamd on FreeBSD.
==15522== Conditional jump or move depends on uninitialised value(s)
==15522== at 0x662EF4: uvc_fixup_video_ctrl (uvc_video.c:135)
==15522== by 0x662EF4: uvc_get_video_ctrl (uvc_video.c:297)
==15522== by 0x6640B0: uvc_video_init (uvc_video.c:2078)
==15522== by 0x65E79D: uvc_register_video (uvc_driver.c:2258)
==15522== by 0x65E79D: uvc_register_terms (uvc_driver.c:2300)
==15522== by 0x65E79D: uvc_register_chains (uvc_driver.c:2321)
==15522== by 0x65E79D: uvc_probe (uvc_driver.c:2463)
==15522== by 0x3C8F46: usb_linux_probe_p (linux_usb.c:449)
==15522== by 0x75B4B2: main (webcamd.c:1021)
==15522== Uninitialised value was created by a heap allocation
==15522== at 0x4853844: malloc (in
/usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
==15522== by 0x3BC8A4: kmalloc (linux_func.c:1807)
==15522== by 0x662C8C: uvc_get_video_ctrl (uvc_video.c:229)
==15522== by 0x6640B0: uvc_video_init (uvc_video.c:2078)
==15522== by 0x65E79D: uvc_register_video (uvc_driver.c:2258)
==15522== by 0x65E79D: uvc_register_terms (uvc_driver.c:2300)
==15522== by 0x65E79D: uvc_register_chains (uvc_driver.c:2321)
==15522== by 0x65E79D: uvc_probe (uvc_driver.c:2463)
==15522== by 0x3C8F46: usb_linux_probe_p (linux_usb.c:449)
==15522== by 0x75B4B2: main (webcamd.c:1021)
Signed-off-by: Hans Petter Selasky <hps@selasky.org>
---
drivers/media/usb/uvc/uvc_video.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 9f37eaf28ce7..6233703f9a50 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -258,7 +258,11 @@ static int uvc_get_video_ctrl(struct uvc_streaming
*stream,
query == UVC_GET_DEF)
return -EIO;
- data = kmalloc(size, GFP_KERNEL);
+ /*
+ * Make sure we parse really received data
+ * by allocating a zeroed buffer.
+ */
+ data = kzalloc(size, GFP_KERNEL);
if (data == NULL)
return -ENOMEM;
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Make sure we parse really received data.
2021-12-22 10:46 [PATCH] Make sure we parse really received data Hans Petter Selasky
@ 2021-12-23 19:09 ` Ricardo Ribalda
2021-12-31 9:59 ` Hans Petter Selasky
0 siblings, 1 reply; 5+ messages in thread
From: Ricardo Ribalda @ 2021-12-23 19:09 UTC (permalink / raw)
To: Hans Petter Selasky, Laurent Pinchart; +Cc: linux-media
Hi Hans
Thanks for your patch
On Thu, 23 Dec 2021 at 16:42, Hans Petter Selasky <hps@selasky.org> wrote:
>
> Hi,
>
> USB control requests may return less data than we ask for.
> Found using valgrind and webcamd on FreeBSD.
If the usb control request returns less data, then the checks for
ret!=size will trigger.
Am I missing something obvious?
Best regards
>
> ==15522== Conditional jump or move depends on uninitialised value(s)
> ==15522== at 0x662EF4: uvc_fixup_video_ctrl (uvc_video.c:135)
> ==15522== by 0x662EF4: uvc_get_video_ctrl (uvc_video.c:297)
> ==15522== by 0x6640B0: uvc_video_init (uvc_video.c:2078)
> ==15522== by 0x65E79D: uvc_register_video (uvc_driver.c:2258)
> ==15522== by 0x65E79D: uvc_register_terms (uvc_driver.c:2300)
> ==15522== by 0x65E79D: uvc_register_chains (uvc_driver.c:2321)
> ==15522== by 0x65E79D: uvc_probe (uvc_driver.c:2463)
> ==15522== by 0x3C8F46: usb_linux_probe_p (linux_usb.c:449)
> ==15522== by 0x75B4B2: main (webcamd.c:1021)
> ==15522== Uninitialised value was created by a heap allocation
> ==15522== at 0x4853844: malloc (in
> /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
> ==15522== by 0x3BC8A4: kmalloc (linux_func.c:1807)
> ==15522== by 0x662C8C: uvc_get_video_ctrl (uvc_video.c:229)
> ==15522== by 0x6640B0: uvc_video_init (uvc_video.c:2078)
> ==15522== by 0x65E79D: uvc_register_video (uvc_driver.c:2258)
> ==15522== by 0x65E79D: uvc_register_terms (uvc_driver.c:2300)
> ==15522== by 0x65E79D: uvc_register_chains (uvc_driver.c:2321)
> ==15522== by 0x65E79D: uvc_probe (uvc_driver.c:2463)
> ==15522== by 0x3C8F46: usb_linux_probe_p (linux_usb.c:449)
> ==15522== by 0x75B4B2: main (webcamd.c:1021)
>
> Signed-off-by: Hans Petter Selasky <hps@selasky.org>
> ---
> drivers/media/usb/uvc/uvc_video.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index 9f37eaf28ce7..6233703f9a50 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -258,7 +258,11 @@ static int uvc_get_video_ctrl(struct uvc_streaming
> *stream,
> query == UVC_GET_DEF)
> return -EIO;
>
> - data = kmalloc(size, GFP_KERNEL);
> + /*
> + * Make sure we parse really received data
> + * by allocating a zeroed buffer.
> + */
> + data = kzalloc(size, GFP_KERNEL);
> if (data == NULL)
> return -ENOMEM;
>
> --
> 2.34.1
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Make sure we parse really received data.
2021-12-23 19:09 ` Ricardo Ribalda
@ 2021-12-31 9:59 ` Hans Petter Selasky
2022-01-03 15:18 ` Ricardo Ribalda
0 siblings, 1 reply; 5+ messages in thread
From: Hans Petter Selasky @ 2021-12-31 9:59 UTC (permalink / raw)
To: Ricardo Ribalda, Laurent Pinchart; +Cc: linux-media
On 12/23/21 20:09, Ricardo Ribalda wrote:
> Hi Hans
>
> Thanks for your patch
>
> On Thu, 23 Dec 2021 at 16:42, Hans Petter Selasky <hps@selasky.org> wrote:
>>
>> Hi,
>>
>> USB control requests may return less data than we ask for.
>> Found using valgrind and webcamd on FreeBSD.
>
> If the usb control request returns less data, then the checks for
> ret!=size will trigger.
>
> Am I missing something obvious?
>
Hi,
USB control transfers are allowed to be short terminated! But there is
no flag to error out on short terminated control transfers, from what I
can see. This is sometimes used for reading strings. You setup a fixed
255 byte buffer, and then simply issue the control read string request.
The length you get back is the actual string length.
> If the usb control request returns less data, then the checks for
> ret!=size will trigger.
Can you point in the code where this check is?
--HPS
> Best regards
>
>
>>
>> ==15522== Conditional jump or move depends on uninitialised value(s)
>> ==15522== at 0x662EF4: uvc_fixup_video_ctrl (uvc_video.c:135)
>> ==15522== by 0x662EF4: uvc_get_video_ctrl (uvc_video.c:297)
>> ==15522== by 0x6640B0: uvc_video_init (uvc_video.c:2078)
>> ==15522== by 0x65E79D: uvc_register_video (uvc_driver.c:2258)
>> ==15522== by 0x65E79D: uvc_register_terms (uvc_driver.c:2300)
>> ==15522== by 0x65E79D: uvc_register_chains (uvc_driver.c:2321)
>> ==15522== by 0x65E79D: uvc_probe (uvc_driver.c:2463)
>> ==15522== by 0x3C8F46: usb_linux_probe_p (linux_usb.c:449)
>> ==15522== by 0x75B4B2: main (webcamd.c:1021)
>> ==15522== Uninitialised value was created by a heap allocation
>> ==15522== at 0x4853844: malloc (in
>> /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
>> ==15522== by 0x3BC8A4: kmalloc (linux_func.c:1807)
>> ==15522== by 0x662C8C: uvc_get_video_ctrl (uvc_video.c:229)
>> ==15522== by 0x6640B0: uvc_video_init (uvc_video.c:2078)
>> ==15522== by 0x65E79D: uvc_register_video (uvc_driver.c:2258)
>> ==15522== by 0x65E79D: uvc_register_terms (uvc_driver.c:2300)
>> ==15522== by 0x65E79D: uvc_register_chains (uvc_driver.c:2321)
>> ==15522== by 0x65E79D: uvc_probe (uvc_driver.c:2463)
>> ==15522== by 0x3C8F46: usb_linux_probe_p (linux_usb.c:449)
>> ==15522== by 0x75B4B2: main (webcamd.c:1021)
>>
>> Signed-off-by: Hans Petter Selasky <hps@selasky.org>
>> ---
>> drivers/media/usb/uvc/uvc_video.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c
>> index 9f37eaf28ce7..6233703f9a50 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -258,7 +258,11 @@ static int uvc_get_video_ctrl(struct uvc_streaming
>> *stream,
>> query == UVC_GET_DEF)
>> return -EIO;
>>
>> - data = kmalloc(size, GFP_KERNEL);
>> + /*
>> + * Make sure we parse really received data
>> + * by allocating a zeroed buffer.
>> + */
>> + data = kzalloc(size, GFP_KERNEL);
>> if (data == NULL)
>> return -ENOMEM;
>>
>> --
>> 2.34.1
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Make sure we parse really received data.
2021-12-31 9:59 ` Hans Petter Selasky
@ 2022-01-03 15:18 ` Ricardo Ribalda
2022-01-03 16:03 ` Hans Petter Selasky
0 siblings, 1 reply; 5+ messages in thread
From: Ricardo Ribalda @ 2022-01-03 15:18 UTC (permalink / raw)
To: Hans Petter Selasky; +Cc: Laurent Pinchart, linux-media
Hi Hans
On Fri, 31 Dec 2021 at 10:59, Hans Petter Selasky <hps@selasky.org> wrote:
>
> On 12/23/21 20:09, Ricardo Ribalda wrote:
> > Hi Hans
> >
> > Thanks for your patch
> >
> > On Thu, 23 Dec 2021 at 16:42, Hans Petter Selasky <hps@selasky.org> wrote:
> >>
> >> Hi,
> >>
> >> USB control requests may return less data than we ask for.
> >> Found using valgrind and webcamd on FreeBSD.
> >
> > If the usb control request returns less data, then the checks for
> > ret!=size will trigger.
> >
> > Am I missing something obvious?
> >
>
> Hi,
>
> USB control transfers are allowed to be short terminated! But there is
> no flag to error out on short terminated control transfers, from what I
> can see. This is sometimes used for reading strings. You setup a fixed
> 255 byte buffer, and then simply issue the control read string request.
> The length you get back is the actual string length.
>
> > If the usb control request returns less data, then the checks for
> > ret!=size will trigger.
>
> Can you point in the code where this check is?
https://elixir.bootlin.com/linux/latest/source/drivers/media/usb/uvc/uvc_video.c#L281
and
https://elixir.bootlin.com/linux/latest/source/drivers/media/usb/uvc/uvc_video.c#L291
>
> --HPS
>
> > Best regards
> >
> >
> >>
> >> ==15522== Conditional jump or move depends on uninitialised value(s)
> >> ==15522== at 0x662EF4: uvc_fixup_video_ctrl (uvc_video.c:135)
> >> ==15522== by 0x662EF4: uvc_get_video_ctrl (uvc_video.c:297)
> >> ==15522== by 0x6640B0: uvc_video_init (uvc_video.c:2078)
> >> ==15522== by 0x65E79D: uvc_register_video (uvc_driver.c:2258)
> >> ==15522== by 0x65E79D: uvc_register_terms (uvc_driver.c:2300)
> >> ==15522== by 0x65E79D: uvc_register_chains (uvc_driver.c:2321)
> >> ==15522== by 0x65E79D: uvc_probe (uvc_driver.c:2463)
> >> ==15522== by 0x3C8F46: usb_linux_probe_p (linux_usb.c:449)
> >> ==15522== by 0x75B4B2: main (webcamd.c:1021)
> >> ==15522== Uninitialised value was created by a heap allocation
> >> ==15522== at 0x4853844: malloc (in
> >> /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
> >> ==15522== by 0x3BC8A4: kmalloc (linux_func.c:1807)
> >> ==15522== by 0x662C8C: uvc_get_video_ctrl (uvc_video.c:229)
> >> ==15522== by 0x6640B0: uvc_video_init (uvc_video.c:2078)
> >> ==15522== by 0x65E79D: uvc_register_video (uvc_driver.c:2258)
> >> ==15522== by 0x65E79D: uvc_register_terms (uvc_driver.c:2300)
> >> ==15522== by 0x65E79D: uvc_register_chains (uvc_driver.c:2321)
> >> ==15522== by 0x65E79D: uvc_probe (uvc_driver.c:2463)
> >> ==15522== by 0x3C8F46: usb_linux_probe_p (linux_usb.c:449)
> >> ==15522== by 0x75B4B2: main (webcamd.c:1021)
> >>
> >> Signed-off-by: Hans Petter Selasky <hps@selasky.org>
> >> ---
> >> drivers/media/usb/uvc/uvc_video.c | 6 +++++-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_video.c
> >> b/drivers/media/usb/uvc/uvc_video.c
> >> index 9f37eaf28ce7..6233703f9a50 100644
> >> --- a/drivers/media/usb/uvc/uvc_video.c
> >> +++ b/drivers/media/usb/uvc/uvc_video.c
> >> @@ -258,7 +258,11 @@ static int uvc_get_video_ctrl(struct uvc_streaming
> >> *stream,
> >> query == UVC_GET_DEF)
> >> return -EIO;
> >>
> >> - data = kmalloc(size, GFP_KERNEL);
> >> + /*
> >> + * Make sure we parse really received data
> >> + * by allocating a zeroed buffer.
> >> + */
> >> + data = kzalloc(size, GFP_KERNEL);
> >> if (data == NULL)
> >> return -ENOMEM;
> >>
> >> --
> >> 2.34.1
> >
> >
> >
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Make sure we parse really received data.
2022-01-03 15:18 ` Ricardo Ribalda
@ 2022-01-03 16:03 ` Hans Petter Selasky
0 siblings, 0 replies; 5+ messages in thread
From: Hans Petter Selasky @ 2022-01-03 16:03 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Laurent Pinchart, linux-media
On 1/3/22 16:18, Ricardo Ribalda wrote:
>> Can you point in the code where this check is?
>
> https://elixir.bootlin.com/linux/latest/source/drivers/media/usb/uvc/uvc_video.c#L281
> and
> https://elixir.bootlin.com/linux/latest/source/drivers/media/usb/uvc/uvc_video.c#L291
Hi Ricardo,
After looking more closely at the code, I believe the issue I found is a
false positive of valgrind, that doesn't see the kernel move data into
the given location after the USB control request.
This patch can be dropped, unless you think zeroing this buffer is good
practice, in case of future UVC descriptor parsing updates.
RET=26, SIZE=26
Thanks for looking into it.
--HPS
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-03 16:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-22 10:46 [PATCH] Make sure we parse really received data Hans Petter Selasky
2021-12-23 19:09 ` Ricardo Ribalda
2021-12-31 9:59 ` Hans Petter Selasky
2022-01-03 15:18 ` Ricardo Ribalda
2022-01-03 16:03 ` Hans Petter Selasky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.