Re: [PATCH] Make sure we parse really received data.

[Ricardo Ribalda <ribalda@chromium.org>]

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