[v2] media: uvcvideo: Fix OOB read

  If the index provided by the user is bigger than the mask size, we might do an
out of bound read.

CC: stable@kernel.org
Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
Reported-by: Zubin Mithra <zsm@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Avoid reading index >= 31
---
Changes in v2:
- Use BITS_PER_TYPE instead of 32 (thanks Sergey).
- Add Reported-by tag.
- Link to v1: https://lore.kernel.org/r/20230717-uvc-oob-v1-1-f5b9b4aba3b4@chromium.org
---
 drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
 1 file changed, 3 insertions(+)


---
base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
change-id: 20230717-uvc-oob-4b0148a00417

Best regards,
  

On (23/07/20 17:46), Ricardo Ribalda wrote:
> 
> If the index provided by the user is bigger than the mask size, we might do an
> out of bound read.
> 
> CC: stable@kernel.org
> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> Reported-by: Zubin Mithra <zsm@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
  

Hi Ricardo,

Thank you for the patch.

On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
> If the index provided by the user is bigger than the mask size, we might do an
> out of bound read.
> 
> CC: stable@kernel.org
> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> Reported-by: Zubin Mithra <zsm@chromium.org>

checkpatch now requests a Reported-by tag to be immediately followed by
a Closes tag that contains the URL to the report. Could you please
provide that ?

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Avoid reading index >= 31
> ---
> Changes in v2:
> - Use BITS_PER_TYPE instead of 32 (thanks Sergey).
> - Add Reported-by tag.
> - Link to v1: https://lore.kernel.org/r/20230717-uvc-oob-v1-1-f5b9b4aba3b4@chromium.org
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 5e9d3da862dd..e59a463c2761 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1402,6 +1402,9 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
>  	query_menu->id = id;
>  	query_menu->index = index;
>  
> +	if (index >= BITS_PER_TYPE(mapping->menu_mask))
> +		return -EINVAL;
> +

I'd move this a few lines up, before setting query_menu.

With those minor changes,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

There's no need for a v3, I can handle the changes locally, but I need
the URL for the Closes tag.

>  	ret = mutex_lock_interruptible(&chain->ctrl_mutex);
>  	if (ret < 0)
>  		return -ERESTARTSYS;
> 
> ---
> base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
> change-id: 20230717-uvc-oob-4b0148a00417
  

Hi Laurent

Thanks for the review!

On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
> > If the index provided by the user is bigger than the mask size, we might do an
> > out of bound read.
> >
> > CC: stable@kernel.org
> > Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> > Reported-by: Zubin Mithra <zsm@chromium.org>
>
> checkpatch now requests a Reported-by tag to be immediately followed by
> a Closes tag that contains the URL to the report. Could you please
> provide that ?
>
I saw that, but the URL is kind of private:

Closes: http://issuetracker.google.com/issues/289975230

> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > Avoid reading index >= 31
> > ---
> > Changes in v2:
> > - Use BITS_PER_TYPE instead of 32 (thanks Sergey).
> > - Add Reported-by tag.
> > - Link to v1: https://lore.kernel.org/r/20230717-uvc-oob-v1-1-f5b9b4aba3b4@chromium.org
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 5e9d3da862dd..e59a463c2761 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1402,6 +1402,9 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> >       query_menu->id = id;
> >       query_menu->index = index;
> >
> > +     if (index >= BITS_PER_TYPE(mapping->menu_mask))
> > +             return -EINVAL;
> > +
>
> I'd move this a few lines up, before setting query_menu.
>

SGTM, I just wanted to clear all the fields to mimic the other error
paths of the function.

> With those minor changes,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> There's no need for a v3, I can handle the changes locally, but I need
> the URL for the Closes tag.
>
> >       ret = mutex_lock_interruptible(&chain->ctrl_mutex);
> >       if (ret < 0)
> >               return -ERESTARTSYS;
> >
> > ---
> > base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
> > change-id: 20230717-uvc-oob-4b0148a00417
>
> --
> Regards,
>
> Laurent Pinchart
  

Hi Ricardo,

