[PATCH] gpio: aggregator: fix a potential use-after-free

[PATCH] gpio: aggregator: fix a potential use-after-free

[Bartosz Golaszewski]

On error we free aggr->lookups->dev_id before removing the entry from
the lookup table. If a concurrent thread calls gpiod_find() before we
remove the entry, it could iterate over the list and call
gpiod_match_lookup_table() which unconditionally dereferences dev_id
when calling strcmp(). Reverse the order of cleanup.

Fixes: 86f162e73d2d ("gpio: aggregator: introduce basic configfs interface")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/gpio/gpio-aggregator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 5915209e1e21..b53230065f50 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -979,8 +979,8 @@ static int gpio_aggregator_activate(struct gpio_aggregator *aggr)
 err_unregister_pdev:
 	platform_device_unregister(pdev);
 err_remove_lookup_table:
-	kfree(aggr->lookups->dev_id);
 	gpiod_remove_lookup_table(aggr->lookups);
+	kfree(aggr->lookups->dev_id);
 err_remove_swnode:
 	fwnode_remove_software_node(swnode);
 err_remove_lookups:
-- 
2.47.3

Re: [PATCH] gpio: aggregator: fix a potential use-after-free

[Geert Uytterhoeven]

Hi Bartosz,

On Wed, 20 May 2026 at 10:49, Bartosz Golaszewski
<bartosz.golaszewski@oss.qualcomm.com> wrote:
> On error we free aggr->lookups->dev_id before removing the entry from
> the lookup table. If a concurrent thread calls gpiod_find() before we
> remove the entry, it could iterate over the list and call
> gpiod_match_lookup_table() which unconditionally dereferences dev_id
> when calling strcmp(). Reverse the order of cleanup.
>
> Fixes: 86f162e73d2d ("gpio: aggregator: introduce basic configfs interface")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -979,8 +979,8 @@ static int gpio_aggregator_activate(struct gpio_aggregator *aggr)
>  err_unregister_pdev:
>         platform_device_unregister(pdev);
>  err_remove_lookup_table:
> -       kfree(aggr->lookups->dev_id);
>         gpiod_remove_lookup_table(aggr->lookups);
> +       kfree(aggr->lookups->dev_id);
>  err_remove_swnode:
>         fwnode_remove_software_node(swnode);
>  err_remove_lookups:

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Note that gpio_aggregator_deactivate() does use the correct ordering.
However, there is a second difference: gpio_aggregator_deactivate()
does not have the call to fwnode_remove_software_node().

I am not very familiar with swnodes. The kerneldoc for
platform_device_info says:

 * @swnode: a secondary software node to be attached to the device. The node
 *      will be automatically registered and its lifetime tied to the platform
 *      device if it is not registered yet.

So perhaps the call to platform_device_unregister() takes care of
that?  But then it should not be done again later, if we jumped to
err_unregister_pdev, and not to a later label?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpio: aggregator: fix a potential use-after-free

[Bartosz Golaszewski]

On Wed, May 20, 2026 at 11:15 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Bartosz,
>
> On Wed, 20 May 2026 at 10:49, Bartosz Golaszewski
> <bartosz.golaszewski@oss.qualcomm.com> wrote:
> > On error we free aggr->lookups->dev_id before removing the entry from
> > the lookup table. If a concurrent thread calls gpiod_find() before we
> > remove the entry, it could iterate over the list and call
> > gpiod_match_lookup_table() which unconditionally dereferences dev_id
> > when calling strcmp(). Reverse the order of cleanup.
> >
> > Fixes: 86f162e73d2d ("gpio: aggregator: introduce basic configfs interface")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>
> Thanks for your patch!
>
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -979,8 +979,8 @@ static int gpio_aggregator_activate(struct gpio_aggregator *aggr)
> >  err_unregister_pdev:
> >         platform_device_unregister(pdev);
> >  err_remove_lookup_table:
> > -       kfree(aggr->lookups->dev_id);
> >         gpiod_remove_lookup_table(aggr->lookups);
> > +       kfree(aggr->lookups->dev_id);
> >  err_remove_swnode:
> >         fwnode_remove_software_node(swnode);
> >  err_remove_lookups:
>
> LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Note that gpio_aggregator_deactivate() does use the correct ordering.
> However, there is a second difference: gpio_aggregator_deactivate()
> does not have the call to fwnode_remove_software_node().
>
> I am not very familiar with swnodes. The kerneldoc for
> platform_device_info says:
>
>  * @swnode: a secondary software node to be attached to the device. The node
>  *      will be automatically registered and its lifetime tied to the platform
>  *      device if it is not registered yet.
>
> So perhaps the call to platform_device_unregister() takes care of
> that?  But then it should not be done again later, if we jumped to
> err_unregister_pdev, and not to a later label?
>

No, you have a good point. Other users of this pattern: gpio-sim and
gpio-virtuser do free the software node in their deactivate() path.

I'll send a separate fix.

Bartosz

Re: [PATCH] gpio: aggregator: fix a potential use-after-free

[Bartosz Golaszewski]

On Wed, 20 May 2026 10:49:11 +0200, Bartosz Golaszewski wrote:
> On error we free aggr->lookups->dev_id before removing the entry from
> the lookup table. If a concurrent thread calls gpiod_find() before we
> remove the entry, it could iterate over the list and call
> gpiod_match_lookup_table() which unconditionally dereferences dev_id
> when calling strcmp(). Reverse the order of cleanup.
> 
> 
> [...]

Applied, thanks!

[1/1] gpio: aggregator: fix a potential use-after-free
      https://git.kernel.org/brgl/c/30c073cab97afb31901f94de9605177b6b84367e

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>