| Message ID | 20260520084911.27938-1-bartosz.golaszewski@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: aggregator: fix a potential use-after-free | expand |
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
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
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,
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:
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(-)