| Message ID | 20211130155026.1082594-1-ribalda@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: uvcvideo: Avoid invalid memory access | expand |
Hi Ricardo, Thank you for the patch. On Tue, Nov 30, 2021 at 03:50:25PM +0000, Ricardo Ribalda wrote: > If mappings points to an invalid memory, we will be invalid accessing > it. > Solve it by initializing the value of the variable mapping and by > changing the order in the conditional statement (to avoid accessing > mapping->id if not needed). > > Fix: > kasan: GPF could be caused by NULL-ptr deref or user memory access > general protection fault: 0000 [#1] PREEMPT SMP KASAN NOPTI > > Fixes: 6350d6a4ed487 ("media: uvcvideo: Set error_idx during ctrl_commit errors") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 30bfe9069a1fb..f7b7add3cfa59 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -852,8 +852,8 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > return; > } > > - if ((*mapping == NULL || (*mapping)->id > map->id) && > - (map->id > v4l2_id) && next) { > + if (next && (map->id > v4l2_id) && > + (*mapping == NULL || (*mapping)->id > map->id)) { > *control = ctrl; > *mapping = map; > } > @@ -1638,7 +1638,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > struct v4l2_ext_controls *ctrls, > struct uvc_control *uvc_control) > { > - struct uvc_control_mapping *mapping; > + struct uvc_control_mapping *mapping = NULL; It seems to me that either change will fix the bug, we don't need both, is that right ? If so I'd drop the change to __uvc_find_control(), as it seems quite fragile to allow mapping to be uninitialized. > struct uvc_control *ctrl_found; > unsigned int i; >
Hi Ricardo, Thank you for the patch. On Tue, Nov 30, 2021 at 03:50:26PM +0000, Ricardo Ribalda wrote: > If the memory where ctrl_found is places has the value of uvc_ctrl and s/places/placed/ s/uvc_ctrl/uvc_control/ > __uvc_find_control does not find the control we will return and invalid s/and invalid/an invalid/ > index. The change of this happening is small, but it exists. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Fixes: 6350d6a4ed487 ("media: uvcvideo: Set error_idx during ctrl_commit errors") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index f7b7add3cfa59..f1f6bb14fb0a6 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1639,7 +1639,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > struct uvc_control *uvc_control) > { > struct uvc_control_mapping *mapping = NULL; > - struct uvc_control *ctrl_found; > + struct uvc_control *ctrl_found = NULL; > unsigned int i; > > if (!entity)
Hi Laurent Thanks for the prompt reply :) On Wed, 1 Dec 2021 at 03:37, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Tue, Nov 30, 2021 at 03:50:25PM +0000, Ricardo Ribalda wrote: > > If mappings points to an invalid memory, we will be invalid accessing > > it. > > Solve it by initializing the value of the variable mapping and by > > changing the order in the conditional statement (to avoid accessing > > mapping->id if not needed). > > > > Fix: > > kasan: GPF could be caused by NULL-ptr deref or user memory access > > general protection fault: 0000 [#1] PREEMPT SMP KASAN NOPTI > > > > Fixes: 6350d6a4ed487 ("media: uvcvideo: Set error_idx during ctrl_commit errors") > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 30bfe9069a1fb..f7b7add3cfa59 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -852,8 +852,8 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > return; > > } > > > > - if ((*mapping == NULL || (*mapping)->id > map->id) && > > - (map->id > v4l2_id) && next) { > > + if (next && (map->id > v4l2_id) && > > + (*mapping == NULL || (*mapping)->id > map->id)) { > > *control = ctrl; > > *mapping = map; > > } > > @@ -1638,7 +1638,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > > struct v4l2_ext_controls *ctrls, > > struct uvc_control *uvc_control) > > { > > - struct uvc_control_mapping *mapping; > > + struct uvc_control_mapping *mapping = NULL; > > It seems to me that either change will fix the bug, we don't need both, > is that right ? If so I'd drop the change to __uvc_find_control(), as it > seems quite fragile to allow mapping to be uninitialized. Just wanted to be extra paranoid. I have just sent a v2 of the patch. Thanks! > > > struct uvc_control *ctrl_found; > > unsigned int i; > > > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 30bfe9069a1fb..f7b7add3cfa59 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -852,8 +852,8 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, return; } - if ((*mapping == NULL || (*mapping)->id > map->id) && - (map->id > v4l2_id) && next) { + if (next && (map->id > v4l2_id) && + (*mapping == NULL || (*mapping)->id > map->id)) { *control = ctrl; *mapping = map; } @@ -1638,7 +1638,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, struct v4l2_ext_controls *ctrls, struct uvc_control *uvc_control) { - struct uvc_control_mapping *mapping; + struct uvc_control_mapping *mapping = NULL; struct uvc_control *ctrl_found; unsigned int i;
If mappings points to an invalid memory, we will be invalid accessing it. Solve it by initializing the value of the variable mapping and by changing the order in the conditional statement (to avoid accessing mapping->id if not needed). Fix: kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] PREEMPT SMP KASAN NOPTI Fixes: 6350d6a4ed487 ("media: uvcvideo: Set error_idx during ctrl_commit errors") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)