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

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

[Kees Cook]

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;
-- 
2.34.1

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

[Ricardo Ribalda]

Hi Kees

Thanks for the patch

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/

Regards!

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;
> --
> 2.34.1
>


-- 
Ricardo Ribalda

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

[Kees Cook]

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

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

[Laurent Pinchart]

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@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!

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@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

[-- Attachment #1: Type: text/plain, Size: 3981 bytes --]

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@ideasonboard.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@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!
>
> 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@ideasonboard.com>
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

[-- Attachment #2: 0001-media-uvcvideo-Refactor-uvc_video_decode_meta.patch --]
[-- Type: text/x-patch, Size: 4504 bytes --]

From dce72fe7f003ba40ba2d534e5f84bafc73b37314 Mon Sep 17 00:00:00 2001
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Mon, 9 Jan 2023 11:42:21 +0100
Subject: [PATCH] media: uvcvideo: Refactor uvc_video_decode_meta

NOT TESTED!

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 58 ++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 04f452d95fd6..f6c091626f3c 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1330,6 +1330,17 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
  * Metadata
  */
 
+struct uvc_frame_header {
+	u8 length;
+	u8 flags;
+	u8 data[];
+} __packed;
+
+struct uvc_scr {
+	u32 scr;
+	u16 frame_sof;
+} __packed;
+
 /*
  * Additionally to the payload headers we also want to provide the user with USB
  * Frame Numbers and system time values. The resulting buffer is thus composed
@@ -1343,7 +1354,8 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
  */
 static void uvc_video_decode_meta(struct uvc_streaming *stream,
 				  struct uvc_buffer *meta_buf,
-				  const u8 *mem, unsigned int length)
+				  const struct uvc_frame_header *header,
+				  unsigned int length)
 {
 	struct uvc_meta_buf *meta;
 	size_t len_std = 2;
@@ -1351,7 +1363,8 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
 	unsigned long flags;
 	unsigned int sof;
 	ktime_t time;
-	const u8 *scr;
+	const struct uvc_scr *scr;
+	const u32 *pts;
 
 	if (!meta_buf || length == 2)
 		return;
@@ -1362,28 +1375,31 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
 		return;
 	}
 
-	has_pts = mem[1] & UVC_STREAM_PTS;
-	has_scr = mem[1] & UVC_STREAM_SCR;
+	has_pts = header->flags & UVC_STREAM_PTS;
+	has_scr = header->flags & UVC_STREAM_SCR;
 
 	if (has_pts) {
-		len_std += 4;
-		scr = mem + 6;
-	} else {
-		scr = mem + 2;
+		pts = (u32 *) header->data;
+		len_std += sizeof(*pts);
 	}
 
-	if (has_scr)
-		len_std += 6;
+	if (has_scr) {
+		u8 offset;
+
+		offset = has_pts ? sizeof(*pts) : 0;
+		scr = (struct uvc_scr *) header->data + offset;
+		len_std += sizeof(struct uvc_scr);
+	}
 
 	if (stream->meta.format == V4L2_META_FMT_UVC)
 		length = len_std;
 
 	if (length == len_std && (!has_scr ||
-				  !memcmp(scr, stream->clock.last_scr, 6)))
+				  !memcmp(scr, stream->clock.last_scr, sizeof(struct uvc_scr))))
 		return;
 
 	meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + meta_buf->bytesused);
-	local_irq_save(flags);
+	local_irq_save(flags); //do we need this?
 	time = uvc_video_get_time();
 	sof = usb_get_current_frame_number(stream->dev->udev);
 	local_irq_restore(flags);
@@ -1391,20 +1407,20 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
 	put_unaligned(sof, &meta->sof);
 
 	if (has_scr)
-		memcpy(stream->clock.last_scr, scr, 6);
+		memcpy(stream->clock.last_scr, scr, sizeof(struct uvc_scr));
 
