Re: [PATCH v13 01/12] spmi: Fix potential use-after-free by grabbing of_node reference

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Improper resource release on error path in `spmi_controller_alloc()` leads to IDA corruption.
--

--- Patch [1]: [PATCH v13 01/12] spmi: Fix potential use-after-free by grabbing of_node reference ---
commit 7ae04c53ea07238f9983ba45fc72ac6070931cc9
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

spmi: Fix potential use-after-free by grabbing of_node reference

In of_spmi_register_devices(), a node is assigned to a newly created SPMI child
device during iteration. The refcount is dropped at every iteration, so a
use-after-free may occur when an overlay is dynamically unloaded. This commit
resolves the issue by increasing the of_node refcount when assigning the node
and releasing it in spmi_dev_release().

Closes: https://sashiko.dev/#/patchset/20260608100949.36309-1-angelogioacchino.delregno@collabora.com?part=2

> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
> index 57b7c0cb42407..f6d2f4bddfcd2 100644
> --- a/drivers/spmi/spmi.c
> +++ b/drivers/spmi/spmi.c
> @@ -24,6 +24,7 @@ static void spmi_dev_release(struct device *dev)
>  {
>  	struct spmi_device *sdev = to_spmi_device(dev);
>  
> +	of_node_put(dev->of_node);
>  	kfree(sdev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the resource
release paths in this file, I noticed a potential IDA corruption in
spmi_controller_alloc() during the error path.

If ida_alloc() fails under memory pressure:

drivers/spmi/spmi.c:spmi_controller_alloc() {
    ...
	id = ida_alloc(&ctrl_ida, GFP_KERNEL);
	if (id < 0) {
		dev_err(parent,
			"unable to allocate SPMI controller identifier.\n");
		spmi_controller_put(ctrl);
		return ERR_PTR(id);
	}
    ...
}

It calls spmi_controller_put(ctrl), which eventually invokes spmi_ctrl_release():

drivers/spmi/spmi.c:spmi_ctrl_release() {
    ...
	ida_free(&ctrl_ida, ctrl->nr);
	kfree(ctrl);
}

Since the controller memory is zero-initialized and ctrl->nr hasn't
been set yet when ida_alloc() fails, ctrl->nr is implicitly 0.

Does this unconditionally free ID 0, inadvertently removing the identifier
for another active SPMI controller and leading to sysfs naming collisions
(spmi-0) on subsequent allocations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721093626.96264-1-angelogioacchino.delregno@collabora.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy