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

[Dan Carpenter <error27@gmail.com>]

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