linux-kernel - Re: [PATCH v6 5/5] media: uvcvideo: Do not turn on the camera for some ioctls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64c00146-e6d2-448d-a416-19d5ae7ae3f6@jjverkuil.nl>
Date: Fri, 9 May 2025 15:44:36 +0200
From: Hans Verkuil <hans@...erkuil.nl>
To: Ricardo Ribalda <ribalda@...omium.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Hans de Goede <hdegoede@...hat.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 Mauro Carvalho Chehab <mchehab+samsung@...nel.org>
Subject: Re: [PATCH v6 5/5] media: uvcvideo: Do not turn on the camera for
 some ioctls

On 27/03/2025 22:05, Ricardo Ribalda wrote:
> There are some ioctls that do not need to turn on the camera. Do not
> call uvc_pm_get in those cases.
> 
> Reviewed-by: Hans de Goede <hdegoede@...hat.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 0f1ed0387b2611c8d21e211afe21a35101071d93..668a4e9d772c6d91f045ca75e2744b3a6c69da6b 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1440,6 +1440,26 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file,
>  	struct uvc_fh *handle = file->private_data;
>  	int ret;
>  
> +	/* The following IOCTLs do not need to turn on the camera. */
> +	switch (cmd) {
> +	case VIDIOC_CREATE_BUFS:
> +	case VIDIOC_DQBUF:
> +	case VIDIOC_ENUM_FMT:
> +	case VIDIOC_ENUM_FRAMEINTERVALS:
> +	case VIDIOC_ENUM_FRAMESIZES:
> +	case VIDIOC_ENUMINPUT:
> +	case VIDIOC_EXPBUF:
> +	case VIDIOC_G_FMT:
> +	case VIDIOC_G_PARM:
> +	case VIDIOC_G_SELECTION:
> +	case VIDIOC_QBUF:
> +	case VIDIOC_QUERYCAP:
> +	case VIDIOC_REQBUFS:
> +	case VIDIOC_SUBSCRIBE_EVENT:
> +	case VIDIOC_UNSUBSCRIBE_EVENT:

Wouldn't it be better to check against the ioctls that DO need to turn on the camera?

That is more future proof IMHO.

If a new ioctl is created, and uvc implements it and that needs to turn on the camera,
then presumably you will realize that when you add that ioctl in uvc.

If a new ioctl is created and uvc does not need to turn on the camera, then you will
almost certainly forget to add it to this list.

I'm not blocking this patch, but I think it will be hard to keep this list up to date.
Inverting the test is probably much easier to handle in the future.

Apologies if this has been discussed before, if so, just point to that discussion so I
can read through it.

Regards,

	Hans

> +		return video_ioctl2(file, cmd, arg);
> +	}
> +
>  	ret = uvc_pm_get(handle->stream->dev);
>  	if (ret)
>  		return ret;
>