Re: [PATCH v7] usb: gadget: uvc: add validate and fix function for uvc response

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Tue, Nov 29, 2022 at 05:10:24AM +0200, Laurent Pinchart wrote:
>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 ?

Why is it not? We don't want the userspace to communicate other things
to the host than what is configured in the configfs. If you only object
the explanation, then I will improve the commit message and send an
fixed v8. If you have more objections please share your doubts, thanks.

Michael

>> Fixes: e219a712bc06 ("usb: gadget: uvc: add v4l2 try_format api call")
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> ---
>> 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
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]