Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230717-uvc-oob-v2-1-c7745a8d5847@chromium.org>
 <20230725213451.GU31069@pendragon.ideasonboard.com> <CANiDSCttkqows7PZS823Jpk-CqK9Gz2rujF_R4SPDi=wcPJ2LA@mail.gmail.com>
 <20230726080753.GX31069@pendragon.ideasonboard.com>
In-Reply-To: <20230726080753.GX31069@pendragon.ideasonboard.com>
From:   Ricardo Ribalda <ribalda@chromium.org>
Date:   Wed, 26 Jul 2023 10:37:17 +0200
Message-ID: <CANiDSCsfmpi3N8hqC7XxYMg_6Zrf=QROjj-JwgzavPBzhW0NPw@mail.gmail.com>
Subject: Re: [PATCH v2] media: uvcvideo: Fix OOB read
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        stable@kernel.org, Zubin Mithra <zsm@chromium.org>,
        =?UTF-8?Q?Kai_Wasserb=C3=A4ch?= <kai@dev.carbon-project.org>,
        Thorsten Leemhuis <linux@leemhuis.info>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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


-- 
Ricardo Ribalda