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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAGn2d8MC4JckjW8isvz6q6SMsb9VEAR2QrnNkJHPRuLZtwqqSg@mail.gmail.com>
Date: Tue, 5 Aug 2025 15:13:07 +0300
From: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>
To: Hans de Goede <hansg@...nel.org>
Cc: mchehab@...nel.org, sakari.ailus@...ux.intel.com, andy@...nel.org, 
	gregkh@...uxfoundation.org, 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 Hans,
Thanks for the feedback.

On Tue, Aug 5, 2025 at 12:00 PM Hans de Goede <hansg@...nel.org> wrote:
>
> 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.
great, now i understand.
>
> 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.
will look into that.
Could you please check my other patch series if you have time
https://lore.kernel.org/all/20250712191325.132666-1-abdelrahmanfekry375@gmail.com/
thank you !
>
> 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);
> >  }
> >
>