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;
Date:   Wed, 26 Jul 2023 00:34:51 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Ricardo Ribalda <ribalda@chromium.org>
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>
Subject: Re: [PATCH v2] media: uvcvideo: Fix OOB read
Message-ID: <20230725213451.GU31069@pendragon.ideasonboard.com>
References: <20230717-uvc-oob-v2-1-c7745a8d5847@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230717-uvc-oob-v2-1-c7745a8d5847@chromium.org>
Precedence: bulk

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

-- 
Regards,

Laurent Pinchart