linux-kernel - Re: [PATCH 10/22] gpio: reinforce desc->flags handling

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]

Date: Wed, 31 Jan 2024 21:35:21 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Kent Gibson <warthog618@...il.com>, Alex Elder <elder@...aro.org>, 
	Geert Uytterhoeven <geert+renesas@...der.be>, "Paul E . McKenney" <paulmck@...nel.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Wolfram Sang <wsa@...-dreams.de>, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 10/22] gpio: reinforce desc->flags handling

On Wed, Jan 31, 2024 at 9:01 PM Linus Walleij <linus.walleij@...aroorg> wrote:
> On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <brgl@...ev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> >
> > We now removed the gpio_lock spinlock and modified the places
> > previously protected by it to handle desc->flags access in a consistent
> > way. Let's improve other places that were previously unprotected by
> > reading the flags field of gpio_desc once and using the stored value for
> > logic consistency. If we need to modify the field, let's also write it
> > back once with a consistent value resulting from the function's logic.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> (...)
>
> I have a trouble with this one:
>
> gpiochip_find_base_unlocked()
> > +       unsigned long flags;
> (...)
> > +       flags = READ_ONCE(desc->flags);
> (...)
> > +       if (test_bit(FLAG_OPEN_DRAIN, &flags) &&
> > +           test_bit(FLAG_IS_OUT, &flags))
> >                 return 0;
> (...)
> > +       assign_bit(FLAG_IS_OUT, &flags, !ret);
> > +       WRITE_ONCE(desc->flags, flags);
>
> I unerstand the atomicity of each operation here, but ... if what you want
> to protect is modifications from other CPUs, how do we know that another
> CPU isn't coming in and reading and modifying and assigning
> another flag inbetween these operations while the value is only
> stored in the CPU-local flags variable?
>
> Same with gpiod_direction_output().
>
> To me it seems like maybe you need to actually protect the desc->flags
> with the SRCU struct in these cases? (and not only use it for the
> label protection then).
>
> An alternative is maybe to rewrite the code with test_and_set().
>
> But as you say it is currently unprotected, I just wonder if this really
> adds any protection.

After re-reading the cover letter I'm fine with this, but I still wonder
if it buys us anything.

Maybe some words looped back from the
commit message that we are not really protecting the callbacks
because access is [predominantly] exclusive?

Reviewed-by: Linus Walleij <linus.walleij@...aro.org>

Yours,
Linus Walleij