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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Allocating a structure embedding a `struct device` via `devm_kzalloc()` creates a fundamental lifetime mismatch. The patch exacerbates this by adding `devm_kfree()` to the `release` callback, triggering a fatal Use-After-Free if the device release is delayed.
- [High] If `ufs_rpmb_probe()` exits early, `hba->rpmbs` is never initialized, leading to a guaranteed NULL-pointer dereference in `ufs_rpmb_remove()` during driver teardown.
--

commit 79815281175f9750b2f8b1eccd86223bc783c72a
Author: Ao Sun <ao.sun@transsion.com>

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

This commit modifies the teardown order of the UFS RPMB device by
unregistering the child RPMB device ahead of the parent device in the
remove path. It also moves the memory deallocation for the ufs_rpmb
structure to the parent release callback.

Link: https://lore.kernel.org/all/20260618064142.8C30E1F000E9@smtp.kernel.org/

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9a..dcb7e521284f1 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -128,7 +128,7 @@ 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);
> +	devm_kfree(ufs_rpmb->hba->dev, ufs_rpmb);

[Severity: High]
This is a pre-existing issue, but does allocating a structure embedding a
struct device via devm_kzalloc() create a lifetime mismatch?

This patch exacerbates the issue by adding devm_kfree() to the release
callback. If a user holds a sysfs attribute open during driver unbind,
devres will automatically free ufs_rpmb. Later, when the sysfs file is
closed and the final kobject reference is dropped, ufs_rpmb_device_release()
executes and dereferences the already-freed ufs_rpmb pointer to read
ufs_rpmb->hba->dev, leading to a use-after-free.

>  }

[ ... ]

> @@ -244,6 +245,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);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but if ufs_rpmb_probe() exits
early before calling INIT_LIST_HEAD(&hba->rpmbs), won't hba->rpmbs remain
zero-initialized?

If the driver is unbound later, ufs_rpmb_remove() is called on that
zero-initialized list:

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) {
...

Since head->next is NULL on a zero-initialized list, list_empty()
evaluates to false. This means list_for_each_entry_safe() will dereference
NULL->next during initialization, causing an immediate kernel panic.

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