| Message ID | 20221128103124.655264-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7] usb: gadget: uvc: add validate and fix function for uvc response | expand |
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@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); > } >
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 >
On 29/11/2022 10:23, Michael Grzeschik wrote: > 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. I'm also not really sure of the benefit; wouldn't this result in userspace streaming data that's configured differently to what the host is expecting? >>> 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; >>> + } >>> + } >>> + What about read requests for controls that aren't for the streaming interface? >>> return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL); >>> } >>> >> >> -- >> Regards, >> >> Laurent Pinchart >> >
Hi Michael, On Tue, Nov 29, 2022 at 11:23:08AM +0100, Michael Grzeschik wrote: > On Tue, Nov 29, 2022 at 05:10:24AM +0200, Laurent Pinchart wrote: > > 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. What bothers me is that this patch silently clamps invalid value, trying to hide the gadget userspace error from the host. It may allow the host to proceed one step further, but if the gadget userspace got it wrong in the first place, there's a very high chance it won't do the right thing in the next step anyway. This will make debugging more complicated, while at the same time not bringing much value. > >> 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); > >> } > >>
On Tue, Nov 29, 2022 at 02:02:02PM +0200, Laurent Pinchart wrote: >Hi Michael, > >On Tue, Nov 29, 2022 at 11:23:08AM +0100, Michael Grzeschik wrote: >> On Tue, Nov 29, 2022 at 05:10:24AM +0200, Laurent Pinchart wrote: >> > 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. > >What bothers me is that this patch silently clamps invalid value, trying >to hide the gadget userspace error from the host. It may allow the host >to proceed one step further, but if the gadget userspace got it wrong in >the first place, there's a very high chance it won't do the right thing >in the next step anyway. This will make debugging more complicated, >while at the same time not bringing much value. I discussed this and we came up with a better approach. When the userspace will send UVCIOC_SEND_RESPONSE we can return with a negativ return value. Like EAGAIN if the validation was seeeing some trouble with the userspaces uvc_streaming_control feedback to the host. The validation code will then still fixup the data, but instead of transfering this manipulated answer to the host, it will return the changes to the application with EAGAIN. So now the userspace can react to it and it should even point out misconfigurations between kernel and userspace and so will simplify the debugging. How about that? Michael
On Tue, Nov 29, 2022 at 04:22:59PM +0100, Michael Grzeschik wrote: >On Tue, Nov 29, 2022 at 02:02:02PM +0200, Laurent Pinchart wrote: >>On Tue, Nov 29, 2022 at 11:23:08AM +0100, Michael Grzeschik wrote: >>>On Tue, Nov 29, 2022 at 05:10:24AM +0200, Laurent Pinchart wrote: >>>> 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. >> >>What bothers me is that this patch silently clamps invalid value, trying >>to hide the gadget userspace error from the host. It may allow the host >>to proceed one step further, but if the gadget userspace got it wrong in >>the first place, there's a very high chance it won't do the right thing >>in the next step anyway. This will make debugging more complicated, >>while at the same time not bringing much value. > >I discussed this and we came up with a better approach. When the >userspace will send UVCIOC_SEND_RESPONSE we can return with a negativ >return value. Like EAGAIN if the validation was seeeing some trouble >with the userspaces uvc_streaming_control feedback to the host. > >The validation code will then still fixup the data, but instead of >transfering this manipulated answer to the host, it will return the >changes to the application with EAGAIN. So now the userspace can >react to it and it should even point out misconfigurations between >kernel and userspace and so will simplify the debugging. > >How about that? While implementing this I came across the problem that the UVCIOC_SEND_RESPONSE is handled in the vidioc_default handler. But for this handler we can not set flag INFO_FL_ALWAYS_COPY like for common v4l2_ioctls. :( I think this is still worth a path to go, but I am currently out of ideas how to achieve it. Help for this is much appreciated. Thanks, Michael
On Tue, Nov 29, 2022 at 04:22:59PM +0100, Michael Grzeschik wrote: > On Tue, Nov 29, 2022 at 02:02:02PM +0200, Laurent Pinchart wrote: > >Hi Michael, > > > >On Tue, Nov 29, 2022 at 11:23:08AM +0100, Michael Grzeschik wrote: > >> On Tue, Nov 29, 2022 at 05:10:24AM +0200, Laurent Pinchart wrote: > >> > 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. > > > >What bothers me is that this patch silently clamps invalid value, trying > >to hide the gadget userspace error from the host. It may allow the host > >to proceed one step further, but if the gadget userspace got it wrong in > >the first place, there's a very high chance it won't do the right thing > >in the next step anyway. This will make debugging more complicated, > >while at the same time not bringing much value. > > I discussed this and we came up with a better approach. When the > userspace will send UVCIOC_SEND_RESPONSE we can return with a negativ > return value. Like EAGAIN if the validation was seeeing some trouble > with the userspaces uvc_streaming_control feedback to the host. > > The validation code will then still fixup the data, but instead of > transfering this manipulated answer to the host, it will return the > changes to the application with EAGAIN. So now the userspace can > react to it and it should even point out misconfigurations between > kernel and userspace and so will simplify the debugging. > > How about that? It still adds pointless complexity to the kernel. I see no reason at all for this. You're trying to guard against userspace bugs by returning an error code to the application that has bugs in the first place. Why would it then handle the error correctly ? Please drop these changes.
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); }
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. 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(+)