Re: [PATCH v2] scsi: ufs: core: Fix UFS RPMB device teardown order

[sashiko-bot@kernel.org]

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

New issues:
- [High] Removing the `release` callback from `ufs_rpmb->dev` directly violates the driver core API contract and will trigger a loud kernel warning (`WARN(1)`) upon device unregistration.

Pre-existing issues:
- [High] Allocating a `struct device` via `devm_kzalloc()` incorrectly ties its memory lifetime to the parent device rather than its own kobject reference count, risking an exploitable Use-After-Free.
- [High] Moving `INIT_LIST_HEAD(&hba->rpmbs)` to `ufs_rpmb_probe()` is an incomplete fix and still allows a NULL pointer dereference during teardown if the UFS host initialization fails.
--

--- Patch [1]: [PATCH v2] scsi: ufs: core: Fix UFS RPMB device teardown order ---
commit 690fc44195b81ef7f3bac92dd785777284387f68
Author: Ao Sun <ao.sun@transsion.com>

scsi: ufs: core: Fix UFS RPMB device teardown order

The child RPMB device holds a reference to its parent, so the parent's
release callback cannot be invoked if the child device is still registered.
Remove the rpmb_dev_unregister() from the parent release handler, and
unregister the child RPMB device ahead of the parent device in the remove
path.

Closes: https://lore.kernel.org/all/20260623121840.300121F000E9@smtp.kernel.org/

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9a..8c63913f7c397 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -141,6 +134,8 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
>  	u32 cap;
>  	int ret;
>  
> +	INIT_LIST_HEAD(&hba->rpmbs);
> +

[Severity: High]
This is a pre-existing issue, but does this still allow a NULL pointer
dereference during driver unbind?

If ufshcd_probe_hba() fails asynchronously after the driver is bound,
ufs_rpmb_probe() might be skipped entirely. This leaves hba->rpmbs
zero-initialized.

Later, if the driver unbinds, ufshcd_remove() calls ufs_rpmb_remove():

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_remove() {
    if (list_empty(&hba->rpmbs))
        return;

    list_for_each_entry_safe(ufs_rpmb, tmp, &hba->rpmbs, node) {
        ...
}

Because &hba->rpmbs points to valid memory but contains NULL pointers,
list_empty() will evaluate to false, and list_for_each_entry_safe() will
dereference the NULL pointer.

[ ... ]

> @@ -174,7 +167,6 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
>  		ufs_rpmb->hba = hba;
>  		ufs_rpmb->dev.parent = &hba->ufs_rpmb_wlun->sdev_gendev;
>  		ufs_rpmb->dev.bus = &ufs_rpmb_bus_type;

[Severity: High]
This is a pre-existing issue, but is it safe to use devm_kzalloc() for
ufs_rpmb, which contains a struct device?

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_probe() {
    ...
    ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
    ...
}

Allocating a struct device using devres ties its memory lifetime to the
parent device rather than its own kobject reference count. If userspace
holds a reference to ufs_rpmb->dev, and the UFS driver unbinds, devres
will free the memory. The subsequent put_device() could then touch
freed memory.

> -		ufs_rpmb->dev.release = ufs_rpmb_device_release;

[Severity: High]
Will removing the release callback trigger a kernel warning during device
unregistration?

The driver core requires every dynamically allocated struct device to have
a valid release callback. When ufs_rpmb is unregistered during probe
failure or driver unbind:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_remove() {
    ...
    device_unregister(&ufs_rpmb->dev);
    ...
}

If no release function is provided directly or via class/type, the driver core
will trigger a WARN_ON when the kobject reference count reaches zero.

>  		dev_set_name(&ufs_rpmb->dev, "ufs_rpmb%d", region);
>  
>  		/* Set driver data BEFORE device_register */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714061532.167-1-ao.sun@transsion.com?part=1