[v5] usb: gadget: uvc: add validate and fix function for uvc response
Commit Message
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.
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
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, 80 insertions(+), 1 deletion(-)
Comments
On Wed, Oct 26, 2022 at 12:26:57AM +0200, 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.
>
> 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
What commit id does this fix? Validating userspace data is a good
thing, so shouldn't this also go to stable kernels?
thanks,
greg k-h
On Wed, Nov 09, 2022 at 11:23:10AM +0100, Greg KH wrote:
>On Wed, Oct 26, 2022 at 12:26:57AM +0200, 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.
>>
>> 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
>
>What commit id does this fix? Validating userspace data is a good
>thing, so shouldn't this also go to stable kernels?
This patch makes use of the uvc_get_frame_size function, which was
introduced with in v6.0.
"e219a712bc06 ("usb: gadget: uvc: add v4l2 try_format api call")
So this should not go in as a stable patch.
Thanks,
Michael
On Wed, Nov 09, 2022 at 03:04:12PM +0100, Michael Grzeschik wrote:
> On Wed, Nov 09, 2022 at 11:23:10AM +0100, Greg KH wrote:
> > On Wed, Oct 26, 2022 at 12:26:57AM +0200, 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.
> > >
> > > 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
> >
> > What commit id does this fix? Validating userspace data is a good
> > thing, so shouldn't this also go to stable kernels?
>
> This patch makes use of the uvc_get_frame_size function, which was
> introduced with in v6.0.
>
> "e219a712bc06 ("usb: gadget: uvc: add v4l2 try_format api call")
>
> So this should not go in as a stable patch.
So why wouldn't 6.0 and 6.1 need this?
confused,
greg k-h
On Wed, Nov 09, 2022 at 03:10:55PM +0100, Greg KH wrote:
>On Wed, Nov 09, 2022 at 03:04:12PM +0100, Michael Grzeschik wrote:
>> On Wed, Nov 09, 2022 at 11:23:10AM +0100, Greg KH wrote:
>> > On Wed, Oct 26, 2022 at 12:26:57AM +0200, 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.
>> > >
>> > > 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
>> >
>> > What commit id does this fix? Validating userspace data is a good
>> > thing, so shouldn't this also go to stable kernels?
>>
>> This patch makes use of the uvc_get_frame_size function, which was
>> introduced with in v6.0.
>>
>> "e219a712bc06 ("usb: gadget: uvc: add v4l2 try_format api call")
>>
>> So this should not go in as a stable patch.
>
>So why wouldn't 6.0 and 6.1 need this?
It would apply, but since it is a feature I would not tag it as a fix.
However you can add:
Fixes: e219a712bc06 ("usb: gadget: uvc: add v4l2 try_format api call")
Thanks,
Michael
On Wed, Oct 26, 2022 at 12:26:57AM +0200, 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.
>
> 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
>
> drivers/usb/gadget/function/f_uvc.c | 4 +-
Does not apply to 6.1-rc6 :(
Please rebase and resubmit.
thanks,
greg k-h
On Tue, Nov 22, 2022 at 04:51:15PM +0100, Greg KH wrote:
>On Wed, Oct 26, 2022 at 12:26:57AM +0200, 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.
>>
>> 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
>>
>> drivers/usb/gadget/function/f_uvc.c | 4 +-
>
>Does not apply to 6.1-rc6 :(
>
>Please rebase and resubmit.
This is, because it is based on
d182bf156c4c ("usb: gadget: uvc: default the ctrl request interface offsets")
which is currently in your usb-testing tree but not on v6.1-rc6.
Since I don't think that this is a fix but a feature. It probably be
better be also on the usb-testing tree aswell.
Thanks,
Michael
@@ -254,8 +254,10 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
*/
mctrl = &uvc_event->req;
mctrl->wIndex &= ~cpu_to_le16(0xff);
- if (interface == uvc->streaming_intf)
+ if (interface == uvc->streaming_intf) {
+ uvc->streaming_request = ctrl->bRequest;
mctrl->wIndex = cpu_to_le16(UVC_STRING_STREAMING_IDX);
+ }
v4l2_event_queue(&uvc->vdev, &v4l2_event);
@@ -151,6 +151,7 @@ struct uvc_device {
void *control_buf;
unsigned int streaming_intf;
+ unsigned char streaming_request;
/* Events */
unsigned int event_length;
@@ -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);
}