Re: [PATCH] mfd: sm501: Fix potential memory leak in sm501_register_gpio_i2c_instance()

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

New issues:
- [Medium] The patch attempts to fix a memory leak by calling `platform_device_put(pdev)`, but `pdev->dev` is uninitialized, so this call triggers kernel warnings and fails to free the memory.

Pre-existing issues:
- [High] Calling `devm_kzalloc()` on an uninitialized device causes a NULL pointer dereference due to an uninitialized `devres_head` list.
- [High] Child device registration failures are ignored and do not unwind previously registered children, violating MFD subsystem guidelines.
- [High] The gpiod_lookup_table is leaked into a global list on device teardown or registration failure, leading to a system-wide use-after-free.
--

--- Patch [1]: [PATCH] mfd: sm501: Fix potential memory leak in sm501_register_gpio_i2c_instance() ---
commit f652b0e65b80462f6ec3c55b8327bfdfcd49d805
Author: Abdun Nihaal <[email protected]>

mfd: sm501: Fix potential memory leak in sm501_register_gpio_i2c_instance()

The memory allocated for pdev in sm501_create_subdev() is not freed in
an error path in sm501_register_gpio_i2c_instance(). Fix that by
releasing the platform dev in the error path.

> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index b5bda477ebfc2..49c6b21a44e8e 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -1103,8 +1103,10 @@ static int sm501_register_gpio_i2c_instance(struct sm501_devdata *sm,
>  	/* Create a gpiod lookup using gpiochip-local offsets */
>  	lookup = devm_kzalloc(&pdev->dev, struct_size(lookup, table, 3),
>  			      GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but does calling devm_kzalloc()
here cause a NULL pointer dereference?

In sm501_create_subdev(), the pdev is allocated using kzalloc():

sm501_create_subdev() {
    ...
    smdev = kzalloc(sizeof(struct sm501_device) + ...
    ...
}

However, device_initialize() is never called before the device is returned.
This means pdev->dev.devres_head is zero-initialized. When devm_kzalloc()
calls devres_add(), it uses list_add_tail() which will unconditionally
dereference the NULL prev pointer.

> -	if (!lookup)
> +	if (!lookup) {
> +		platform_device_put(pdev);

[Severity: Medium]
Since pdev was only allocated with kzalloc() and lacks a device_initialize()
call, will platform_device_put() fail to actually free the memory?

Because the kobject state is uninitialized and the refcount is zero,
kobject_put() will emit a warning, and kref_put() operating on a zero
refcount prevents the release function from executing. Would it be safer
to use kfree() directly on pdev here, or properly initialize the device
in sm501_create_subdev() first?

>  		return -ENOMEM;
> +	}
>  
>  	lookup->dev_id = "i2c-gpio";
>  	lookup->table[0] = (struct gpiod_lookup)

[ ... ]

>  	lookup->table[1] = (struct gpiod_lookup)
>  		GPIO_LOOKUP_IDX(iic->pin_scl < 32 ? "SM501-LOW" : "SM501-HIGH",
>  				iic->pin_scl % 32, NULL, 1,
>  				GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN);
>  	gpiod_add_lookup_table(lookup);

[Severity: High]
This is a pre-existing issue, but does adding this lookup table to the global
gpio_lookup_list create a system-wide use-after-free risk?

When the device is unregistered during driver unbind (e.g., in
sm501_dev_remove()), devres will automatically free the lookup table memory.
However, since gpiod_remove_lookup_table() is never called in the teardown
path, the freed memory remains linked in the global list, which could
corrupt traversals during subsequent device probes or module removals.

>  
>  	icd = dev_get_platdata(&pdev->dev);

[ ... ]

>  	pdev->id = iic->bus_num;
>  
>  	dev_info(sm->dev, "registering i2c-%d: sda=%d, scl=%d\n",
>  		 iic->bus_num,
>  		 iic->pin_sda, iic->pin_scl);
>  
>  	return sm501_register_device(sm, pdev);
>  }

[Severity: High]
This is a pre-existing issue, but does the error handling in sm501_init_dev()
properly unwind child devices when registration fails?

Currently, child registration failures are ignored:

sm501_init_dev() {
    ...
    sm501_register_gpio_i2c(sm, pdata);
    
    ret = sm501_check_clocks(sm);
    if (ret) {
        ...
        return ret;
    }
}
    
If a failure occurs during sm501_check_clocks(), the parent probe will
exit with an error and free the parent sm501_devdata context. Since the
previously registered children are never unregistered, will this leave
them active with dangling pointers to the freed parent context?

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