linux-kernel - Re: [PATCH] media: uvcvideo: Silence memcpy() run-time false positive warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiDSCvB8vRp43A1J4BpNZveCvG66XbDmnkKZykbWSFCLX1XUQ@mail.gmail.com>
Date:   Mon, 9 Jan 2023 11:46:00 +0100
From:   Ricardo Ribalda <ribalda@...omium.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Kees Cook <keescook@...omium.org>, ionut_n2001@...oo.com,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] media: uvcvideo: Silence memcpy() run-time false positive warnings

Hi Laurent

I was thinking about something on the line of the attached patch,

uvc_frame_header->data could also be replaced with a union.

Warning, not tested ;)


On Sat, 7 Jan 2023 at 02:42, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hello,
>
> On Fri, Jan 06, 2023 at 12:19:01PM -0800, Kees Cook wrote:
> > On Fri, Jan 06, 2023 at 12:43:44PM +0100, Ricardo Ribalda wrote:
> > > On Fri, 6 Jan 2023 at 07:19, Kees Cook wrote:
> > > >
> > > > The memcpy() in uvc_video_decode_meta() intentionally copies across the
> > > > length and flags members and into the trailing buf flexible array.
> > > > Split the copy so that the compiler can better reason about (the lack
> > > > of) buffer overflows here. Avoid the run-time false positive warning:
> > > >
> > > >   memcpy: detected field-spanning write (size 12) of single field "&meta->length" at drivers/media/usb/uvc/uvc_video.c:1355 (size 1)
> > > >
> > > > Additionally fix a typo in the documentation for struct uvc_meta_buf.
> > > >
> > > > Reported-by: ionut_n2001@...oo.com
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216810
> > > > Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@...nel.org>
> > > > Cc: linux-media@...r.kernel.org
> > > > Signed-off-by: Kees Cook <keescook@...omium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_video.c | 4 +++-
> > > >  include/uapi/linux/uvcvideo.h     | 2 +-
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index d2eb9066e4dc..b67347ab4181 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -1352,7 +1352,9 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > > >         if (has_scr)
> > > >                 memcpy(stream->clock.last_scr, scr, 6);
> > > >
> > > > -       memcpy(&meta->length, mem, length);
> > > > +       meta->length = mem[0];
> > > > +       meta->flags  = mem[1];
> > > > +       memcpy(meta->buf, &mem[2], length - 2);
> > > >         meta_buf->bytesused += length + sizeof(meta->ns) + sizeof(meta->sof);
> > > >
> > > >         uvc_dbg(stream->dev, FRAME,
> > > > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> > > > index 8288137387c0..a9d0a64007ba 100644
> > > > --- a/include/uapi/linux/uvcvideo.h
> > > > +++ b/include/uapi/linux/uvcvideo.h
> > > > @@ -86,7 +86,7 @@ struct uvc_xu_control_query {
> > > >   * struct. The first two fields are added by the driver, they can be used for
> > > >   * clock synchronisation. The rest is an exact copy of a UVC payload header.
> > > >   * Only complete objects with complete buffers are included. Therefore it's
> > > > - * always sizeof(meta->ts) + sizeof(meta->sof) + meta->length bytes large.
> > > > + * always sizeof(meta->ns) + sizeof(meta->sof) + meta->length bytes large.
> > > >   */
> > > >  struct uvc_meta_buf {
> > > >         __u64 ns;
> > > [...]
> > >
> > > Would it make more sense to replace *mem with a structure/union. Something like:
> > > https://patchwork.linuxtv.org/project/linux-media/patch/20221214-uvc-status-alloc-v4-0-f8e3e2994ebd@chromium.org/
> >
> > I wasn't sure -- it seemed like this routine was doing the serializing
> > into a struct already and an additional struct overlay wasn't going to
> > improve readability. But I can certainly do that if it's preferred!
>
> I'm not sure to see how using an additional struct or union would help.
> We can't use struct assignment as the data may be unaligned, so memcpy()
> is required. The issue isn't with the source operand of the memcpy() but
> with the destination operand. Ricardo, if I'm missing something, please
> submit an alternative patch to explain what you meant.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

View attachment "0001-media-uvcvideo-Refactor-uvc_video_decode_meta.patch" of type "text/x-patch" (4504 bytes)