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

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

[Ao Sun]

From: Ao Sun <ao.sun@transsion.com>

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.

Memory for ufs_rpmb_dev is allocated via devm_kzalloc(), devres frees it on
parent remove, so no extra resource release is needed, and remove redundant
release callback.

Initialize hba->rpmbs at the start of ufs_rpmb_probe() so the list is
always initialized, preventing NULL-pointer dereferences in
ufs_rpmb_remove() during driver teardown.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260623121840.300121F000E9@smtp.kernel.org/
Signed-off-by: Jiazi Li <jiazi.li@transsion.com>
Signed-off-by: Ao Sun <ao.sun@transsion.com>
---
Changes in v2:
  - drop the release callback
  - init rpmbs list early
---
 drivers/ufs/core/ufs-rpmb.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
index ffad049872b9..8c63913f7c39 100644
--- a/drivers/ufs/core/ufs-rpmb.c
+++ b/drivers/ufs/core/ufs-rpmb.c
@@ -124,13 +124,6 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
 	return ret;
 }
 
-static void ufs_rpmb_device_release(struct device *dev)
-{
-	struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
-
-	rpmb_dev_unregister(ufs_rpmb->rdev);
-}
-
 /* UFS RPMB device registration */
 int ufs_rpmb_probe(struct ufs_hba *hba)
 {
@@ -141,6 +134,8 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
 	u32 cap;
 	int ret;
 
+	INIT_LIST_HEAD(&hba->rpmbs);
+
 	if (!hba->ufs_rpmb_wlun || hba->dev_info.b_advanced_rpmb_en) {
 		dev_info(hba->dev, "Skip OP-TEE RPMB registration\n");
 		return -ENODEV;
@@ -152,8 +147,6 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
 		return -EINVAL;
 	}
 
-	INIT_LIST_HEAD(&hba->rpmbs);
-
 	struct rpmb_descr descr = {
 		.type = RPMB_TYPE_UFS,
 		.route_frames = ufs_rpmb_route_frames,
@@ -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;
-		ufs_rpmb->dev.release = ufs_rpmb_device_release;
 		dev_set_name(&ufs_rpmb->dev, "ufs_rpmb%d", region);
 
 		/* Set driver data BEFORE device_register */
@@ -224,6 +216,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
 	kfree(cid);
 	list_for_each_entry_safe(it, tmp, &hba->rpmbs, node) {
 		list_del(&it->node);
+		rpmb_dev_unregister(it->rdev);
 		device_unregister(&it->dev);
 	}
 
@@ -244,6 +237,7 @@ void ufs_rpmb_remove(struct ufs_hba *hba)
 		/* Remove from list first */
 		list_del(&ufs_rpmb->node);
 		/* Unregister device */
+		rpmb_dev_unregister(ufs_rpmb->rdev);
 		device_unregister(&ufs_rpmb->dev);
 	}
 
-- 
2.34.1

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

[sashiko-bot]

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