gpiolib: Adopt revocable mechanism for UAF prevention

[PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Tzung-Bi Shih 3 weeks, 2 days ago

`kobj->name` should be freed by kfree_const()[1][2].  Correct it.

[1] https://elixir.bootlin.com/linux/v6.18/source/lib/kasprintf.c#L41
[2] https://elixir.bootlin.com/linux/v6.18/source/lib/kobject.c#L695

Cc: stable@vger.kernel.org
Fixes: c351bb64cbe6 ("gpiolib: free device name on error path to fix kmemleak")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5eb918da7ea2..ba9323432e3a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1263,7 +1263,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 err_free_descs:
 	kfree(gdev->descs);
 err_free_dev_name:
-	kfree(dev_name(&gdev->dev));
+	kfree_const(dev_name(&gdev->dev));
 err_free_ida:
 	ida_free(&gpio_ida, gdev->id);
 err_free_gdev:
-- 
2.52.0.457.g6b5491de43-goog

Re: [PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Jason Gunthorpe 3 weeks, 1 day ago

On Fri, Jan 16, 2026 at 08:10:14AM +0000, Tzung-Bi Shih wrote:
> `kobj->name` should be freed by kfree_const()[1][2].  Correct it.
> 
> [1] https://elixir.bootlin.com/linux/v6.18/source/lib/kasprintf.c#L41
> [2] https://elixir.bootlin.com/linux/v6.18/source/lib/kobject.c#L695
> 
> Cc: stable@vger.kernel.org
> Fixes: c351bb64cbe6 ("gpiolib: free device name on error path to fix kmemleak")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>  drivers/gpio/gpiolib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5eb918da7ea2..ba9323432e3a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1263,7 +1263,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  err_free_descs:
>  	kfree(gdev->descs);
>  err_free_dev_name:
> -	kfree(dev_name(&gdev->dev));
> +	kfree_const(dev_name(&gdev->dev));
>  err_free_ida:
>  	ida_free(&gpio_ida, gdev->id);
>  err_free_gdev:
        kfree(gdev);

I don't think users should be open coding this, put_device() frees the
dev_name properly. The issue here is that the code doesn't call
device_initialize() before doing dev_set_name() and then tries to
fiddle a weird teardown sequence when it eventually does get initialized:

err_remove_from_list:
        if (gdev->dev.release) {
                /* release() has been registered by gpiochip_setup_dev() */
                gpio_device_put(gdev);
                goto err_print_message;
        }

If gpiochip_add_data_with_key() is split into two functions, one that
does kzalloc(), some initialization and then ends with
device_initialize(), then a second function that calls the first and
does the rest of the initialization and error unwinds with
put_device() it will work a lot better.

Jason

Re: [PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Bartosz Golaszewski 3 weeks, 1 day ago

On Fri, Jan 16, 2026 at 3:14 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Fri, Jan 16, 2026 at 08:10:14AM +0000, Tzung-Bi Shih wrote:
> > `kobj->name` should be freed by kfree_const()[1][2].  Correct it.
> >
> > [1] https://elixir.bootlin.com/linux/v6.18/source/lib/kasprintf.c#L41
> > [2] https://elixir.bootlin.com/linux/v6.18/source/lib/kobject.c#L695
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c351bb64cbe6 ("gpiolib: free device name on error path to fix kmemleak")
> > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > ---
> >  drivers/gpio/gpiolib.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 5eb918da7ea2..ba9323432e3a 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1263,7 +1263,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >  err_free_descs:
> >       kfree(gdev->descs);
> >  err_free_dev_name:
> > -     kfree(dev_name(&gdev->dev));
> > +     kfree_const(dev_name(&gdev->dev));
> >  err_free_ida:
> >       ida_free(&gpio_ida, gdev->id);
> >  err_free_gdev:
>         kfree(gdev);
>
> I don't think users should be open coding this, put_device() frees the
> dev_name properly. The issue here is that the code doesn't call
> device_initialize() before doing dev_set_name() and then tries to
> fiddle a weird teardown sequence when it eventually does get initialized:
>
> err_remove_from_list:
>         if (gdev->dev.release) {
>                 /* release() has been registered by gpiochip_setup_dev() */
>                 gpio_device_put(gdev);
>                 goto err_print_message;
>         }
>
> If gpiochip_add_data_with_key() is split into two functions, one that
> does kzalloc(), some initialization and then ends with
> device_initialize(), then a second function that calls the first and
> does the rest of the initialization and error unwinds with
> put_device() it will work a lot better.
>

In theory yes but you wouldn't be the first one to attempt to improve
it. This code is very brittle when it comes to GPIO chips that need to
be initialized very early into the boot process. I'm talking old
drivers in arch which call this function without even an associated
parent struct device. When I'm looking at it now, it does seem
possible to call device_initialize() early but whether that will work
correctly for all existing users is a bigger question.

I'm open to trying it after v7.0-rc1 is tagged. This would give it
enough time in linux-next to make sure it works.

Bartosz

Re: [PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Tzung-Bi Shih 2 weeks, 5 days ago

On Fri, Jan 16, 2026 at 03:38:37PM +0100, Bartosz Golaszewski wrote:
> On Fri, Jan 16, 2026 at 3:14 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Fri, Jan 16, 2026 at 08:10:14AM +0000, Tzung-Bi Shih wrote:
> > > `kobj->name` should be freed by kfree_const()[1][2].  Correct it.
> > >
> > > [1] https://elixir.bootlin.com/linux/v6.18/source/lib/kasprintf.c#L41
> > > [2] https://elixir.bootlin.com/linux/v6.18/source/lib/kobject.c#L695
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: c351bb64cbe6 ("gpiolib: free device name on error path to fix kmemleak")
> > > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > > ---
> > >  drivers/gpio/gpiolib.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 5eb918da7ea2..ba9323432e3a 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1263,7 +1263,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> > >  err_free_descs:
> > >       kfree(gdev->descs);
> > >  err_free_dev_name:
> > > -     kfree(dev_name(&gdev->dev));
> > > +     kfree_const(dev_name(&gdev->dev));
> > >  err_free_ida:
> > >       ida_free(&gpio_ida, gdev->id);
> > >  err_free_gdev:
> >         kfree(gdev);
> >
> > I don't think users should be open coding this, put_device() frees the
> > dev_name properly. The issue here is that the code doesn't call
> > device_initialize() before doing dev_set_name() and then tries to
> > fiddle a weird teardown sequence when it eventually does get initialized:
> >
> > err_remove_from_list:
> >         if (gdev->dev.release) {
> >                 /* release() has been registered by gpiochip_setup_dev() */
> >                 gpio_device_put(gdev);
> >                 goto err_print_message;
> >         }
> >
> > If gpiochip_add_data_with_key() is split into two functions, one that
> > does kzalloc(), some initialization and then ends with
> > device_initialize(), then a second function that calls the first and
> > does the rest of the initialization and error unwinds with
> > put_device() it will work a lot better.

That's basically what the aggressive patch 03/23 tries to do without
separating the first half to an indepedent function.

Generally, I think we can try to move device_initialize() earlier in the
function.  On error handling paths, just put_device() for it.  In the
.release() callback, free the resource iff it has initialized.

> In theory yes but you wouldn't be the first one to attempt to improve
> it. This code is very brittle when it comes to GPIO chips that need to
> be initialized very early into the boot process. I'm talking old
> drivers in arch which call this function without even an associated
> parent struct device. When I'm looking at it now, it does seem
> possible to call device_initialize() early but whether that will work
> correctly for all existing users is a bigger question.

FWIW: found a very early stage calling path when I was investigating
`gpiolib_initialized`: start_kernel() -> init_IRQ() -> dove_init_irq() ->
orion_gpio_init() -> gpiochip_add_data() -> gpiochip_add_data_with_key().

Prior to aab5c6f20023 ("gpio: set device type for GPIO chips"),
device_initialize() is also called in gpiochip_add_data_with_key().  It
seems to me it's possible to move it back to gpiochip_add_data_with_key()
as 03/23 does, and move it earlier in the function.

Re: [PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Bartosz Golaszewski 2 weeks, 5 days ago

On Tue, Jan 20, 2026 at 5:30 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> Generally, I think we can try to move device_initialize() earlier in the
> function.  On error handling paths, just put_device() for it.  In the
> .release() callback, free the resource iff it has initialized.
>
> > In theory yes but you wouldn't be the first one to attempt to improve
> > it. This code is very brittle when it comes to GPIO chips that need to
> > be initialized very early into the boot process. I'm talking old
> > drivers in arch which call this function without even an associated
> > parent struct device. When I'm looking at it now, it does seem
> > possible to call device_initialize() early but whether that will work
> > correctly for all existing users is a bigger question.
>
> FWIW: found a very early stage calling path when I was investigating
> `gpiolib_initialized`: start_kernel() -> init_IRQ() -> dove_init_irq() ->
> orion_gpio_init() -> gpiochip_add_data() -> gpiochip_add_data_with_key().
>
> Prior to aab5c6f20023 ("gpio: set device type for GPIO chips"),
> device_initialize() is also called in gpiochip_add_data_with_key().  It
> seems to me it's possible to move it back to gpiochip_add_data_with_key()
> as 03/23 does, and move it earlier in the function.

Sounds good, let's try it next cycle!

Tzung-Bi: please make it a change separate from the wider Revocable
series for GPIO.

Bart

Re: [PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Bartosz Golaszewski 3 weeks, 1 day ago

On Fri, 16 Jan 2026 09:10:14 +0100, Tzung-Bi Shih <tzungbi@kernel.org> said:
> `kobj->name` should be freed by kfree_const()[1][2].  Correct it.
>
> [1] https://elixir.bootlin.com/linux/v6.18/source/lib/kasprintf.c#L41
> [2] https://elixir.bootlin.com/linux/v6.18/source/lib/kobject.c#L695
>

Please don't add links third-party groks to git commit messages.

> Cc: stable@vger.kernel.org
> Fixes: c351bb64cbe6 ("gpiolib: free device name on error path to fix kmemleak")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>  drivers/gpio/gpiolib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5eb918da7ea2..ba9323432e3a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1263,7 +1263,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  err_free_descs:
>  	kfree(gdev->descs);
>  err_free_dev_name:
> -	kfree(dev_name(&gdev->dev));
> +	kfree_const(dev_name(&gdev->dev));
>  err_free_ida:
>  	ida_free(&gpio_ida, gdev->id);
>  err_free_gdev:
> --
> 2.52.0.457.g6b5491de43-goog
>
>

I've never paid attention to this bit but it really looks broken. I understand
that this string won't get freed until we initialize refcounting on the
underlying kobject but reaching two abstraction layers below to get the string
for freeing out of the kobject looks incorrect to me.

It's also one of only two instances of doing kfree(dev_name(dev)), the other
one being in drivers/scsi/hosts.c.

It looks to me that the device name is not really used in
gpiochip_add_data_with_key(). Can we move dev_set_name() after
device_initialize()?

Bart

Re: [PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Tzung-Bi Shih 2 weeks, 5 days ago

On Fri, Jan 16, 2026 at 01:15:17PM +0000, Bartosz Golaszewski wrote:
> On Fri, 16 Jan 2026 09:10:14 +0100, Tzung-Bi Shih <tzungbi@kernel.org> said:
> > `kobj->name` should be freed by kfree_const()[1][2].  Correct it.
> >
> > [1] https://elixir.bootlin.com/linux/v6.18/source/lib/kasprintf.c#L41
> > [2] https://elixir.bootlin.com/linux/v6.18/source/lib/kobject.c#L695
> >
> 
> Please don't add links third-party groks to git commit messages.
> 
> > Cc: stable@vger.kernel.org
> > Fixes: c351bb64cbe6 ("gpiolib: free device name on error path to fix kmemleak")
> > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > ---
> >  drivers/gpio/gpiolib.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 5eb918da7ea2..ba9323432e3a 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1263,7 +1263,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >  err_free_descs:
> >  	kfree(gdev->descs);
> >  err_free_dev_name:
> > -	kfree(dev_name(&gdev->dev));
> > +	kfree_const(dev_name(&gdev->dev));
> >  err_free_ida:
> >  	ida_free(&gpio_ida, gdev->id);
> >  err_free_gdev:
> > --
> > 2.52.0.457.g6b5491de43-goog
> >
> >
> 
> I've never paid attention to this bit but it really looks broken. I understand
> that this string won't get freed until we initialize refcounting on the
> underlying kobject but reaching two abstraction layers below to get the string
> for freeing out of the kobject looks incorrect to me.
> 
> It's also one of only two instances of doing kfree(dev_name(dev)), the other
> one being in drivers/scsi/hosts.c.
> 
> It looks to me that the device name is not really used in

The device name is indeed used in gpiochip_add_data_with_key().  E.g.,
gpiochip_get_ngpios() -> dev_err().

> gpiochip_add_data_with_key(). Can we move dev_set_name() after
> device_initialize()?

Sounds a good idea.  I'll try to approach it in accompany with the
aggressive patch 03/23.

Re: [PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Greg Kroah-Hartman 3 weeks, 1 day ago

On Fri, Jan 16, 2026 at 01:15:17PM +0000, Bartosz Golaszewski wrote:
> On Fri, 16 Jan 2026 09:10:14 +0100, Tzung-Bi Shih <tzungbi@kernel.org> said:
> > `kobj->name` should be freed by kfree_const()[1][2].  Correct it.
> >
> > [1] https://elixir.bootlin.com/linux/v6.18/source/lib/kasprintf.c#L41
> > [2] https://elixir.bootlin.com/linux/v6.18/source/lib/kobject.c#L695
> >
> 
> Please don't add links third-party groks to git commit messages.
> 
> > Cc: stable@vger.kernel.org
> > Fixes: c351bb64cbe6 ("gpiolib: free device name on error path to fix kmemleak")
> > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > ---
> >  drivers/gpio/gpiolib.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 5eb918da7ea2..ba9323432e3a 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1263,7 +1263,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >  err_free_descs:
> >  	kfree(gdev->descs);
> >  err_free_dev_name:
> > -	kfree(dev_name(&gdev->dev));
> > +	kfree_const(dev_name(&gdev->dev));
> >  err_free_ida:
> >  	ida_free(&gpio_ida, gdev->id);
> >  err_free_gdev:
> > --
> > 2.52.0.457.g6b5491de43-goog
> >
> >
> 
> I've never paid attention to this bit but it really looks broken. I understand
> that this string won't get freed until we initialize refcounting on the
> underlying kobject but reaching two abstraction layers below to get the string
> for freeing out of the kobject looks incorrect to me.
> 
> It's also one of only two instances of doing kfree(dev_name(dev)), the other
> one being in drivers/scsi/hosts.c.

That one is wrong, I already rejected it :)

> It looks to me that the device name is not really used in
> gpiochip_add_data_with_key(). Can we move dev_set_name() after
> device_initialize()?

This should be cleaned up automatically by the driver core, no need to
free this on its own.

thanks,

greg k-h

Re: [PATCH 01/23] gpiolib: Correct wrong kfree() usage for `kobj->name`

Posted by Bartosz Golaszewski 3 weeks, 1 day ago

On Fri, Jan 16, 2026 at 2:27 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jan 16, 2026 at 01:15:17PM +0000, Bartosz Golaszewski wrote:
> > On Fri, 16 Jan 2026 09:10:14 +0100, Tzung-Bi Shih <tzungbi@kernel.org> said:
> > > `kobj->name` should be freed by kfree_const()[1][2].  Correct it.
> > >
> > > [1] https://elixir.bootlin.com/linux/v6.18/source/lib/kasprintf.c#L41
> > > [2] https://elixir.bootlin.com/linux/v6.18/source/lib/kobject.c#L695
> > >
> >
> > Please don't add links third-party groks to git commit messages.
> >
> > > Cc: stable@vger.kernel.org
> > > Fixes: c351bb64cbe6 ("gpiolib: free device name on error path to fix kmemleak")
> > > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > > ---
> > >  drivers/gpio/gpiolib.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 5eb918da7ea2..ba9323432e3a 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1263,7 +1263,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> > >  err_free_descs:
> > >     kfree(gdev->descs);
> > >  err_free_dev_name:
> > > -   kfree(dev_name(&gdev->dev));
> > > +   kfree_const(dev_name(&gdev->dev));
> > >  err_free_ida:
> > >     ida_free(&gpio_ida, gdev->id);
> > >  err_free_gdev:
> > > --
> > > 2.52.0.457.g6b5491de43-goog
> > >
> > >
> >
> > I've never paid attention to this bit but it really looks broken. I understand
> > that this string won't get freed until we initialize refcounting on the
> > underlying kobject but reaching two abstraction layers below to get the string
> > for freeing out of the kobject looks incorrect to me.
> >
> > It's also one of only two instances of doing kfree(dev_name(dev)), the other
> > one being in drivers/scsi/hosts.c.
>
> That one is wrong, I already rejected it :)
>
> > It looks to me that the device name is not really used in
> > gpiochip_add_data_with_key(). Can we move dev_set_name() after
> > device_initialize()?
>
> This should be cleaned up automatically by the driver core, no need to
> free this on its own.
>

It will once we initiate kobject reference counting from
device_initialize(). The code here still hasn't called it though. It
doesn't look like it'll be freed to me on errors before
device_initialize(). A later patch in this series seems to try to
address it though so maybe this one's not even needed except for
backporting to stable branches.

Bartosz