Re: [PATCH] mfd: sm501: unwind common init failures

[Lee Jones <lee@kernel.org>]

Can you consider these please.

Also, we don't allow forward declarations.

/* Sashiko Automation: Reviewed (2 Findings) */

On Tue, 16 Jun 2026, Pengpeng Hou wrote:

> sm501_init_dev() creates the debug sysfs file and can register child
> devices before the final clock-source sanity check. If that check fails,
> the caller tears down the parent mapping but the children and debug file
> remain published. The PCI probe path also ignored the return value from
> sm501_init_dev().
> 
> Unwind the common SM501 registrations before returning the clock-check
> failure, and make the PCI probe path propagate sm501_init_dev() errors.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  drivers/mfd/sm501.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index 0ee6d8940e69..b21b4fc18e87 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -1249,6 +1249,8 @@ static unsigned int sm501_mem_local[] = {
>  	[5]	= 2*1024*1024,
>  };
>  
> +static void sm501_dev_remove(struct sm501_devdata *sm);
> +
>  /* sm501_init_dev
>   *
>   * Common init code for an SM501
> @@ -1321,6 +1323,7 @@ static int sm501_init_dev(struct sm501_devdata *sm)
>  	if (ret) {
>  		dev_err(sm->dev, "M1X and M clocks sourced from different "
>  					"PLLs\n");
> +		sm501_dev_remove(sm);

[Severity: High]
This isn't a bug introduced by this patch, but can this error path unwinding
lead to a Use-After-Free?

When sm501_dev_remove() unregisters the child platform devices in
sm501_remove_sub(), the devres framework automatically frees the memory
allocated for the GPIO lookup table.

However, since gpiod_remove_lookup_table() is never called to remove it
from the global gpio_lookup_list, the freed memory remains linked. Will any
subsequent GPIO lookup system-wide traverse this corrupted list, resulting
in a Use-After-Free?



[Severity: High]
This is another pre-existing issue, but does calling devm_kzalloc() on an
uninitialized platform device trigger a kernel panic here?

Earlier in initialization, sm501_register_gpio_i2c_instance() allocates the
platform device using kzalloc() and then immediately calls devm_kzalloc()
to allocate the GPIO lookup table:

drivers/mfd/sm501.c:sm501_register_gpio_i2c_instance() {
    ...
    pdev = sm501_create_subdev(sm, "i2c-gpio", 0,
                   sizeof(struct i2c_gpio_platform_data));
    ...
    lookup = devm_kzalloc(&pdev->dev, struct_size(lookup, table, 3),
                  GFP_KERNEL);
    ...
}

Because device_initialize() hasn't been called on pdev yet,
pdev->dev.devres_head is completely uninitialized. When devres attempts to
attach the node, wouldn't it call list_add_tail() on an uninitialized list
head and dereference a NULL pointer?


>  		return -EINVAL;
>  	}
>  
> @@ -1583,9 +1586,14 @@ static int sm501_pci_probe(struct pci_dev *dev,
>  		goto err4;
>  	}
>  
> -	sm501_init_dev(sm);
> +	err = sm501_init_dev(sm);
> +	if (err)
> +		goto err5;
> +
>  	return 0;
>  
> + err5:
> +	iounmap(sm->regs);
>   err4:
>  	release_mem_region(sm->io_res->start, 0x100);
>   err3:
> -- 
> 2.50.1 (Apple Git-155)
> 

-- 
Lee Jones