linux-kernel - Re: [PATCH v3] staging: media: atomisp: add missing mutex lock in atomisp_s_fmt

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <5c9bf027-f362-4176-b4af-bcba9aa96e0c@kernel.org>
Date: Tue, 5 Aug 2025 11:00:08 +0200
From: Hans de Goede <hansg@...nel.org>
To: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>, mchehab@...nel.org,
 sakari.ailus@...ux.intel.com, andy@...nel.org, gregkh@...uxfoundation.org
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-staging@...ts.linux.dev, linux-kernel-mentees@...ts.linux.dev,
 skhan@...uxfoundation.org, dan.carpenter@...aro.org
Subject: Re: [PATCH v3] staging: media: atomisp: add missing mutex lock in
 atomisp_s_fmt_cap

Hi,

On 17-Jul-25 2:42 PM, Abdelrahman Fekry wrote:
> The function atomisp_set_fmt() modifies shared device state and expects
> callers to hold the isp->mutex for synchronization. While most internal
> callers correctly lock the mutex before invoking atomisp_set_fmt(), the
> V4L2 ioctl handler atomisp_s_fmt_cap() does not.
> 
> This results in an unsafe execution path for VIDIOC_S_FMT ioctls
> (e.g. via v4l2-ctl), where shared structures such as pipe->pix and
> pipe->frame_info may be modified concurrently without proper protection.
> 
> - Fix this by explicitly locking isp->mutex in atomisp_s_fmt_cap().

Thank you for your patch, but I'm afraid that this stems from
not understanding how v4l2 drivers work.

isp->mutex is set as the mutex for the /dev/video# node in:
drivers/staging/media/atomisp/pci/atomisp_v4l2.c:

int atomisp_video_init(struct atomisp_video_pipe *video)
{
	...
	video->vdev.lock = &video->isp->mutex;

This guarantees that any ioctl handlers will be called with
isp->mutex already held.

The suggested change here will result in trying to lock
the mutex a second time leading to a deadlock.

So NACK for this patch.

What would be useful is adding:

	lockdep_assert_held(&isp->mutex);

to functions which expect the mutex to be held. I regularly
test the atomisp code with lockdep enabled kernels so this
will help catch any paths where we indeed are not locking
the mutex while we should lock it.

Regards.

Hans






> 
> Fixes: 4bdab80981ca ("media: atomisp: Make it possible to call atomisp_set_fmt() without a file handle")
> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>
> ---
> v3:
> - Use guard(mutex) as suggested by Dan Carpenter and Andy Shevchenko
> - Remove extra blank line after variable declaration
> - Fix include order as requested by Andy Shevchenko
> 
> v2: https://lore.kernel.org/all/20250717013003.20936-1-abdelrahmanfekry375@gmail.com/
> - Add Fixes tag
> - use cleanup.h micros instead of explicitly calling mutex_lock/unlock
> 
> v1: https://lore.kernel.org/all/20250716014225.15279-1-abdelrahmanfekry375@gmail.com/
> ---
>  drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index bb8b2f2213b0..2690c05afff3 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -7,6 +7,7 @@
>   * Copyright (c) 2010 Silicon Hive www.siliconhive.com.
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/pci.h>
>  
> @@ -416,7 +417,9 @@ static int atomisp_s_fmt_cap(struct file *file, void *fh,
>  			     struct v4l2_format *f)
>  {
>  	struct video_device *vdev = video_devdata(file);
> +	struct atomisp_device *isp = video_get_drvdata(vdev);
>  
> +	guard(mutex)(&isp->mutex);
>  	return atomisp_set_fmt(vdev, f);
>  }
>