| Message ID | 20230717-uvc-oob-v2-1-c7745a8d5847@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: uvcvideo: Fix OOB read | expand |
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.
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;
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,