| 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
| ||
|
Message-ID: <Y6L02NgsTLKN47Z0@pendragon.ideasonboard.com>
Date: Wed, 21 Dec 2022 13:58:16 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Yunke Cao <yunkec@...omium.org>, Max Staudt <mstaudt@...omium.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-media@...r.kernel.org,
Sergey Senozhatsky <senozhatsky@...omium.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] media: uvcvideo: Remove void casting for the status
endpoint
Hi Ricardo,
Thank you for the patch.
On Tue, Dec 20, 2022 at 11:56:44PM +0100, Ricardo Ribalda wrote:
> Make the code more resiliant, by replacing the castings with proper
> structure definitions and using offsetof() instead of open coding the
> location of the data.
>
> Suggested-by: Sergey Senozhatsky <senozhatsky@...omium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
> media: uvcvideo: Code cleanup for dev->status
>
> Lets remove all the castings and open coding of offsets for it.
>
> To: Yunke Cao <yunkec@...omium.org>
> To: Sergey Senozhatsky <senozhatsky@...omium.org>
> To: Max Staudt <mstaudt@...omium.org>
> To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> To: Mauro Carvalho Chehab <mchehab@...nel.org>
> Cc: linux-media@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> ---
> Changes in v4:
> - Fix sizeof() error.
> - Keep kzalloc(). Thanks Alan, Christoph and Jonathan
> - Reducing the cc: to:
> - Link to v3: https://lore.kernel.org/r/20221214-uvc-status-alloc-v3-0-9a67616cc549@chromium.org
>
> Changes in v3:
> - Split the patch in two
> - Add linux-usb, Alan and Christoph for the allocation change.
> - Link to v2: https://lore.kernel.org/r/20221214-uvc-status-alloc-v2-0-3f1cba6fc734@chromium.org
>
> Changes in v2:
> - using __aligned(), to keep the old alignment
> - Adding Johnathan Cameron to:, as he has some similar experience with iio
> - Adding Ming Lei, as this patch kind of revert his patch.
> - Link to v1: https://lore.kernel.org/r/20221214-uvc-status-alloc-v1-0-a0098ddc7c93@chromium.org
> ---
> drivers/media/usb/uvc/uvc_status.c | 66 +++++++++++++-------------------------
> drivers/media/usb/uvc/uvcvideo.h | 25 +++++++++++++--
> 2 files changed, 45 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index 7518ffce22ed..00f4036d0683 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -73,38 +73,24 @@ static void uvc_input_report_key(struct uvc_device *dev, unsigned int code,
> /* --------------------------------------------------------------------------
> * Status interrupt endpoint
> */
> -struct uvc_streaming_status {
> - u8 bStatusType;
> - u8 bOriginator;
> - u8 bEvent;
> - u8 bValue[];
> -} __packed;
> -
> -struct uvc_control_status {
> - u8 bStatusType;
> - u8 bOriginator;
> - u8 bEvent;
> - u8 bSelector;
> - u8 bAttribute;
> - u8 bValue[];
> -} __packed;
> -
> static void uvc_event_streaming(struct uvc_device *dev,
> - struct uvc_streaming_status *status, int len)
> + struct uvc_status *status, int len)
> {
> - if (len < 3) {
> + if (len <= offsetof(struct uvc_status, bEvent)) {
> uvc_dbg(dev, STATUS,
> "Invalid streaming status event received\n");
> return;
> }
>
> if (status->bEvent == 0) {
> - if (len < 4)
> + if (len <= offsetof(struct uvc_status, streaming))
> return;
> +
> uvc_dbg(dev, STATUS, "Button (intf %u) %s len %d\n",
> status->bOriginator,
> - status->bValue[0] ? "pressed" : "released", len);
> - uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
> + status->streaming.button ? "pressed" : "released", len);
> + uvc_input_report_key(dev, KEY_CAMERA,
> + status->streaming.button);
This holds on a single line.
I'll fix this small issue when applying.
Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> } else {
> uvc_dbg(dev, STATUS, "Stream %u error event %02x len %d\n",
> status->bOriginator, status->bEvent, len);
> @@ -131,7 +117,7 @@ static struct uvc_control *uvc_event_entity_find_ctrl(struct uvc_entity *entity,
> }
>
> static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> - const struct uvc_control_status *status,
> + const struct uvc_status *status,
> struct uvc_video_chain **chain)
> {
> list_for_each_entry((*chain), &dev->chains, list) {
> @@ -143,7 +129,7 @@ static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> continue;
>
> ctrl = uvc_event_entity_find_ctrl(entity,
> - status->bSelector);
> + status->control.bSelector);
> if (ctrl)
> return ctrl;
> }
> @@ -153,7 +139,7 @@ static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> }
>
> static bool uvc_event_control(struct urb *urb,
> - const struct uvc_control_status *status, int len)
> + const struct uvc_status *status, int len)
> {
> static const char *attrs[] = { "value", "info", "failure", "min", "max" };
> struct uvc_device *dev = urb->context;
> @@ -161,24 +147,24 @@ static bool uvc_event_control(struct urb *urb,
> struct uvc_control *ctrl;
>
> if (len < 6 || status->bEvent != 0 ||
> - status->bAttribute >= ARRAY_SIZE(attrs)) {
> + status->control.bAttribute >= ARRAY_SIZE(attrs)) {
> uvc_dbg(dev, STATUS, "Invalid control status event received\n");
> return false;
> }
>
> uvc_dbg(dev, STATUS, "Control %u/%u %s change len %d\n",
> - status->bOriginator, status->bSelector,
> - attrs[status->bAttribute], len);
> + status->bOriginator, status->control.bSelector,
> + attrs[status->control.bAttribute], len);
>
> /* Find the control. */
> ctrl = uvc_event_find_ctrl(dev, status, &chain);
> if (!ctrl)
> return false;
>
> - switch (status->bAttribute) {
> + switch (status->control.bAttribute) {
> case UVC_CTRL_VALUE_CHANGE:
> return uvc_ctrl_status_event_async(urb, chain, ctrl,
> - status->bValue);
> + status->control.bValue);
>
> case UVC_CTRL_INFO_CHANGE:
> case UVC_CTRL_FAILURE_CHANGE:
> @@ -214,28 +200,22 @@ static void uvc_status_complete(struct urb *urb)
>
> len = urb->actual_length;
> if (len > 0) {
> - switch (dev->status[0] & 0x0f) {
> + switch (dev->status->bStatusType & 0x0f) {
> case UVC_STATUS_TYPE_CONTROL: {
> - struct uvc_control_status *status =
> - (struct uvc_control_status *)dev->status;
> -
> - if (uvc_event_control(urb, status, len))
> + if (uvc_event_control(urb, dev->status, len))
> /* The URB will be resubmitted in work context. */
> return;
> break;
> }
>
> case UVC_STATUS_TYPE_STREAMING: {
> - struct uvc_streaming_status *status =
> - (struct uvc_streaming_status *)dev->status;
> -
> - uvc_event_streaming(dev, status, len);
> + uvc_event_streaming(dev, dev->status, len);
> break;
> }
>
> default:
> uvc_dbg(dev, STATUS, "Unknown status event type %u\n",
> - dev->status[0]);
> + dev->status->bStatusType);
> break;
> }
> }
> @@ -259,12 +239,12 @@ int uvc_status_init(struct uvc_device *dev)
>
> uvc_input_init(dev);
>
> - dev->status = kzalloc(UVC_MAX_STATUS_SIZE, GFP_KERNEL);
> - if (dev->status == NULL)
> + dev->status = kzalloc(sizeof(*dev->status), GFP_KERNEL);
> + if (!dev->status)
> return -ENOMEM;
>
> dev->int_urb = usb_alloc_urb(0, GFP_KERNEL);
> - if (dev->int_urb == NULL) {
> + if (!dev->int_urb) {
> kfree(dev->status);
> return -ENOMEM;
> }
> @@ -281,7 +261,7 @@ int uvc_status_init(struct uvc_device *dev)
> interval = fls(interval) - 1;
>
> usb_fill_int_urb(dev->int_urb, dev->udev, pipe,
> - dev->status, UVC_MAX_STATUS_SIZE, uvc_status_complete,
> + dev->status, sizeof(*dev->status), uvc_status_complete,
> dev, interval);
>
> return 0;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..84326991ec36 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -51,8 +51,6 @@
> #define UVC_URBS 5
> /* Maximum number of packets per URB. */
> #define UVC_MAX_PACKETS 32
> -/* Maximum status buffer size in bytes of interrupt URB. */
> -#define UVC_MAX_STATUS_SIZE 16
>
> #define UVC_CTRL_CONTROL_TIMEOUT 5000
> #define UVC_CTRL_STREAMING_TIMEOUT 5000
> @@ -527,6 +525,26 @@ struct uvc_device_info {
> const struct uvc_control_mapping **mappings;
> };
>
> +struct uvc_status_streaming {
> + u8 button;
> +} __packed;
> +
> +struct uvc_status_control {
> + u8 bSelector;
> + u8 bAttribute;
> + u8 bValue[11];
> +} __packed;
> +
> +struct uvc_status {
> + u8 bStatusType;
> + u8 bOriginator;
> + u8 bEvent;
> + union {
> + struct uvc_status_control control;
> + struct uvc_status_streaming streaming;
> + };
> +} __packed;
> +
> struct uvc_device {
> struct usb_device *udev;
> struct usb_interface *intf;
> @@ -559,7 +577,8 @@ struct uvc_device {
> /* Status Interrupt Endpoint */
> struct usb_host_endpoint *int_ep;
> struct urb *int_urb;
> - u8 *status;
> + struct uvc_status *status;
> +
> struct input_dev *input;
> char input_phys[64];
>
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists