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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216810
> > > > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > > > Cc: linux-media@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxx/
> >
> > 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@xxxxxxxxxxxxxxxx>
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
From dce72fe7f003ba40ba2d534e5f84bafc73b37314 Mon Sep 17 00:00:00 2001
From: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
---
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
