* [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
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.