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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Premature release of the OF node reference in `spmi_device_remove()` causes a use-after-free during device unregistration.

Pre-existing issues:
- [High] If `ida_alloc()` fails in `spmi_controller_alloc()`, the error path erroneously frees ID 0.
--

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

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

This patch increases the of_node refcount when assigning in
of_spmi_register_devices() and releases it in spmi_device_remove() to fix a
potential use-after-free during dynamic overlay unloading.

Link: 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..6e701aff60454 100644
> --- a/drivers/spmi/spmi.c
> +++ b/drivers/spmi/spmi.c
> @@ -86,6 +86,9 @@ EXPORT_SYMBOL_GPL(spmi_device_add);
>   */
>  void spmi_device_remove(struct spmi_device *sdev)
>  {
> +	if (IS_ENABLED(CONFIG_OF))
> +		of_node_put(sdev->dev.of_node);
> +
>  	device_unregister(&sdev->dev);
>  }

[Severity: High]
Could this prematurely release the OF node reference?

By dropping the reference before device_unregister() completes, this seems to
risk a use-after-free. During device_unregister(), device_del() is called and
the driver core emits a KOBJ_REMOVE uevent. This triggers the bus callback
spmi_drv_uevent(), which calls of_device_uevent_modalias() where it directly
dereferences the freed dev->of_node to read the compatible string.

Would it be better to drop the OF node reference in spmi_dev_release() so it
is bound to the device's lifetime? (If moved there, the manual of_node_put()
in the spmi_device_add() error path might need to be removed to prevent a
double-free).

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in the error handling for spmi_controller_alloc().

drivers/spmi/spmi.c:spmi_controller_alloc() {
    ...
	ctrl = kzalloc_flex(*ctrl, priv, size);
    ...
	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);
	}
    ...
}

If ida_alloc() fails, wouldn't this error path inadvertently free ID 0?

Since kzalloc_flex() zero-initializes ctrl->nr, an ida_alloc() failure will
call spmi_controller_put(ctrl) while ctrl->nr is still 0. This triggers the
spmi_ctrl_release() callback:

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

Because it unconditionally frees ctrl->nr, ID 0 is released. If ID 0 was
already assigned to another SPMI controller, this could cause its ID to be
erroneously marked as available, potentially leading to a sysfs naming
collision ('spmi-0') during the next controller allocation.

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

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