[PATCH] reset: gpio: suppress bind attributes in sysfs

[PATCH] reset: gpio: suppress bind attributes in sysfs

[Bartosz Golaszewski]

This is a special device that's created dynamically and is supposed to
stay in memory forever. We also currently don't have a devlink between
it and the actual reset consumer. Suppress sysfs bind attributes so that
user-space can't unbind the device because - as of now - it will cause a
use-after-free splat from any user that puts the reset control handle.

Fixes: cee544a40e44 ("reset: gpio: Add GPIO-based reset controller")
Cc: stable@vger.kernel.org
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/reset/reset-gpio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
index e5512b3b596b..626c4c639c15 100644
--- a/drivers/reset/reset-gpio.c
+++ b/drivers/reset/reset-gpio.c
@@ -111,6 +111,7 @@ static struct auxiliary_driver reset_gpio_driver = {
 	.id_table	= reset_gpio_ids,
 	.driver	= {
 		.name = "reset-gpio",
+		.suppress_bind_attrs = true,
 	},
 };
 module_auxiliary_driver(reset_gpio_driver);
-- 
2.51.0

Re: [PATCH] reset: gpio: suppress bind attributes in sysfs

[Krzysztof Kozlowski]

On 04/12/2025 10:44, Bartosz Golaszewski wrote:
> This is a special device that's created dynamically and is supposed to
> stay in memory forever. We also currently don't have a devlink between

Not forever. If every consumer is unloaded, this can be unloaded too, no?

> it and the actual reset consumer. Suppress sysfs bind attributes so that

With that reasoning every reset consumer should have suppress binds.
Devlink should be created by reset controller framework so it is not
this driver's fault.

git grep suppress_bind_attrs -- drivers/reset/

gives only few of such controllers.


> user-space can't unbind the device because - as of now - it will cause a
> use-after-free splat from any user that puts the reset control handle.
> 
> Fixes: cee544a40e44 ("reset: gpio: Add GPIO-based reset controller")

Nothing to be fixed here, unless you claim that every reset provider is
broken as well? What is exactly different in handling devlinks between
this driver and every other reset provider?

> Cc: stable@vger.kernel.org
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---


Best regards,
Krzysztof

Re: [PATCH] reset: gpio: suppress bind attributes in sysfs

[Bartosz Golaszewski]

On Thu, Dec 4, 2025 at 12:14 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 04/12/2025 10:44, Bartosz Golaszewski wrote:
> > This is a special device that's created dynamically and is supposed to
> > stay in memory forever. We also currently don't have a devlink between
>
> Not forever. If every consumer is unloaded, this can be unloaded too, no?
>
> > it and the actual reset consumer. Suppress sysfs bind attributes so that
>
> With that reasoning every reset consumer should have suppress binds.
> Devlink should be created by reset controller framework so it is not
> this driver's fault.
>

Here's my reasoning: I will add a devlink but Phillipp requested some
changes so I still need to resend it. It will be a bigger change than
this one-liner. The reset-gpio device was also converted to auxiliary
bus for v6.19 and I will also convert reset core to using fwnodes for
v6.20 so we'll significantly diverge in stable branches, while this
issue is present ever since the reset-gpio driver exists. It's not the
driver's fault but it's easier to fix it here and it very much is a
special case - it's a software based device rammed in between two
firmware-described devices.

>
> > user-space can't unbind the device because - as of now - it will cause a
> > use-after-free splat from any user that puts the reset control handle.
> >
> > Fixes: cee544a40e44 ("reset: gpio: Add GPIO-based reset controller")
>
> Nothing to be fixed here, unless you claim that every reset provider is
> broken as well? What is exactly different in handling devlinks between
> this driver and every other reset provider?
>

I don't care if we keep the tag, it's just that this commit introduced
a way for user-space to crash the system by simply unbinding
reset-gpio and then its active consumers.

And the difference here is that there is no devlink between reset-gpio
and its consumers. We need to first agree how to add it.

