Message ID | 20230106061659.never.817-kees@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp669779wrt; Thu, 5 Jan 2023 22:19:49 -0800 (PST) X-Google-Smtp-Source: AMrXdXvE0No6IY0sbXySJZJSSAmop8zlyWJllURCdNKg4BhS7pqeO+KLM6nghscJpil1cbz17McX X-Received: by 2002:a17:906:3943:b0:7c1:52a:e903 with SMTP id g3-20020a170906394300b007c1052ae903mr63819949eje.55.1672985988885; Thu, 05 Jan 2023 22:19:48 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1672985988; cv=none; d=google.com; s=arc-20160816; b=j+VMSa28dK84CCm4n8gDmUs0W5ocAwS1Yn6u+EWUboghkpjN2u3GRc/Q8MzOO0UcmZ gUB7x/tPrD0k6+YrZfi+zxQyvR9tV6cceVUP0EUdGbRjt6LdJwI9Hg3fjZykbrNI4z9o tRYycVymASu5xzFLXI7Ere6o6t7/bz5UvslLXZrofmmn/E9SBiOxwgsWfPKylRS2L5UC y7jI9jv3cciyFOJriirl5+GrLiSexoMfO9/Fs2WWQv5JPUyRVgLR3E5BgjgIE6Xu9FxG 1hv77bgeIMtj8XyN+FHHRoUEVJ7zQw3R113F3XoGHxq25Xi/v91B+/Xb83vyATsIJdsv BtHw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=IVoLYcfDVSSj5Hj2x3GSCmfRlg0nzKREwK4vCCiBijM=; b=EzBSg7H/tpjuGcD1i+2t7SSFUhDFNe87E8zSmCEfDLEisrBXLXqgY/yNkm+0l98TK/ euS1pPHePnL8G8PGxV9ik0bgHEkLzrO8tZoKjE/n7CBlRP5O+/DdKaJs8fd6n2IY3Wz0 5/T1wMwJzd5Na7uikV0aejebIcj8lYEr/M4y+7yGsuV5+N04YNmUxLqKZOmXZuMEpD03 PVm4rkVNUniRvJ02YXHjsDgReiZFCONR8MMbo0BMAilNF35RDX2tbFTGe5T/hc0nRXw2 lMCyb1VPZKWARdqieiNIKWNj79GrIgQn377lAR1UuANQTRWkdT80G7nPe7IDJfyig7ta g/Mg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=AyQ3QZW4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id js19-20020a17090797d300b007ebc267e9fasi428333ejc.83.2023.01.05.22.19.25; Thu, 05 Jan 2023 22:19:48 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=AyQ3QZW4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231938AbjAFGSu (ORCPT <rfc822;tmhikaru@gmail.com> + 99 others); Fri, 6 Jan 2023 01:18:50 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53734 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231848AbjAFGSW (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 6 Jan 2023 01:18:22 -0500 Received: from mail-pj1-x1029.google.com (mail-pj1-x1029.google.com [IPv6:2607:f8b0:4864:20::1029]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 19DAC6ECAF for <linux-kernel@vger.kernel.org>; Thu, 5 Jan 2023 22:17:33 -0800 (PST) Received: by mail-pj1-x1029.google.com with SMTP id w4-20020a17090ac98400b002186f5d7a4cso4349509pjt.0 for <linux-kernel@vger.kernel.org>; Thu, 05 Jan 2023 22:17:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=IVoLYcfDVSSj5Hj2x3GSCmfRlg0nzKREwK4vCCiBijM=; b=AyQ3QZW4y+sUR958e4HRcaZdsf/qgBkw1mlhowceF0XUEUWrY2e1ma6nmNllYeoV/Q Yy5UQs03pzNlRQAeKfjmpys5zi0J3ZFYI3CFleutVn2UmhUlRUTnNoHoZg9gZQxdi1sK ITivwS+vfEb+n4eN81dcLcd4zY2NXxnBYIXWE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=IVoLYcfDVSSj5Hj2x3GSCmfRlg0nzKREwK4vCCiBijM=; b=dGku1j1AXvgfX3rrJt7921D9hOzxfvPPbhllSZ51DTVVpWZCbTHDjB13P7DMPfgceb 6RfERjZKAsRM5FF7E2dRbVPRc5COsdhVys2VIHZ56G7oz9N4S3LWEnZXsK60x0EeQrxN sEyuKNrGvznboo9MgtAFHg7m6ZBeUDqlqyit4TwLrxrdaSA6qGEKXBg4Hzdc/utaKrC5 GAwVokyWIPG8H9hmN2x2E5XS+ZPo+M73VglWAp78wU7nzpU+y7Qi6dcpgbW/PZu3zQVr ijGHBt41oauk95nobxZfIVrX4iHlV5shVZm6B7pj/6b6/B7hWoHCeb1WJjBfObHcIKG2 4w2g== X-Gm-Message-State: AFqh2koFLQ0x62D1nB/JykUPcybK7za55dZTfLU7HZ//QUSi/cN+29uZ PCMSdLoSCJM0dz7BD3blo4UZUw== X-Received: by 2002:a17:90a:b011:b0:226:8fc5:b55 with SMTP id x17-20020a17090ab01100b002268fc50b55mr13282906pjq.33.1672985826224; Thu, 05 Jan 2023 22:17:06 -0800 (PST) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id t23-20020a17090ae51700b00226d4009505sm278581pjy.3.2023.01.05.22.17.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Jan 2023 22:17:05 -0800 (PST) From: Kees Cook <keescook@chromium.org> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Kees Cook <keescook@chromium.org>, 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: [PATCH] media: uvcvideo: Silence memcpy() run-time false positive warnings Date: Thu, 5 Jan 2023 22:17:04 -0800 Message-Id: <20230106061659.never.817-kees@kernel.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=2208; h=from:subject:message-id; bh=HRw9p7KL+4QYDBK7zvJtazO5keiEwenv1mXQer7Jbt4=; b=owEBbQKS/ZANAwAKAYly9N/cbcAmAcsmYgBjt7zf29HyDZw/F4MlHzU+Bl4G8jxpLHfoOnTvU5u5 FlEqB3GJAjMEAAEKAB0WIQSlw/aPIp3WD3I+bhOJcvTf3G3AJgUCY7e83wAKCRCJcvTf3G3AJm9kD/ 47yg+LzAvpwtJYOb3OyV+MyCV2HpjQshWvE4+pgbpPOInmvvsqXeKW0Wm8McFGBzx2yl6blx5Pwpca /47EBMRpOiaT3GJNNxENwSAQHww/dF0ks2rY0PhvtPJbR+ZGd0MsSL03PBC85Kic9R1sf0fibXcTse 1/1izkyXUaxNymvLqFlfEIzecRP8k/7OXqvn9QcyXT/SOtVQb40dPRMPuxukLzjWqFxL0dA+kTz2uc RNSPIVy75p4LrE/mJBnwHLx/1tB3SJqNOxdimbYNR7+YP/fpUq6Bh7o1mhZJID8BxJJNDvbRqTPsn9 uURfsmbYsTr7rdQtiXrc2MLqikfo69Xdwm5Ufx/hpJ16yjjaU1d4ft7iT1kjKRc0hIsLsZVJgOXKhW hKw0Mrd5QMh7dm0xeoH49RHCtw3HUPdFfH+OdzuwPnVoPBSUDZuvKvdZ+Z/vpKbtOVIl/n0gzG/97j 8o76yXgd56XlarYhCNPv1gMoPKB1G8/7NUdoBNRR2QbCYZpCWjKuzxzfQ+sdKw4VZ3qTCLa1zS3TLV A49o0bouLm+QU+00+amCZ0rjTUw4yIyFfOm31Sg3Shec09zsbbMGjf9ShNfNpzy0zxZEswGJzvcZTh w4R++sq98XmsmnzSX/87MTHp2cWhQxB276FXEZS0Unn1URmr+UqjmtV/wfkg== X-Developer-Key: i=keescook@chromium.org; a=openpgp; fpr=A5C3F68F229DD60F723E6E138972F4DFDC6DC026 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1754252956514683479?= X-GMAIL-MSGID: =?utf-8?q?1754252956514683479?= |
Series |
media: uvcvideo: Silence memcpy() run-time false positive warnings
|
|
Commit Message
Kees Cook
Jan. 6, 2023, 6:17 a.m. UTC
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(-)
Comments
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 >
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
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>
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
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);
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 > >
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;