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

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

Date: Wed, 31 Jan 2024 21:01:25 +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 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.

Yours,
Linus Walleij