If the index provided by the user is bigger than the mask size, we might do an
out of bound read.
CC: [email protected]
Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
Reported-by: Zubin Mithra <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
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/[email protected]
---
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;
+
ret = mutex_lock_interruptible(&chain->ctrl_mutex);
if (ret < 0)
return -ERESTARTSYS;
---
base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
change-id: 20230717-uvc-oob-4b0148a00417
Best regards,
--
Ricardo Ribalda <[email protected]>
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: [email protected]
> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> Reported-by: Zubin Mithra <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
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: [email protected]
> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> Reported-by: Zubin Mithra <[email protected]>
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 <[email protected]>
> ---
> 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/[email protected]
> ---
> 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 <[email protected]>
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 Laurent
Thanks for the review!
On Tue, 25 Jul 2023 at 23:34, Laurent Pinchart
<[email protected]> 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: [email protected]
> > Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> > Reported-by: Zubin Mithra <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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/[email protected]
> > ---
> > 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 <[email protected]>
>
> 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
--
Ricardo Ribalda
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: [email protected]
> > > Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> > > Reported-by: Zubin Mithra <[email protected]>
> >
> > 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 <[email protected]>
> > > ---
> > > 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/[email protected]
> > > ---
> > > 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 <[email protected]>
> >
> > 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 <[email protected]> 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: [email protected]
> >>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> >>>> Reported-by: Zubin Mithra <[email protected]>
> >>>
> >>> 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
--
Ricardo Ribalda
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: [email protected]
>>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
>>>> Reported-by: Zubin Mithra <[email protected]>
>>>
>>> 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
On 26.07.23 10:38, Ricardo Ribalda wrote:
> On Wed, 26 Jul 2023 at 10:33, Thorsten Leemhuis <[email protected]> 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: [email protected]
>>>>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
>>>>>> Reported-by: Zubin Mithra <[email protected]>
>>>>>
>>>>> 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
Hi Laurent
On Wed, 26 Jul 2023 at 10:07, Laurent Pinchart
<[email protected]> 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: [email protected]
> > > > Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> > > > Reported-by: Zubin Mithra <[email protected]>
> > >
> > > 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 <[email protected]>
> > > > ---
> > > > 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/[email protected]
> > > > ---
> > > > 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 <[email protected]>
> > >
> > > 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
--
Ricardo Ribalda
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 <[email protected]> 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: [email protected]
> >>>>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> >>>>>> Reported-by: Zubin Mithra <[email protected]>
> >>>>>
> >>>>> 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 <[email protected]> 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: [email protected]
> >>>>>> Fixes: 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU")
> >>>>>> Reported-by: Zubin Mithra <[email protected]>
> >>>>>
> >>>>> 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.
--
Regards,
Laurent Pinchart