Hi Michael,
(CC'ing Dan)
Thank you for the patch.
On Mon, Nov 28, 2022 at 11:31:25AM +0100, Michael Grzeschik wrote:
> When the userspace gets the setup requests for UVC_GET_CUR UVC_GET_MIN,
> UVC_GET_MAX, UVC_GET_DEF it will fill out the ctrl response. This data
> needs to be validated. Since the kernel also knows the limits for valid
> cases, it can fixup the values in case the userspace is setting invalid
> data.
Why is this a good idea ?
> Fixes: e219a712bc06 ("usb: gadget: uvc: add v4l2 try_format api call")
> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
>
> ---
> v1: -> v4:
> - new patch
> v4: -> v5:
> - changed uvcg_info to uvcg_dbg for fixups, updated info strings
> v5: -> v6:
> - no changes
> v6 -> v7:
> - reworked to not need 'd182bf156c4c ("usb: gadget: uvc: default the ctrl request interface offsets")'
>
> This will apply to v6.1-rc6.
>
> drivers/usb/gadget/function/f_uvc.c | 4 ++
> drivers/usb/gadget/function/uvc.h | 1 +
> drivers/usb/gadget/function/uvc_v4l2.c | 76 ++++++++++++++++++++++++++
> 3 files changed, 81 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 6e196e06181ecf..89f0100dae60f4 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -248,6 +248,10 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
> memset(&v4l2_event, 0, sizeof(v4l2_event));
> v4l2_event.type = UVC_EVENT_SETUP;
> memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
> +
> + if (interface == uvc->streaming_intf)
> + uvc->streaming_request = ctrl->bRequest;
> +
> v4l2_event_queue(&uvc->vdev, &v4l2_event);
>
> return 0;
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 40226b1f7e148a..1be4d5f24b46bf 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -151,6 +151,7 @@ struct uvc_device {
> void *control_buf;
>
> unsigned int streaming_intf;
> + unsigned char streaming_request;
>
> /* Events */
> unsigned int event_length;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index a189b08bba800d..a12475d289167a 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -178,6 +178,67 @@ static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc,
> * Requests handling
> */
>
> +/* validate and fixup streaming ctrl request response data if possible */
> +static void
> +uvc_validate_streaming_ctrl(struct uvc_device *uvc,
> + struct uvc_streaming_control *ctrl)
> +{
> + struct f_uvc_opts *opts = fi_to_f_uvc_opts(uvc->func.fi);
> + unsigned int iformat, iframe;
> + struct uvcg_format *uformat;
> + struct uvcg_frame *uframe;
> + bool ival_found = false;
> + int i;
> +
> + iformat = ctrl->bFormatIndex;
> + iframe = ctrl->bFrameIndex;
> +
> + /* Restrict the iformat, iframe and dwFrameInterval to valid values.
> + * Negative values for iformat and iframe will result in the maximum
> + * valid value being selected
> + */
> + iformat = clamp((unsigned int)iformat, 1U,
> + (unsigned int)uvc->header->num_fmt);
> + if (iformat != ctrl->bFormatIndex) {
> + uvcg_dbg(&uvc->func,
> + "userspace set invalid format index - fixup\n");
> + ctrl->bFormatIndex = iformat;
> + }
> + uformat = find_format_by_index(uvc, iformat);
> +
> + iframe = clamp((unsigned int)iframe, 1U,
> + (unsigned int)uformat->num_frames);
> + if (iframe != ctrl->bFrameIndex) {
> + uvcg_dbg(&uvc->func,
> + "userspace set invalid frame index - fixup\n");
> + ctrl->bFrameIndex = iframe;
> + }
> + uframe = find_frame_by_index(uvc, uformat, iframe);
> +
> + if (ctrl->dwFrameInterval) {
> + for (i = 0; i < uframe->frame.b_frame_interval_type; i++) {
> + if (ctrl->dwFrameInterval ==
> + uframe->dw_frame_interval[i])
> + ival_found = true;
> + }
> + }
> + if (!ival_found) {
> + uvcg_dbg(&uvc->func,
> + "userspace set invalid frame interval - fixup\n");
> + ctrl->dwFrameInterval = uframe->frame.dw_default_frame_interval;
> + }
> +
> + if (!ctrl->dwMaxPayloadTransferSize ||
> + ctrl->dwMaxPayloadTransferSize >
> + opts->streaming_maxpacket)
> + ctrl->dwMaxPayloadTransferSize = opts->streaming_maxpacket;
> +
> + if (!ctrl->dwMaxVideoFrameSize ||
> + ctrl->dwMaxVideoFrameSize >
> + uframe->frame.dw_max_video_frame_buffer_size)
> + ctrl->dwMaxVideoFrameSize = uvc_get_frame_size(uformat, uframe);
> +}
> +
> static int
> uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
> {
> @@ -192,6 +253,21 @@ uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
>
> memcpy(req->buf, data->data, req->length);
>
> + /* validate the ctrl content and fixup */
> + if (!uvc->event_setup_out) {
> + struct uvc_streaming_control *ctrl = req->buf;
> +
> + switch (uvc->streaming_request) {
> + case UVC_GET_CUR:
> + case UVC_GET_MIN:
> + case UVC_GET_MAX:
> + case UVC_GET_DEF:
> + uvc_validate_streaming_ctrl(uvc, ctrl);
> + default:
> + break;
> + }
> + }
> +
> return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL);
> }
>
--
Regards,
Laurent Pinchart
