2021-11-30 15:53:12

by Ricardo Ribalda

Subject: [PATCH 1/2] media: uvcvideo: Avoid invalid memory access

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 <[email protected]>
---
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;
struct uvc_control *ctrl_found;
unsigned int i;

--
2.34.0.384.gca35af8252-goog

2021-11-30 15:53:17

by Ricardo Ribalda

Subject: [PATCH 2/2] media: uvcvideo: Avoid returning invalid controls

If the memory where ctrl_found is places has the value of uvc_ctrl and
__uvc_find_control does not find the control we will return and invalid
index.

Fixes: 6350d6a4ed487 ("media: uvcvideo: Set error_idx during ctrl_commit errors")
Signed-off-by: Ricardo Ribalda <[email protected]>
---
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)
--
2.34.0.384.gca35af8252-goog

2021-12-01 02:37:41

by Laurent Pinchart

Subject: Re: [PATCH 1/2] media: uvcvideo: Avoid invalid memory access

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 <[email protected]>
> ---
> 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;
>

--
Regards,

Laurent Pinchart

2021-12-01 02:39:51

by Laurent Pinchart

Subject: Re: [PATCH 2/2] media: uvcvideo: Avoid returning invalid controls

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 <[email protected]>

> Fixes: 6350d6a4ed487 ("media: uvcvideo: Set error_idx during ctrl_commit errors")
> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> 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)

--
Regards,

Laurent Pinchart

2021-12-01 05:41:47

by Ricardo Ribalda

Subject: Re: [PATCH 1/2] media: uvcvideo: Avoid invalid memory access

Hi Laurent

Thanks for the prompt reply :)

On Wed, 1 Dec 2021 at 03:37, Laurent Pinchart
<[email protected]> 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 <[email protected]>
> > ---
> > 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



--
Ricardo Ribalda