| Message ID | 20221130132855.1263114-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v8] usb: gadget: uvc: add validate and fix function for uvc response | expand |
Hi Michael, On Wed, Nov 30, 2022 at 02:28:55PM +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 we can > return EINVAL if the userspace does try to send invalid data to the > host. > > For UVC_GET_MAX and UVC_GET_MIN we fixup the bFrameIndex and > bFormatIndex to the correct boundaries. And for all other requests we > set valid dwFrameInterval, dwMaxPayloadTransferSize and > dwMaxVideoFrameSize if the userspace did not set any value at all. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> For the reason explained in the review of v7, NACK. > --- > 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")' > v7 -> v8: > - splitup validate and fix to separate functions > - only validate on UVC_GET_CUR > - return with EINVAL if validate fails > - fixup on UVC_GET_MIN, UVC_GET_MAX and UVC_GET_DEF > - rebased again on necessary patch 'd182bf156c4c ("usb: gadget: uvc: default the ctrl request interface offsets")' > - after validating one streaming request unset the current streaming_request > > drivers/usb/gadget/function/f_uvc.c | 4 +- > drivers/usb/gadget/function/uvc.h | 1 + > drivers/usb/gadget/function/uvc_v4l2.c | 154 +++++++++++++++++++++++++ > 3 files changed, 158 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > index 6e131624011a5e..098bd3c4e3c0b3 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -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); > > 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..a5a109e7708f4b 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -178,6 +178,137 @@ static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc, > * Requests handling > */ > > +/* validate streaming ctrl request response data for valid parameters */ > +static int > +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; > + > + iformat = clamp((unsigned int)iformat, 1U, > + (unsigned int)uvc->header->num_fmt); > + if (iformat != ctrl->bFormatIndex) { > + uvcg_dbg(&uvc->func, > + "invalid bFormatIndex set: %d\n", > + ctrl->bFormatIndex); > + return -EINVAL; > + } > + 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, > + "invalid bFrameIndex set: %d\n", > + ctrl->bFrameIndex); > + return -EINVAL; > + } > + 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; > + break; > + } > + } > + if (!ival_found) { > + uvcg_dbg(&uvc->func, > + "invalid dwFrameInterval set: %d\n", > + ctrl->dwFrameInterval); > + return -EINVAL; > + } > + } > + > + if (ctrl->dwMaxPayloadTransferSize > > + opts->streaming_maxpacket) { > + uvcg_dbg(&uvc->func, > + "invalid dwMaxPayloadTransferSize set: %d\n", > + ctrl->dwMaxPayloadTransferSize); > + return -EINVAL; > + } > + > + if (ctrl->dwMaxVideoFrameSize > > + uframe->frame.dw_max_video_frame_buffer_size) { > + uvcg_dbg(&uvc->func, > + "invalid dwMaxVideoFrameSize set: %d\n", > + ctrl->dwMaxVideoFrameSize); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static void > +uvc_fixup_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; > + > + iformat = ctrl->bFormatIndex; > + iframe = ctrl->bFrameIndex; > + > + /* Fixup the bFormatIndex on UVC_GET_{MIN,MAX} events */ > + if (ctrl->bFormatIndex == U8_MAX || !ctrl->bFormatIndex) { > + iformat = clamp((unsigned int)iformat, 1U, > + (unsigned int)uvc->header->num_fmt); > + ctrl->bFormatIndex = iformat; > + uvcg_dbg(&uvc->func, > + "fixup bFormatIndex: %d\n", > + ctrl->bFormatIndex); > + } > + > + uformat = find_format_by_index(uvc, iformat); > + > + /* Fixup the bFrameIndex on UVC_GET_{MIN,MAX} events */ > + if (ctrl->bFrameIndex == U8_MAX || !ctrl->bFrameIndex) { > + iframe = clamp((unsigned int)iframe, 1U, > + (unsigned int)uformat->num_frames); > + ctrl->bFrameIndex = iframe; > + uvcg_dbg(&uvc->func, > + "fixup bFrameIndex: %d\n", > + ctrl->bFrameIndex); > + } > + > + uframe = find_frame_by_index(uvc, uformat, iframe); > + > + /* Fixup values where the userspace does not set the paramters > + * but does handover the preparing of the values to the kernel. > + */ > + if (!ctrl->dwFrameInterval) { > + ctrl->dwFrameInterval = uframe->frame.dw_default_frame_interval; > + uvcg_dbg(&uvc->func, > + "fixup dwFrameInterval: %d\n", > + ctrl->dwFrameInterval); > + } > + > + if (!ctrl->dwMaxPayloadTransferSize) { > + ctrl->dwMaxPayloadTransferSize = opts->streaming_maxpacket; > + uvcg_dbg(&uvc->func, > + "fixup dwMaxPayloadTransferSize: %d\n", > + ctrl->dwMaxPayloadTransferSize); > + } > + > + if (!ctrl->dwMaxVideoFrameSize) { > + ctrl->dwMaxVideoFrameSize = uvc_get_frame_size(uformat, uframe); > + uvcg_dbg(&uvc->func, > + "fixup dwMaxVideoFrameSize: %d\n", > + ctrl->dwMaxVideoFrameSize); > + } > +} > + > static int > uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data) > { > @@ -192,6 +323,29 @@ 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; > + int ret; > + > + switch (uvc->streaming_request) { > + case UVC_GET_CUR: > + ret = uvc_validate_streaming_ctrl(uvc, ctrl); > + if (ret) > + return ret; > + fallthrough; > + case UVC_GET_MIN: > + case UVC_GET_MAX: > + case UVC_GET_DEF: > + uvc_fixup_streaming_ctrl(uvc, ctrl); > + break; > + default: > + break; > + } > + > + uvc->streaming_request = UVC_RC_UNDEFINED; > + } > + > return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL); > } >
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 6e131624011a5e..098bd3c4e3c0b3 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -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); 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..a5a109e7708f4b 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -178,6 +178,137 @@ static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc, * Requests handling */ +/* validate streaming ctrl request response data for valid parameters */ +static int +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; + + iformat = clamp((unsigned int)iformat, 1U, + (unsigned int)uvc->header->num_fmt); + if (iformat != ctrl->bFormatIndex) { + uvcg_dbg(&uvc->func, + "invalid bFormatIndex set: %d\n", + ctrl->bFormatIndex); + return -EINVAL; + } + 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, + "invalid bFrameIndex set: %d\n", + ctrl->bFrameIndex); + return -EINVAL; + } + 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; + break; + } + } + if (!ival_found) { + uvcg_dbg(&uvc->func, + "invalid dwFrameInterval set: %d\n", + ctrl->dwFrameInterval); + return -EINVAL; + } + } + + if (ctrl->dwMaxPayloadTransferSize > + opts->streaming_maxpacket) { + uvcg_dbg(&uvc->func, + "invalid dwMaxPayloadTransferSize set: %d\n", + ctrl->dwMaxPayloadTransferSize); + return -EINVAL; + } + + if (ctrl->dwMaxVideoFrameSize > + uframe->frame.dw_max_video_frame_buffer_size) { + uvcg_dbg(&uvc->func, + "invalid dwMaxVideoFrameSize set: %d\n", + ctrl->dwMaxVideoFrameSize); + return -EINVAL; + } + + return 0; +} + +static void +uvc_fixup_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; + + iformat = ctrl->bFormatIndex; + iframe = ctrl->bFrameIndex; + + /* Fixup the bFormatIndex on UVC_GET_{MIN,MAX} events */ + if (ctrl->bFormatIndex == U8_MAX || !ctrl->bFormatIndex) { + iformat = clamp((unsigned int)iformat, 1U, + (unsigned int)uvc->header->num_fmt); + ctrl->bFormatIndex = iformat; + uvcg_dbg(&uvc->func, + "fixup bFormatIndex: %d\n", + ctrl->bFormatIndex); + } + + uformat = find_format_by_index(uvc, iformat); + + /* Fixup the bFrameIndex on UVC_GET_{MIN,MAX} events */ + if (ctrl->bFrameIndex == U8_MAX || !ctrl->bFrameIndex) { + iframe = clamp((unsigned int)iframe, 1U, + (unsigned int)uformat->num_frames); + ctrl->bFrameIndex = iframe; + uvcg_dbg(&uvc->func, + "fixup bFrameIndex: %d\n", + ctrl->bFrameIndex); + } + + uframe = find_frame_by_index(uvc, uformat, iframe); + + /* Fixup values where the userspace does not set the paramters + * but does handover the preparing of the values to the kernel. + */ + if (!ctrl->dwFrameInterval) { + ctrl->dwFrameInterval = uframe->frame.dw_default_frame_interval; + uvcg_dbg(&uvc->func, + "fixup dwFrameInterval: %d\n", + ctrl->dwFrameInterval); + } + + if (!ctrl->dwMaxPayloadTransferSize) { + ctrl->dwMaxPayloadTransferSize = opts->streaming_maxpacket; + uvcg_dbg(&uvc->func, + "fixup dwMaxPayloadTransferSize: %d\n", + ctrl->dwMaxPayloadTransferSize); + } + + if (!ctrl->dwMaxVideoFrameSize) { + ctrl->dwMaxVideoFrameSize = uvc_get_frame_size(uformat, uframe); + uvcg_dbg(&uvc->func, + "fixup dwMaxVideoFrameSize: %d\n", + ctrl->dwMaxVideoFrameSize); + } +} + static int uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data) { @@ -192,6 +323,29 @@ 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; + int ret; + + switch (uvc->streaming_request) { + case UVC_GET_CUR: + ret = uvc_validate_streaming_ctrl(uvc, ctrl); + if (ret) + return ret; + fallthrough; + case UVC_GET_MIN: + case UVC_GET_MAX: + case UVC_GET_DEF: + uvc_fixup_streaming_ctrl(uvc, ctrl); + break; + default: + break; + } + + uvc->streaming_request = UVC_RC_UNDEFINED; + } + 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 we can return EINVAL if the userspace does try to send invalid data to the host. For UVC_GET_MAX and UVC_GET_MIN we fixup the bFrameIndex and bFormatIndex to the correct boundaries. And for all other requests we set valid dwFrameInterval, dwMaxPayloadTransferSize and dwMaxVideoFrameSize if the userspace did not set any value at all. 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")' v7 -> v8: - splitup validate and fix to separate functions - only validate on UVC_GET_CUR - return with EINVAL if validate fails - fixup on UVC_GET_MIN, UVC_GET_MAX and UVC_GET_DEF - rebased again on necessary patch 'd182bf156c4c ("usb: gadget: uvc: default the ctrl request interface offsets")' - after validating one streaming request unset the current streaming_request drivers/usb/gadget/function/f_uvc.c | 4 +- drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 154 +++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 1 deletion(-)