Bartosz

Re: [PATCH] reset: gpio: suppress bind attributes in sysfs

[Krzysztof Kozlowski]

On 04/12/2025 12:22, Bartosz Golaszewski wrote:
> On Thu, Dec 4, 2025 at 12:14 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 04/12/2025 10:44, Bartosz Golaszewski wrote:
>>> This is a special device that's created dynamically and is supposed to
>>> stay in memory forever. We also currently don't have a devlink between
>>
>> Not forever. If every consumer is unloaded, this can be unloaded too, no?
>>
>>> it and the actual reset consumer. Suppress sysfs bind attributes so that
>>
>> With that reasoning every reset consumer should have suppress binds.
>> Devlink should be created by reset controller framework so it is not
>> this driver's fault.
>>
> 
> Here's my reasoning: I will add a devlink but Phillipp requested some
> changes so I still need to resend it. It will be a bigger change than
> this one-liner. The reset-gpio device was also converted to auxiliary
> bus for v6.19 and I will also convert reset core to using fwnodes for
> v6.20 so we'll significantly diverge in stable branches, while this
> issue is present ever since the reset-gpio driver exists. It's not the
> driver's fault but it's easier to fix it here and it very much is a
> special case - it's a software based device rammed in between two
> firmware-described devices.

That's not the answer to my question. You can unbind every other reset
controller. Why is this special although maybe you mentioned below?

> 
>>
>>> user-space can't unbind the device because - as of now - it will cause a
>>> use-after-free splat from any user that puts the reset control handle.
>>>
>>> Fixes: cee544a40e44 ("reset: gpio: Add GPIO-based reset controller")
>>
>> Nothing to be fixed here, unless you claim that every reset provider is
>> broken as well? What is exactly different in handling devlinks between
>> this driver and every other reset provider?
>>
> 
> I don't care if we keep the tag, it's just that this commit introduced
> a way for user-space to crash the system by simply unbinding
> reset-gpio and then its active consumers.
> 
> And the difference here is that there is no devlink between reset-gpio
> and its consumers. We need to first agree how to add it.

So you mean that between every other reset consumer and reset provider
there is a devlink? And here there is no devlink?


Best regards,
Krzysztof

Re: [PATCH] reset: gpio: suppress bind attributes in sysfs

[Bartosz Golaszewski]

On Thu, Dec 4, 2025 at 12:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 04/12/2025 12:22, Bartosz Golaszewski wrote:
> > On Thu, Dec 4, 2025 at 12:14 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 04/12/2025 10:44, Bartosz Golaszewski wrote:
> >>> This is a special device that's created dynamically and is supposed to
> >>> stay in memory forever. We also currently don't have a devlink between
> >>
> >> Not forever. If every consumer is unloaded, this can be unloaded too, no?
> >>
> >>> it and the actual reset consumer. Suppress sysfs bind attributes so that
> >>
> >> With that reasoning every reset consumer should have suppress binds.
> >> Devlink should be created by reset controller framework so it is not
> >> this driver's fault.
> >>
> >
> > Here's my reasoning: I will add a devlink but Phillipp requested some
> > changes so I still need to resend it. It will be a bigger change than
> > this one-liner. The reset-gpio device was also converted to auxiliary
> > bus for v6.19 and I will also convert reset core to using fwnodes for
> > v6.20 so we'll significantly diverge in stable branches, while this
> > issue is present ever since the reset-gpio driver exists. It's not the
> > driver's fault but it's easier to fix it here and it very much is a
> > special case - it's a software based device rammed in between two
> > firmware-described devices.
>
> That's not the answer to my question. You can unbind every other reset
> controller. Why is this special although maybe you mentioned below?
>

Well, for one: when you unbind the device, it's never removed from the
reset-gpio lookup list, the next consumer will never be able to get
its reset control again. I recall seeing either a comment, an email or
a commit message saying that this device stays in memory forever so my
point stands: there's no reason to allow unbinding it. The kernel will
never do it, nor should user-space.

