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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >  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