All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] media: uvcvideo: Avoid invalid memory access
@ 2021-11-30 15:50 Ricardo Ribalda
  2021-11-30 15:50 ` [PATCH 2/2] media: uvcvideo: Avoid returning invalid controls Ricardo Ribalda
  2021-12-01  2:36 ` [PATCH 1/2] media: uvcvideo: Avoid invalid memory access Laurent Pinchart
  0 siblings, 2 replies; 5+ messages in thread
From: Ricardo Ribalda @ 2021-11-30 15:50 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, Hans Verkuil,
	linux-media, linux-kernel
  Cc: Ricardo Ribalda

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;
 	struct uvc_control *ctrl_found;
 	unsigned int i;
 
-- 
2.34.0.384.gca35af8252-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 2/2] media: uvcvideo: Avoid returning invalid controls
  2021-11-30 15:50 [PATCH 1/2] media: uvcvideo: Avoid invalid memory access Ricardo Ribalda
@ 2021-11-30 15:50 ` Ricardo Ribalda
  2021-12-01  2:39   ` Laurent Pinchart
  2021-12-01  2:36 ` [PATCH 1/2] media: uvcvideo: Avoid invalid memory access Laurent Pinchart
  1 sibling, 1 reply; 5+ messages in thread
From: Ricardo Ribalda @ 2021-11-30 15:50 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, Hans Verkuil,
	linux-media, linux-kernel
  Cc: Ricardo Ribalda

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 <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)
-- 
2.34.0.384.gca35af8252-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] media: uvcvideo: Avoid invalid memory access
  2021-11-30 15:50 [PATCH 1/2] media: uvcvideo: Avoid invalid memory access Ricardo Ribalda
  2021-11-30 15:50 ` [PATCH 2/2] media: uvcvideo: Avoid returning invalid controls Ricardo Ribalda
@ 2021-12-01  2:36 ` Laurent Pinchart
  2021-12-01  5:41   ` Ricardo Ribalda
  1 sibling, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2021-12-01  2:36 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, linux-kernel

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;
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] media: uvcvideo: Avoid returning invalid controls
  2021-11-30 15:50 ` [PATCH 2/2] media: uvcvideo: Avoid returning invalid controls Ricardo Ribalda
@ 2021-12-01  2:39   ` Laurent Pinchart
  0 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2021-12-01  2:39 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, linux-kernel

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)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] media: uvcvideo: Avoid invalid memory access
  2021-12-01  2:36 ` [PATCH 1/2] media: uvcvideo: Avoid invalid memory access Laurent Pinchart
@ 2021-12-01  5:41   ` Ricardo Ribalda
  0 siblings, 0 replies; 5+ messages in thread
From: Ricardo Ribalda @ 2021-12-01  5:41 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, linux-kernel

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



-- 
Ricardo Ribalda

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-01  5:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-30 15:50 [PATCH 1/2] media: uvcvideo: Avoid invalid memory access Ricardo Ribalda
2021-11-30 15:50 ` [PATCH 2/2] media: uvcvideo: Avoid returning invalid controls Ricardo Ribalda
2021-12-01  2:39   ` Laurent Pinchart
2021-12-01  2:36 ` [PATCH 1/2] media: uvcvideo: Avoid invalid memory access Laurent Pinchart
2021-12-01  5:41   ` Ricardo Ribalda

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.