Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 6 Jan 2023 12:19:01 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Ricardo Ribalda <ribalda@chromium.org>
Cc:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        ionut_n2001@yahoo.com, Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH] media: uvcvideo: Silence memcpy() run-time false
 positive warnings
Message-ID: <202301061217.816FC0313D@keescook>
References: <20230106061659.never.817-kees@kernel.org>
 <CANiDSCtTz4mpTz4RHBzNXL=yBvXNXHBZQ-HYMFegLytoScW4eA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANiDSCtTz4mpTz4RHBzNXL=yBvXNXHBZQ-HYMFegLytoScW4eA@mail.gmail.com>
Precedence: bulk

On Fri, Jan 06, 2023 at 12:43:44PM +0100, Ricardo Ribalda wrote:
> On Fri, 6 Jan 2023 at 07:19, Kees Cook <keescook@chromium.org> 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@yahoo.com
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216810
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: linux-media@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.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!

-Kees

-- 
Kees Cook