(CC'ing Kai and Thorsten who have added the check to checkpatch)

On Wed, Jul 26, 2023 at 08:24:50AM +0200, Ricardo Ribalda wrote:
> On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart wrote:
> > On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
> > > If the index provided by the user is bigger than the mask size, we might do an
> > > out of bound read.
> > >
> > > CC: stable@kernel.org
> > > Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> > > Reported-by: Zubin Mithra <zsm@chromium.org>
> >
> > checkpatch now requests a Reported-by tag to be immediately followed by
> > a Closes tag that contains the URL to the report. Could you please
> > provide that ?
>
> I saw that, but the URL is kind of private:
> 
> Closes: http://issuetracker.google.com/issues/289975230

Ah :-S I wonder if we should drop the Reported-by tag then ?

> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > Avoid reading index >= 31
> > > ---
> > > Changes in v2:
> > > - Use BITS_PER_TYPE instead of 32 (thanks Sergey).
> > > - Add Reported-by tag.
> > > - Link to v1: https://lore.kernel.org/r/20230717-uvc-oob-v1-1-f5b9b4aba3b4@chromium.org
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 5e9d3da862dd..e59a463c2761 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1402,6 +1402,9 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > >       query_menu->id = id;
> > >       query_menu->index = index;
> > >
> > > +     if (index >= BITS_PER_TYPE(mapping->menu_mask))
> > > +             return -EINVAL;
> > > +
> >
> > I'd move this a few lines up, before setting query_menu.
> 
> SGTM, I just wanted to clear all the fields to mimic the other error
> paths of the function.

I'm fine with that too if you prefer.

> > With those minor changes,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > There's no need for a v3, I can handle the changes locally, but I need
> > the URL for the Closes tag.
> >
> > >       ret = mutex_lock_interruptible(&chain->ctrl_mutex);
> > >       if (ret < 0)
> > >               return -ERESTARTSYS;
> > >
> > > ---
> > > base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
> > > change-id: 20230717-uvc-oob-4b0148a00417
  

On 26.07.23 10:07, Laurent Pinchart wrote:
> (CC'ing Kai and Thorsten who have added the check to checkpatch)
> 
> On Wed, Jul 26, 2023 at 08:24:50AM +0200, Ricardo Ribalda wrote:
>> On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart wrote:
>>> On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
>>>> If the index provided by the user is bigger than the mask size, we might do an
>>>> out of bound read.
>>>>
>>>> CC: stable@kernel.org
>>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
>>>> Reported-by: Zubin Mithra <zsm@chromium.org>
>>>
>>> checkpatch now requests a Reported-by tag to be immediately followed by
>>> a Closes

Not that it matters, the changes I performed only required a Link: tag,
which is how things should have been done for many years already. It
later became Closes: due to patches from Matthieu. But whatever. :-D

>>> tag that contains the URL to the report. Could you please
>>> provide that ?
>> I saw that, but the URL is kind of private:
>> Closes: http://issuetracker.google.com/issues/289975230
> Ah :-S I wonder if we should drop the Reported-by tag then ?

That's what I do, unless the reporter granted his permission. To quote
Documentation/process/5.Posting.rst : ```Be careful in the addition of
tags to your patches, as only Cc: is appropriate for addition without
the explicit permission of the person named; using Reported-by: is fine
most of the time as well, but ask for permission if the bug was reported
in private.```

I heard of on instance where a GDPR complaint was filed due to a
Reported-by: tag. So maybe that part should be even revisited reg. the
Cc: aspect. :-/

Ciao, Thorsten
  

Hi Laurent

On Wed, 26 Jul 2023 at 10:07, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> (CC'ing Kai and Thorsten who have added the check to checkpatch)
>
> On Wed, Jul 26, 2023 at 08:24:50AM +0200, Ricardo Ribalda wrote:
> > On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart wrote:
> > > On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
> > > > If the index provided by the user is bigger than the mask size, we might do an
> > > > out of bound read.
> > > >
> > > > CC: stable@kernel.org
> > > > Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> > > > Reported-by: Zubin Mithra <zsm@chromium.org>
> > >
> > > checkpatch now requests a Reported-by tag to be immediately followed by
> > > a Closes tag that contains the URL to the report. Could you please
> > > provide that ?
> >
> > I saw that, but the URL is kind of private:
> >
> > Closes: http://issuetracker.google.com/issues/289975230
>
> Ah :-S I wonder if we should drop the Reported-by tag then ?
>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > > Avoid reading index >= 31
> > > > ---
> > > > Changes in v2:
> > > > - Use BITS_PER_TYPE instead of 32 (thanks Sergey).
> > > > - Add Reported-by tag.
> > > > - Link to v1: https://lore.kernel.org/r/20230717-uvc-oob-v1-1-f5b9b4aba3b4@chromium.org
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 5e9d3da862dd..e59a463c2761 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -1402,6 +1402,9 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > > >       query_menu->id = id;
> > > >       query_menu->index = index;
> > > >
> > > > +     if (index >= BITS_PER_TYPE(mapping->menu_mask))
> > > > +             return -EINVAL;
> > > > +
> > >
> > > I'd move this a few lines up, before setting query_menu.
> >
> > SGTM, I just wanted to clear all the fields to mimic the other error
> > paths of the function.
>
> I'm fine with that too if you prefer.

Your call. I prefer my version, but I am of course biased :P

>
> > > With those minor changes,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > There's no need for a v3, I can handle the changes locally, but I need
> > > the URL for the Closes tag.
> > >
> > > >       ret = mutex_lock_interruptible(&chain->ctrl_mutex);
> > > >       if (ret < 0)
> > > >               return -ERESTARTSYS;
> > > >
> > > > ---
> > > > base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
> > > > change-id: 20230717-uvc-oob-4b0148a00417
>
> --
> Regards,
>
> Laurent Pinchart
  

Hi Thorsten

On Wed, 26 Jul 2023 at 10:33, Thorsten Leemhuis <linux@leemhuis.info> wrote:
>
> On 26.07.23 10:07, Laurent Pinchart wrote:
> > (CC'ing Kai and Thorsten who have added the check to checkpatch)
> >
> > On Wed, Jul 26, 2023 at 08:24:50AM +0200, Ricardo Ribalda wrote:
> >> On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart wrote:
> >>> On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
> >>>> If the index provided by the user is bigger than the mask size, we might do an
> >>>> out of bound read.
> >>>>
> >>>> CC: stable@kernel.org
> >>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> >>>> Reported-by: Zubin Mithra <zsm@chromium.org>
> >>>
> >>> checkpatch now requests a Reported-by tag to be immediately followed by
> >>> a Closes
>
> Not that it matters, the changes I performed only required a Link: tag,
> which is how things should have been done for many years already. It
> later became Closes: due to patches from Matthieu. But whatever. :-D
>

I prefer to leave the Reported-by and remove the Closes, that way we
credit the reporter (assuming they approved to be referred).

But if that is not possible, just remove the reported-by. A private
link is pretty much noise on the tree.

Thanks!

> >>> tag that contains the URL to the report. Could you please
> >>> provide that ?
> >> I saw that, but the URL is kind of private:
> >> Closes: http://issuetracker.google.com/issues/289975230
> > Ah :-S I wonder if we should drop the Reported-by tag then ?
>
> That's what I do, unless the reporter granted his permission. To quote
> Documentation/process/5.Posting.rst : ```Be careful in the addition of
> tags to your patches, as only Cc: is appropriate for addition without
> the explicit permission of the person named; using Reported-by: is fine
> most of the time as well, but ask for permission if the bug was reported
> in private.```
>
> I heard of on instance where a GDPR complaint was filed due to a
> Reported-by: tag. So maybe that part should be even revisited reg. the
> Cc: aspect. :-/
>
> Ciao, Thorsten
  

On 26.07.23 10:38, Ricardo Ribalda wrote:
> On Wed, 26 Jul 2023 at 10:33, Thorsten Leemhuis <linux@leemhuis.info> wrote:
>> On 26.07.23 10:07, Laurent Pinchart wrote:
>>> (CC'ing Kai and Thorsten who have added the check to checkpatch)
>>>
>>> On Wed, Jul 26, 2023 at 08:24:50AM +0200, Ricardo Ribalda wrote:
>>>> On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart wrote:
>>>>> On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
>>>>>> If the index provided by the user is bigger than the mask size, we might do an
>>>>>> out of bound read.
>>>>>>
>>>>>> CC: stable@kernel.org
>>>>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
>>>>>> Reported-by: Zubin Mithra <zsm@chromium.org>
>>>>>
>>>>> checkpatch now requests a Reported-by tag to be immediately followed by
>>>>> a Closes
>>
>> Not that it matters, the changes I performed only required a Link: tag,
>> which is how things should have been done for many years already. It
>> later became Closes: due to patches from Matthieu. But whatever. :-D
> 
> I prefer to leave the Reported-by and remove the Closes, that way we
> credit the reporter (assuming they approved to be referred).
> 
> But if that is not possible, just remove the reported-by. A private
> link is pretty much noise on the tree.

Yeah, of course that's the right strategy (Linus made it pretty clear
that he doesn't want any private links) in case the reporter okay with
the Reported-by. Sorry, forgot to cover that case in my reply.

Ciao, Thorsten
  

On Wed, Jul 26, 2023 at 10:47:46AM +0200, Thorsten Leemhuis wrote:
> On 26.07.23 10:38, Ricardo Ribalda wrote:
> > On Wed, 26 Jul 2023 at 10:33, Thorsten Leemhuis <linux@leemhuis.info> wrote:
> >> On 26.07.23 10:07, Laurent Pinchart wrote:
> >>> (CC'ing Kai and Thorsten who have added the check to checkpatch)
> >>>
> >>> On Wed, Jul 26, 2023 at 08:24:50AM +0200, Ricardo Ribalda wrote:
> >>>> On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart wrote:
> >>>>> On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
> >>>>>> If the index provided by the user is bigger than the mask size, we might do an
> >>>>>> out of bound read.
> >>>>>>
> >>>>>> CC: stable@kernel.org
> >>>>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> >>>>>> Reported-by: Zubin Mithra <zsm@chromium.org>
> >>>>>
> >>>>> checkpatch now requests a Reported-by tag to be immediately followed by
> >>>>> a Closes
> >>
> >> Not that it matters, the changes I performed only required a Link: tag,
> >> which is how things should have been done for many years already. It
> >> later became Closes: due to patches from Matthieu. But whatever. :-D
> > 
> > I prefer to leave the Reported-by and remove the Closes, that way we
> > credit the reporter (assuming they approved to be referred).
> > 
> > But if that is not possible, just remove the reported-by. A private
> > link is pretty much noise on the tree.
> 
> Yeah, of course that's the right strategy (Linus made it pretty clear
> that he doesn't want any private links) in case the reporter okay with
> the Reported-by. Sorry, forgot to cover that case in my reply.
> 

I don't have a preference either way. Please feel free to remove the
reported-by tag.

Thanks,
- Zubin

> Ciao, Thorsten
  

On Wed, Jul 26, 2023 at 10:47:46AM +0200, Thorsten Leemhuis wrote:
> On 26.07.23 10:38, Ricardo Ribalda wrote:
> > On Wed, 26 Jul 2023 at 10:33, Thorsten Leemhuis <linux@leemhuis.info> wrote:
> >> On 26.07.23 10:07, Laurent Pinchart wrote:
> >>> (CC'ing Kai and Thorsten who have added the check to checkpatch)
> >>>
> >>> On Wed, Jul 26, 2023 at 08:24:50AM +0200, Ricardo Ribalda wrote:
> >>>> On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart wrote:
> >>>>> On Thu, Jul 20, 2023 at 05:46:54PM +0000, Ricardo Ribalda wrote:
> >>>>>> If the index provided by the user is bigger than the mask size, we might do an
> >>>>>> out of bound read.
> >>>>>>
> >>>>>> CC: stable@kernel.org
> >>>>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> >>>>>> Reported-by: Zubin Mithra <zsm@chromium.org>
> >>>>>
> >>>>> checkpatch now requests a Reported-by tag to be immediately followed by
> >>>>> a Closes
> >>
> >> Not that it matters, the changes I performed only required a Link: tag,
> >> which is how things should have been done for many years already. It
> >> later became Closes: due to patches from Matthieu. But whatever. :-D
> > 
> > I prefer to leave the Reported-by and remove the Closes, that way we
> > credit the reporter (assuming they approved to be referred).
> > 
> > But if that is not possible, just remove the reported-by. A private
> > link is pretty much noise on the tree.
> 
> Yeah, of course that's the right strategy (Linus made it pretty clear
> that he doesn't want any private links) in case the reporter okay with
> the Reported-by. Sorry, forgot to cover that case in my reply.

I'll keep the Reported-by and omit the Link/Closes tags.