Re: [PATCH] mfd: sm501: Fix potential memory leaks during remove

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing unwind on `sm501_init_dev()` failure leads to Use-After-Free.
- [High] `sm501_pci_probe()` completely ignores the return value of `sm501_init_dev()`, leading to a partially initialized device.
- [High] `devm_kzalloc` is called on an uninitialized `platform_device`, causing a guaranteed NULL pointer dereference.
- [High] Dynamically allocated `gpiod_lookup_table` is never unregistered, leading to a Use-After-Free in the global GPIO lookup list.
--

--- Patch [1]: [PATCH] mfd: sm501: Fix potential memory leaks during remove ---
commit 9027636a8f2b645961f4a4f75f1856b59c0a2754
Author: Abdun Nihaal <[email protected]>

mfd: sm501: Fix potential memory leaks during remove

This patch fixes potential memory leaks of the sm501_devdata structure
during device removal. It adds missing kfree() calls in both the PCI and
platform remove functions.

> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index b5bda477ebfc2..77888965d91ed 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -1627,6 +1627,7 @@ static void sm501_pci_remove(struct pci_dev *dev)
>  	release_mem_region(sm->io_res->start, 0x100);
>  
>  	pci_disable_device(dev);
> +	kfree(sm);

[Severity: High]
This isn't a bug introduced by this patch, but does sm501_init_dev() lack
proper cleanup unwinding on failure?

In drivers/mfd/sm501.c:sm501_init_dev(), child devices (USB, UART, GPIO)
and a sysfs file (dev_attr_dbg_regs) are registered. If a subsequent step
like sm501_check_clocks() fails, the function returns an error without
unwinding these registrations.

In sm501_plat_probe(), the error path cleans up by calling kfree(sm).
However, the leaked child devices and sysfs entries remain active in the
system and hold pointers to the now-freed sm structure, potentially causing
a use-after-free.

[Severity: High]
This is a pre-existing issue, but is the return value of sm501_init_dev()
being completely ignored in sm501_pci_probe()?

Looking at sm501_pci_probe(), the return value of sm501_init_dev(sm) is
discarded, and the function unconditionally returns 0. If sm501_init_dev()
fails (for example, if the device clocks are misconfigured), the probe
succeeds but leaves the device in a partially initialized, dangerous state.

[Severity: High]
This isn't a bug introduced by this patch, but does
sm501_register_gpio_i2c_instance() call devm_kzalloc() on an uninitialized
platform_device?

In drivers/mfd/sm501.c:sm501_register_gpio_i2c_instance(), a new
platform_device is created using sm501_create_subdev(). Before the device
is initialized via platform_device_register(), devm_kzalloc(&pdev->dev, ...)
is called.

The devres core attempts to add the allocation to the uninitialized
(zeroed) devres_head list, dereferencing a NULL prev pointer in
list_add_tail() during the device probe.

[Severity: High]
This is a pre-existing issue, but is the dynamically allocated
gpiod_lookup_table left unregistered on removal?

In sm501_register_gpio_i2c_instance(), a gpiod_lookup_table is allocated
with devm_kzalloc() and registered globally via gpiod_add_lookup_table().

When the child device is removed during sm501_remove_sub(), the
devm-managed memory is automatically freed, but gpiod_remove_lookup_table()
is never called. This leaves a dangling pointer in the global
gpio_lookup_list, which could lead to a use-after-free when another driver
requests a GPIO.

>  }
>  
>  static void sm501_plat_remove(struct platform_device *dev)
> @@ -1637,6 +1638,7 @@ static void sm501_plat_remove(struct platform_device *dev)
>  	iounmap(sm->regs);
>  
>  	release_mem_region(sm->io_res->start, 0x100);
> +	kfree(sm);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1