-	meta->length = mem[0];
-	meta->flags  = mem[1];
-	memcpy(meta->buf, &mem[2], length - 2);
+	meta->length = header->length;
+	meta->flags  = header->flags;
+	memcpy(meta->buf, header->data, length - offsetof(struct uvc_frame_header, data));
 	meta_buf->bytesused += length + sizeof(meta->ns) + sizeof(meta->sof);
 
 	uvc_dbg(stream->dev, FRAME,
 		"%s(): t-sys %lluns, SOF %u, len %u, flags 0x%x, PTS %u, STC %u frame SOF %u\n",
 		__func__, ktime_to_ns(time), meta->sof, meta->length,
 		meta->flags,
-		has_pts ? *(u32 *)meta->buf : 0,
-		has_scr ? *(u32 *)scr : 0,
-		has_scr ? *(u32 *)(scr + 4) & 0x7ff : 0);
+		has_pts ? *pts : 0,
+		has_scr ? scr->scr : 0,
+		has_scr ? scr->frame_sof & 0x7ff : 0);
 }
 
 /* ------------------------------------------------------------------------
@@ -1479,7 +1495,7 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
 		if (ret < 0)
 			continue;
 
-		uvc_video_decode_meta(stream, meta_buf, mem, ret);
+		uvc_video_decode_meta(stream, meta_buf, (struct uvc_frame_header *) mem, ret);
 
 		/* Decode the payload data. */
 		uvc_video_decode_data(uvc_urb, buf, mem + ret,
@@ -1531,7 +1547,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
 			memcpy(stream->bulk.header, mem, ret);
 			stream->bulk.header_size = ret;
 
-			uvc_video_decode_meta(stream, meta_buf, mem, ret);
+			uvc_video_decode_meta(stream, meta_buf, (struct uvc_frame_header *)mem, ret);
 
 			mem += ret;
 			len -= ret;
-- 
2.39.0.314.g84b9a713c41-goog

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

[Andy Shevchenko]

On Mon, Jan 09, 2023 at 11:46:00AM +0100, Ricardo Ribalda wrote:
> 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 ;)

...

> +struct uvc_frame_header {
> +	u8 length;
> +	u8 flags;
> +	u8 data[];
> +} __packed;

__packed! (See below)

...

> +		pts = (u32 *) header->data;

Ai-ai-ai.
Here is just a yellow flag...

...

>  	uvc_dbg(stream->dev, FRAME,
>  		"%s(): t-sys %lluns, SOF %u, len %u, flags 0x%x, PTS %u, STC %u frame SOF %u\n",
>  		__func__, ktime_to_ns(time), meta->sof, meta->length,
>  		meta->flags,
> +		has_pts ? *pts : 0,

...and here is a red flag. What you need to have is

	void *pts;
	u32 pts_val;

	pts_val = get_unaligned_be32(); // or le32

	...use pts_val...

> +		has_scr ? scr->scr : 0,
> +		has_scr ? scr->frame_sof & 0x7ff : 0);


-- 
With Best Regards,
Andy Shevchenko

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

[Ricardo Ribalda]

Hi Andy

On Tue, 7 Feb 2023 at 23:07, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jan 09, 2023 at 11:46:00AM +0100, Ricardo Ribalda wrote:
> > 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 ;)
>
> ...
>
> > +struct uvc_frame_header {
> > +     u8 length;
> > +     u8 flags;
> > +     u8 data[];
> > +} __packed;
>
> __packed! (See below)
>
> ...
>
> > +             pts = (u32 *) header->data;
>
> Ai-ai-ai.
> Here is just a yellow flag...
>
> ...
>
> >       uvc_dbg(stream->dev, FRAME,
> >               "%s(): t-sys %lluns, SOF %u, len %u, flags 0x%x, PTS %u, STC %u frame SOF %u\n",
> >               __func__, ktime_to_ns(time), meta->sof, meta->length,
> >               meta->flags,
> > +             has_pts ? *pts : 0,
>
> ...and here is a red flag. What you need to have is

Thanks! you are absolutely right :)

Luckily  Laurent merged the original patch not mine.

Regards!

>
>         void *pts;
>         u32 pts_val;
>
>         pts_val = get_unaligned_be32(); // or le32
>
>         ...use pts_val...
>
> > +             has_scr ? scr->scr : 0,
> > +             has_scr ? scr->frame_sof & 0x7ff : 0);
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Ricardo Ribalda