linux-kernel - Re: [PATCH] amba: Fix atomicity violation in amba

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] [day] [month] [year] [list]

Message-ID: <CANgpojVTqXc0OurTWS3b9A6naDtN7Js48r8gaoAp1QAoHvm0GA@mail.gmail.com>
Date: Wed, 13 Nov 2024 00:40:01 +0800
From: Qiu-ji Chen <chenqiuji666@...il.com>
To: Suzuki K Poulose <suzuki.poulose@....com>
Cc: linux@...linux.org.uk, rmk+kernel@...linux.org.uk, sumit.garg@...aro.org, 
	gregkh@...uxfoundation.org, andi.shyti@...nel.org, krzk@...nel.org, 
	linux-kernel@...r.kernel.org, baijiaju1990@...il.com, stable@...r.kernel.org
Subject: Re: [PATCH] amba: Fix atomicity violation in amba_match()

Hello,

We have addressed the concurrency issue in the match driver interface
at a higher level, as detailed in the
[https://lore.kernel.org/all/20241112163041.40083-1-chenqiuji666@gmail.com/].
Due to the widespread nature of the issue, it is more appropriate to
resolve it by adding a lock at the higher level.

If this patch is merged with the lock added at the lower level, it
could potentially lead to a deadlock issue. Therefore, I kindly ask
that you do not merge this patch. I sincerely apologize for the
inconvenience caused and thank you for your understanding.

Regards,
Qiu-ji Chen

On Fri, Oct 18, 2024 at 5:39 PM Suzuki K Poulose <suzuki.poulose@....com> wrote:
>
> On 18/10/2024 09:15, Qiu-ji Chen wrote:
> > Atomicity violation occurs during consecutive reads of
> > pcdev->driver_override. Consider a scenario: after pvdev->driver_override
> > passes the if statement, due to possible concurrency,
> > pvdev->driver_override may change. This leads to pvdev->driver_override
> > passing the condition with an old value, but entering the
> > return !strcmp(pcdev->driver_override, drv->name); statement with a new
> > value. This causes the function to return an unexpected result.
> > Since pvdev->driver_override is a string that is modified byte by byte,
> > without considering atomicity, data races may cause a partially modified
> > pvdev->driver_override to enter both the condition and return statements,
> > resulting in an error.
> >
> > To fix this, we suggest protecting all reads of pvdev->driver_override
> > with a lock, and storing the result of the strcmp() function in a new
> > variable retval. This ensures that pvdev->driver_override does not change
> > during the entire operation, allowing the function to return the expected
> > result.
> >
> > This possible bug is found by an experimental static analysis tool
> > developed by our team. This tool analyzes the locking APIs
> > to extract function pairs that can be concurrently executed, and then
> > analyzes the instructions in the paired functions to identify possible
> > concurrency bugs including data races and atomicity violations.
> >
> > Fixes: 5150a8f07f6c ("amba: reorder functions")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Qiu-ji Chen <chenqiuji666@...il.com>
> > ---
> >   drivers/amba/bus.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 34bc880ca20b..e310f4f83b27 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -209,6 +209,7 @@ static int amba_match(struct device *dev, const struct device_driver *drv)
> >   {
> >       struct amba_device *pcdev = to_amba_device(dev);
> >       const struct amba_driver *pcdrv = to_amba_driver(drv);
> > +     int retval;
> >
> >       mutex_lock(&pcdev->periphid_lock);
> >       if (!pcdev->periphid) {
> > @@ -230,8 +231,14 @@ static int amba_match(struct device *dev, const struct device_driver *drv)
> >       mutex_unlock(&pcdev->periphid_lock);
> >
> >       /* When driver_override is set, only bind to the matching driver */
> > -     if (pcdev->driver_override)
> > -             return !strcmp(pcdev->driver_override, drv->name);
> > +
> > +     device_lock(dev);
> > +     if (pcdev->driver_override) {
> > +             retval = !strcmp(pcdev->driver_override, drv->name);
> > +             device_unlock(dev);
> > +             return retval;
> > +     }
> > +     device_unlock(dev);
> >
> >       return amba_lookup(pcdrv->id_table, pcdev) != NULL;
> >   }
>
>
> Looks correct to me
>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@....com>
>
>