[PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[Dan Carpenter]

The "idx" value is a user controlled u32 so we have to bounds check it
before calling test_bit() to avoid reading beyond the end of the bitmap.

Fixes: 4e15c535659b ("media: uvcvideo: Support any size for mapping get/set")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
 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 3ca108b83f1d..f157ed99be4e 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -538,7 +538,7 @@ static void uvc_mapping_set_s32(struct uvc_control_mapping *mapping,
 static int uvc_mapping_get_menu_value(const struct uvc_control_mapping *mapping,
 				      u32 idx)
 {
-	if (!test_bit(idx, &mapping->menu_mask))
+	if (idx >= BITS_PER_LONG || !test_bit(idx, &mapping->menu_mask))
 		return -EINVAL;
 
 	if (mapping->menu_mapping)
-- 
2.53.0

Re: [PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[Ricardo Ribalda]

Hi Dan

I believe that for all the uses of uvc_mapping_get_menu_value we are
already doing bound checks:

index >= BITS_PER_TYPE(mapping->menu_mask) in uvc_query_v4l2_menu()
value> fls(mapping->menu_mask) -1 in uvc_ctrl_clamp()
BIT(i) <= mapping->menu_mask in uvc_menu_to_v4l2_menu()

In any case, I think this patch is still worthwhile. It will help us
avoiding bugs in the future (have you found this with a new test for
smatch?)
and if we go that way it is probably a good idea to also "fix"
uvc_mapping_get_menu_name()


On Fri, 18 Sept 2026 at 14:20, Dan Carpenter <error27@gmail.com> wrote:
>
> The "idx" value is a user controlled u32 so we have to bounds check it
> before calling test_bit() to avoid reading beyond the end of the bitmap.
Maybe change the commit message as well to avoid stable cherry picking
it blindly (sorry seems like I am more picky than usual  :) )

>
> Fixes: 4e15c535659b ("media: uvcvideo: Support any size for mapping get/set")
I believe this should be:
Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
or no Fixes at all.



> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
>  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 3ca108b83f1d..f157ed99be4e 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -538,7 +538,7 @@ static void uvc_mapping_set_s32(struct uvc_control_mapping *mapping,
>  static int uvc_mapping_get_menu_value(const struct uvc_control_mapping *mapping,
>                                       u32 idx)
>  {
> -       if (!test_bit(idx, &mapping->menu_mask))
> +       if (idx >= BITS_PER_LONG || !test_bit(idx, &mapping->menu_mask))
>                 return -EINVAL;

What about using BITS_PER_TYPE(mapping->menu_mask) to be consistent?

>
>         if (mapping->menu_mapping)
> --
> 2.53.0
>

with those changes:
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[Dan Carpenter]

On Fri, Sep 18, 2026 at 02:59:29PM +0200, Ricardo Ribalda wrote:
> Hi Dan
> 
> I believe that for all the uses of uvc_mapping_get_menu_value we are
> already doing bound checks:
> 
> index >= BITS_PER_TYPE(mapping->menu_mask) in uvc_query_v4l2_menu()
> value> fls(mapping->menu_mask) -1 in uvc_ctrl_clamp()
> BIT(i) <= mapping->menu_mask in uvc_menu_to_v4l2_menu()
>

The problematic caller is uvc_set_le_value().

	value = *(s32 *)v4l2_in;

Smatch thinks that is called from uvc_mapping_set_xctrl_compound().

drivers/media/usb/uvc/uvc_ctrl.c
  2772          data = memdup_user(xctrl->ptr, size);
  2773          if (IS_ERR(data))
  2774                  return PTR_ERR(data);
  2775  
  2776          return mapping->set(mapping, size, data,
  2777                              uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));

But maybe that mapping->set can't point to uvc_set_le_value.

> In any case, I think this patch is still worthwhile. It will help us
> avoiding bugs in the future (have you found this with a new test for
> smatch?)
> and if we go that way it is probably a good idea to also "fix"
> uvc_mapping_get_menu_name()
> 
> 
> On Fri, 18 Sept 2026 at 14:20, Dan Carpenter <error27@gmail.com> wrote:
> >
> > The "idx" value is a user controlled u32 so we have to bounds check it
> > before calling test_bit() to avoid reading beyond the end of the bitmap.
> Maybe change the commit message as well to avoid stable cherry picking
> it blindly (sorry seems like I am more picky than usual  :) )
> 

I mean if uvc_set_le_value() turns out not to be a bug then, of course.

> >
> > Fixes: 4e15c535659b ("media: uvcvideo: Support any size for mapping get/set")
> I believe this should be:
> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> or no Fixes at all.
> 
> 
> 
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> >  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 3ca108b83f1d..f157ed99be4e 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -538,7 +538,7 @@ static void uvc_mapping_set_s32(struct uvc_control_mapping *mapping,
> >  static int uvc_mapping_get_menu_value(const struct uvc_control_mapping *mapping,
> >                                       u32 idx)
> >  {
> > -       if (!test_bit(idx, &mapping->menu_mask))
> > +       if (idx >= BITS_PER_LONG || !test_bit(idx, &mapping->menu_mask))
> >                 return -EINVAL;
> 
> What about using BITS_PER_TYPE(mapping->menu_mask) to be consistent?
> 

Sure.

regards,
dan carpenter

Re: [PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[Ricardo Ribalda]

Hi Dan

On Fri, 18 Sept 2026 at 15:18, Dan Carpenter <error27@gmail.com> wrote:
>
> On Fri, Sep 18, 2026 at 02:59:29PM +0200, Ricardo Ribalda wrote:
> > Hi Dan
> >
> > I believe that for all the uses of uvc_mapping_get_menu_value we are
> > already doing bound checks:
> >
> > index >= BITS_PER_TYPE(mapping->menu_mask) in uvc_query_v4l2_menu()
> > value> fls(mapping->menu_mask) -1 in uvc_ctrl_clamp()
> > BIT(i) <= mapping->menu_mask in uvc_menu_to_v4l2_menu()
> >
>
> The problematic caller is uvc_set_le_value().
>
>         value = *(s32 *)v4l2_in;
>
> Smatch thinks that is called from uvc_mapping_set_xctrl_compound().

uvc_mapping_set_xctrl_compound() are only called if v4l2_type >=
V4L2_CTRL_TYPE_RECT and then v4l2_type != V4L2_CTRL_TYPE_MENU. So I
think we are safe.

>
> drivers/media/usb/uvc/uvc_ctrl.c
>   2772          data = memdup_user(xctrl->ptr, size);
>   2773          if (IS_ERR(data))
>   2774                  return PTR_ERR(data);
>   2775
>   2776          return mapping->set(mapping, size, data,
>   2777                              uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
>
> But maybe that mapping->set can't point to uvc_set_le_value.
>
> > In any case, I think this patch is still worthwhile. It will help us
> > avoiding bugs in the future (have you found this with a new test for
> > smatch?)
> > and if we go that way it is probably a good idea to also "fix"
> > uvc_mapping_get_menu_name()
> >
> >
> > On Fri, 18 Sept 2026 at 14:20, Dan Carpenter <error27@gmail.com> wrote:
> > >
> > > The "idx" value is a user controlled u32 so we have to bounds check it
> > > before calling test_bit() to avoid reading beyond the end of the bitmap.
> > Maybe change the commit message as well to avoid stable cherry picking
> > it blindly (sorry seems like I am more picky than usual  :) )
> >
>
> I mean if uvc_set_le_value() turns out not to be a bug then, of course.
>
> > >
> > > Fixes: 4e15c535659b ("media: uvcvideo: Support any size for mapping get/set")
> > I believe this should be:
> > Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> > or no Fixes at all.
> >
> >
> >
> > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > > ---
> > >  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 3ca108b83f1d..f157ed99be4e 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -538,7 +538,7 @@ static void uvc_mapping_set_s32(struct uvc_control_mapping *mapping,
> > >  static int uvc_mapping_get_menu_value(const struct uvc_control_mapping *mapping,
> > >                                       u32 idx)
> > >  {
> > > -       if (!test_bit(idx, &mapping->menu_mask))
> > > +       if (idx >= BITS_PER_LONG || !test_bit(idx, &mapping->menu_mask))
> > >                 return -EINVAL;
> >
> > What about using BITS_PER_TYPE(mapping->menu_mask) to be consistent?
> >
>
> Sure.
>
> regards,
> dan carpenter
>


-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[Dan Carpenter]

On Fri, Sep 18, 2026 at 03:46:11PM +0200, Ricardo Ribalda wrote:
> Hi Dan
> 
> On Fri, 18 Sept 2026 at 15:18, Dan Carpenter <error27@gmail.com> wrote:
> >
> > On Fri, Sep 18, 2026 at 02:59:29PM +0200, Ricardo Ribalda wrote:
> > > Hi Dan
> > >
> > > I believe that for all the uses of uvc_mapping_get_menu_value we are
> > > already doing bound checks:
> > >
> > > index >= BITS_PER_TYPE(mapping->menu_mask) in uvc_query_v4l2_menu()
> > > value> fls(mapping->menu_mask) -1 in uvc_ctrl_clamp()
> > > BIT(i) <= mapping->menu_mask in uvc_menu_to_v4l2_menu()
> > >
> >
> > The problematic caller is uvc_set_le_value().
> >
> >         value = *(s32 *)v4l2_in;
> >
> > Smatch thinks that is called from uvc_mapping_set_xctrl_compound().
> 
> uvc_mapping_set_xctrl_compound() are only called if v4l2_type >=
> V4L2_CTRL_TYPE_RECT and then v4l2_type != V4L2_CTRL_TYPE_MENU. So I
> think we are safe.
> 

Yeah...  I have reprimanded ChatGPT and it says it has updated the
warning review skill.

This is difficult to silence.  But I'm going to ask AI to create
an tool to automatically rebuild a second temporary database which
only checks the problematic call tree and points any impossible
constraints.  That would have flagged this warning as a false
positive.

regards,
dan carpenter

Re: [PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[Laurent Pinchart]

On Fri, Sep 18, 2026 at 03:20:25PM +0300, Dan Carpenter wrote:
> The "idx" value is a user controlled u32 so we have to bounds check it
> before calling test_bit() to avoid reading beyond the end of the bitmap.
> 
> Fixes: 4e15c535659b ("media: uvcvideo: Support any size for mapping get/set")
> Signed-off-by: Dan Carpenter <error27@gmail.com>

What's the status of this patch ? The issue is a false positive, do you
still think there's value in this redundant test ?

> ---
>  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 3ca108b83f1d..f157ed99be4e 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -538,7 +538,7 @@ static void uvc_mapping_set_s32(struct uvc_control_mapping *mapping,
>  static int uvc_mapping_get_menu_value(const struct uvc_control_mapping *mapping,
>  				      u32 idx)
>  {
> -	if (!test_bit(idx, &mapping->menu_mask))
> +	if (idx >= BITS_PER_LONG || !test_bit(idx, &mapping->menu_mask))
>  		return -EINVAL;
>  
>  	if (mapping->menu_mapping)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[Dan Carpenter]

On Mon, Sep 28, 2026 at 11:31:05PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 18, 2026 at 03:20:25PM +0300, Dan Carpenter wrote:
> > The "idx" value is a user controlled u32 so we have to bounds check it
> > before calling test_bit() to avoid reading beyond the end of the bitmap.
> > 
> > Fixes: 4e15c535659b ("media: uvcvideo: Support any size for mapping get/set")
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> What's the status of this patch ? The issue is a false positive, do you
> still think there's value in this redundant test ?
> 

No, it's a false positive.

With how Smatch works, I don't have an easy way to silence it in Smatch,
but I'm going to publish my AI skills file for reviewing static checker
warnings.  Also I've started writing a new tool to help filter false
positives.

regards,
dan carpenter

Re: [PATCH] media: uvcvideo: Fix buffer overflow in uvc_mapping_get_menu_value()

[Ricardo Ribalda]

Hi all



On Tue, 29 Sept 2026 at 07:30, Dan Carpenter <error27@gmail.com> wrote:
>
> On Mon, Sep 28, 2026 at 11:31:05PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 18, 2026 at 03:20:25PM +0300, Dan Carpenter wrote:
> > > The "idx" value is a user controlled u32 so we have to bounds check it
> > > before calling test_bit() to avoid reading beyond the end of the bitmap.
> > >
> > > Fixes: 4e15c535659b ("media: uvcvideo: Support any size for mapping get/set")
> > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> >
> > What's the status of this patch ? The issue is a false positive, do you
> > still think there's value in this redundant test ?
> >
>
> No, it's a false positive.
>
> With how Smatch works, I don't have an easy way to silence it in Smatch,
> but I'm going to publish my AI skills file for reviewing static checker
> warnings.  Also I've started writing a new tool to help filter false
> positives.

Even if it is a false positive, I think it is a good addition, it
makes the code more robust.

Do you mind if I make a respin of it with my comments (keeping you as author)?
(Or if you want to send a v2 that would be awesome as well :) )

Thanks!

>
> regards,
> dan carpenter
>


-- 
Ricardo Ribalda