> > I don't care if we keep the tag, it's just that this commit introduced
> > a way for user-space to crash the system by simply unbinding
> > reset-gpio and then its active consumers.
> >
> > And the difference here is that there is no devlink between reset-gpio
> > and its consumers. We need to first agree how to add it.
>
> So you mean that between every other reset consumer and reset provider
> there is a devlink? And here there is no devlink?
>

Effectively: yes, but only because reset is OF-only (as of commit
8bffbfdc01df ("reset: remove legacy reset lookup code")) so device
links are created from device-tree. If you look at any device using
reset-gpio in sysfs, you'll see a supplier:gpiochipX entry but no
entry for the reset. So the answer is: yes, there's no devlink between
the reset-gpio provider and its consumers unless we create them
explicitly, which we should eventually do but I need to rethink the
patch because we should possibly also remove the devlink between the
gpiochip and the reset consumer as well.

Of course, here we could mention a different story: reset seems like
one of these subsystems suffering from object lifetime issues: if you
remove the supplier and the consumer tries to use the reset, you'll
crash because of the dangling rstc->rcdev pointer. That should be
addressed as well eventually.

Bartosz

Re: [PATCH] reset: gpio: suppress bind attributes in sysfs

[Krzysztof Kozlowski]

On 06/12/2025 09:45, Bartosz Golaszewski wrote:
>>> And the difference here is that there is no devlink between reset-gpio
>>> and its consumers. We need to first agree how to add it.
>>
>> So you mean that between every other reset consumer and reset provider
>> there is a devlink? And here there is no devlink?
>>
> 
> Effectively: yes, but only because reset is OF-only (as of commit
> 8bffbfdc01df ("reset: remove legacy reset lookup code")) so device
> links are created from device-tree. If you look at any device using
> reset-gpio in sysfs, you'll see a supplier:gpiochipX entry but no
> entry for the reset. So the answer is: yes, there's no devlink between
> the reset-gpio provider and its consumers unless we create them
> explicitly, which we should eventually do but I need to rethink the
> patch because we should possibly also remove the devlink between the
> gpiochip and the reset consumer as well.
> 
> Of course, here we could mention a different story: reset seems like
> one of these subsystems suffering from object lifetime issues: if you
> remove the supplier and the consumer tries to use the reset, you'll
> crash because of the dangling rstc->rcdev pointer. That should be
> addressed as well eventually.
> 

Yeah, I get it. We talked in person, so unless you plan to rework the
patch to fix it differently:


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [PATCH] reset: gpio: suppress bind attributes in sysfs

[Bartosz Golaszewski]

On Thu, Dec 4, 2025 at 10:47 AM Bartosz Golaszewski
<bartosz.golaszewski@oss.qualcomm.com> wrote:
>
> This is a special device that's created dynamically and is supposed to
> stay in memory forever. We also currently don't have a devlink between
> it and the actual reset consumer. Suppress sysfs bind attributes so that
> user-space can't unbind the device because - as of now - it will cause a
> use-after-free splat from any user that puts the reset control handle.
>
> Fixes: cee544a40e44 ("reset: gpio: Add GPIO-based reset controller")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/reset/reset-gpio.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
> index e5512b3b596b..626c4c639c15 100644
> --- a/drivers/reset/reset-gpio.c
> +++ b/drivers/reset/reset-gpio.c
> @@ -111,6 +111,7 @@ static struct auxiliary_driver reset_gpio_driver = {
>         .id_table       = reset_gpio_ids,
>         .driver = {
>                 .name = "reset-gpio",
> +               .suppress_bind_attrs = true,
>         },
>  };
>  module_auxiliary_driver(reset_gpio_driver);
> --
> 2.51.0
>
>

Hi Phillipp!

This is now reviewed by Krzysztof, can you pick it up, please?

Bartosz