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

[PATCH v3] 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 struct ufs_rpmb_dev is allocated via kzalloc_obj(), and free it
from the device release callback, following the same pattern as MMC RPMB.

Initialize the hba->rpmbs list in ufshcd_alloc_host() to prevent NULL
pointer dereference in the device teardown path if ufs_rpmb_probe()
fails.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260714064356.CF7101F000E9@smtp.kernel.org/
Signed-off-by: Jiazi Li <jiazi.li@transsion.com>
Signed-off-by: Ao Sun <ao.sun@transsion.com>
---
Changes in v3:
  - switch devm_kzalloc() to kzalloc_obj()
  - init rpmbs list in ufshcd
---
Changes in v2:
  - drop the release callback
  - init rpmbs list early
---
 drivers/ufs/core/ufs-rpmb.c | 8 ++++----
 drivers/ufs/core/ufshcd.c   | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
index ffad049872b9..cfd1f074d004 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);
+	kfree(ufs_rpmb);
 }
 
 /* UFS RPMB device registration */
@@ -152,8 +152,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,
@@ -165,7 +163,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
 		if (!cap)
 			continue;
 
-		ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
+		ufs_rpmb = kzalloc_obj(*ufs_rpmb);
 		if (!ufs_rpmb) {
 			ret = -ENOMEM;
 			goto err_out;
@@ -224,6 +222,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 +243,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);
 	}
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d3044a3089b5..60227069db06 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10988,6 +10988,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 	hba->nop_out_timeout = NOP_OUT_TIMEOUT;
 	ufshcd_set_sg_entry_size(hba, sizeof(struct ufshcd_sg_entry));
 	INIT_LIST_HEAD(&hba->clk_list_head);
+	INIT_LIST_HEAD(&hba->rpmbs);
 	spin_lock_init(&hba->outstanding_lock);
 
 	*hba_handle = hba;
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `ufs_rpmb` memory is freed while its child RPMB device (`rdev`) may still be in use by other subsystems (e.g., OP-TEE), leading to a Use-After-Free when routing frames.
--

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

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

This commit changes the UFS RPMB device teardown sequence to unregister the
child RPMB device before the parent device. It also moves the memory freeing
of the ufs_rpmb_dev struct to the parent device's release callback and
initializes the hba->rpmbs list earlier.

Link: https://lore.kernel.org/all/20260714064356.CF7101F000E9@smtp.kernel.org/

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9a..cfd1f074d0043 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c

[ ... ]

> @@ -224,6 +222,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);

[Severity: High]
Could this teardown sequence result in a use-after-free if another
subsystem (like OP-TEE) has already acquired a reference to the RPMB device?

When rpmb_dev_unregister() is called, the driver core removes the child
from the hierarchy and drops its reference to the parent. The subsequent
device_unregister() on the parent drops its refcount to zero, triggering
ufs_rpmb_device_release() and freeing the ufs_rpmb object.

If a concurrent subsystem holds a reference to rdev via rpmb_dev_get(),
it might later call rpmb_route_frames(rdev, ...). This executes
rdev->descr.route_frames(rdev->dev.parent, ...).

Since dev.parent is now a dangling pointer to the freed parent device,
ufs_rpmb_route_frames() will access freed memory when fetching the driver
data.

>  	}
>  
> @@ -244,6 +243,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]
Does this have the same use-after-free risk as the probe error path above?

If a concurrent subsystem retains a reference to the child device,
unregistering the devices in this order frees the parent object while the
child device might still route frames, leading to a use-after-free on the
parent device pointer.

>  	}

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

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

[Bean Huo]

On Tue, 2026-07-21 at 08:41 +0000, Ao Sun wrote:
> 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 struct ufs_rpmb_dev is allocated via kzalloc_obj(), and free it
> from the device release callback, following the same pattern as MMC RPMB.
> 
> Initialize the hba->rpmbs list in ufshcd_alloc_host() to prevent NULL
> pointer dereference in the device teardown path if ufs_rpmb_probe()
> fails.
> 
> Reported-by: sashiko-bot@kernel.org
> Closes:
> https://lore.kernel.org/all/20260714064356.CF7101F000E9@smtp.kernel.org/
> Signed-off-by: Jiazi Li <jiazi.li@transsion.com>
> Signed-off-by: Ao Sun <ao.sun@transsion.com>
> ---
> Changes in v3:
>   - switch devm_kzalloc() to kzalloc_obj()
>   - init rpmbs list in ufshcd
> ---
> Changes in v2:
>   - drop the release callback
>   - init rpmbs list early
> ---
>  drivers/ufs/core/ufs-rpmb.c | 8 ++++----
>  drivers/ufs/core/ufshcd.c   | 1 +
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9..cfd1f074d004 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);
> +       kfree(ufs_rpmb);
>  }
>  
>  /* UFS RPMB device registration */
> @@ -152,8 +152,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,
> @@ -165,7 +163,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
>                 if (!cap)
>                         continue;
>  
> -               ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb),
> GFP_KERNEL);
> +               ufs_rpmb = kzalloc_obj(*ufs_rpmb);
>                 if (!ufs_rpmb) {
>                         ret = -ENOMEM;
>                         goto err_out;
> @@ -224,6 +222,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 +243,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);
>         }
>  
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d3044a3089b5..60227069db06 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10988,6 +10988,7 @@ int ufshcd_alloc_host(struct device *dev, struct
> ufs_hba **hba_handle)
>         hba->nop_out_timeout = NOP_OUT_TIMEOUT;
>         ufshcd_set_sg_entry_size(hba, sizeof(struct ufshcd_sg_entry));
>         INIT_LIST_HEAD(&hba->clk_list_head);
> +       INIT_LIST_HEAD(&hba->rpmbs);
>         spin_lock_init(&hba->outstanding_lock);
>  
>         *hba_handle = hba;


Hi Ao,

Thanks for the patch. I reviewed the whole RPMB core and OPTEE call sequencey,
the issue is real, 

The issue is the lifetime of ufs_rpmb / ufs_rpmb->dev versus the rpmb device
rdev when a consumer holds a reference of rdev.


but I don't think we can take this patch. moving rpmb_dev_unregister() into the
remove path and freeing ufs_rpmb eagerly, then make use-after-free.


OP-TEE caches the rpmb device: on the RPMB probe path: optee->rpmb_dev =
rpmb_dev_find_device(), whch taking its own reference, and reuses that handle
for every subsequent frame in handle_rpc_func_rpmb_frames(). It only drops the
reference in secure-world rescan or at optee teardown. And rpmb_class_intf has
only add_dev, but no remove_dev, so nothing tells OP-TEE core when we unregister
the device.


I think the correct fix belongs in rpmb-core: a revocation barrier so that once
rpmb_dev_unregister() returns, route_frames() is guaranteed never to run
again, for example:

      route_frames()
	{
          down_read(&rdev->route_lock);  //block 
          if (rdev->no_route)
              ret = -ENODEV;
          else
              ret = rdev->descr.route_frames(rdev->dev.parent, ...);
          up_read(&rdev->route_lock);
          return ret;
      }
      
      int rpmb_dev_unregister(struct rpmb_dev *rdev)
      {
          down_write(&rdev->route_lock);  //drain in-flight, block future 
          rdev->no_route = true;
          up_write(&rdev->route_lock);
          device_del(&rdev->dev);
          rpmb_dev_put(rdev);
          return 0;
      }   
      
@jens, how do you think? I'm happy to help with the rpmb-core piece if that's
useful.


the INIT_LIST_HEAD() move is a separate patch (NULL deref on the early-return
probe paths, since hba->rpmbs is left zeroed). Please split it into new patch
with a Fixes tag.


Kind regards, 
Bean

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

[Bean Huo]

On Wed, 2026-07-22 at 11:43 +0200, Bean Huo wrote:
> On Tue, 2026-07-21 at 08:41 +0000, Ao Sun wrote:
> > 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 struct ufs_rpmb_dev is allocated via kzalloc_obj(), and free it
> > from the device release callback, following the same pattern as MMC RPMB.
> > 
> > Initialize the hba->rpmbs list in ufshcd_alloc_host() to prevent NULL
> > pointer dereference in the device teardown path if ufs_rpmb_probe()
> > fails.
> > 
> > Reported-by: sashiko-bot@kernel.org
> > Closes:
> > https://lore.kernel.org/all/20260714064356.CF7101F000E9@smtp.kernel.org/
> > Signed-off-by: Jiazi Li <jiazi.li@transsion.com>
> > Signed-off-by: Ao Sun <ao.sun@transsion.com>
> > ---
> > Changes in v3:
> >   - switch devm_kzalloc() to kzalloc_obj()
> >   - init rpmbs list in ufshcd
> > ---
> > Changes in v2:
> >   - drop the release callback
> >   - init rpmbs list early
> > ---
> >  drivers/ufs/core/ufs-rpmb.c | 8 ++++----
> >  drivers/ufs/core/ufshcd.c   | 1 +
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> > index ffad049872b9..cfd1f074d004 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);
> > +       kfree(ufs_rpmb);
> >  }
> >  
> >  /* UFS RPMB device registration */
> > @@ -152,8 +152,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,
> > @@ -165,7 +163,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> >                 if (!cap)
> >                         continue;
> >  
> > -               ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb),
> > GFP_KERNEL);
> > +               ufs_rpmb = kzalloc_obj(*ufs_rpmb);
> >                 if (!ufs_rpmb) {
> >                         ret = -ENOMEM;
> >                         goto err_out;
> > @@ -224,6 +222,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 +243,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);
> >         }
> >  
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index d3044a3089b5..60227069db06 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -10988,6 +10988,7 @@ int ufshcd_alloc_host(struct device *dev, struct
> > ufs_hba **hba_handle)
> >         hba->nop_out_timeout = NOP_OUT_TIMEOUT;
> >         ufshcd_set_sg_entry_size(hba, sizeof(struct ufshcd_sg_entry));
> >         INIT_LIST_HEAD(&hba->clk_list_head);
> > +       INIT_LIST_HEAD(&hba->rpmbs);
> >         spin_lock_init(&hba->outstanding_lock);
> >  
> >         *hba_handle = hba;
> 
> 
> Hi Ao,
> 
> Thanks for the patch. I reviewed the whole RPMB core and OPTEE call sequencey,
> the issue is real, 
> 
> The issue is the lifetime of ufs_rpmb / ufs_rpmb->dev versus the rpmb device
> rdev when a consumer holds a reference of rdev.
> 
> 
> but I don't think we can take this patch. moving rpmb_dev_unregister() into
> the
> remove path and freeing ufs_rpmb eagerly, then make use-after-free.
> 
> 
> OP-TEE caches the rpmb device: on the RPMB probe path: optee->rpmb_dev =
> rpmb_dev_find_device(), whch taking its own reference, and reuses that handle
> for every subsequent frame in handle_rpc_func_rpmb_frames(). It only drops the
> reference in secure-world rescan or at optee teardown. And rpmb_class_intf has
> only add_dev, but no remove_dev, so nothing tells OP-TEE core when we
> unregister
> the device.
> 
> 
> I think the correct fix belongs in rpmb-core: a revocation barrier so that
> once
> rpmb_dev_unregister() returns, route_frames() is guaranteed never to run
> again, for example:
> 
>       route_frames()
>         {
>           down_read(&rdev->route_lock);  //block 
>           if (rdev->no_route)
>               ret = -ENODEV;
>           else
>               ret = rdev->descr.route_frames(rdev->dev.parent, ...);
>           up_read(&rdev->route_lock);
>           return ret;
>       }
>       
>       int rpmb_dev_unregister(struct rpmb_dev *rdev)
>       {
>           down_write(&rdev->route_lock);  //drain in-flight, block future 
>           rdev->no_route = true;
>           up_write(&rdev->route_lock);
>           device_del(&rdev->dev);
>           rpmb_dev_put(rdev);
>           return 0;
>       }   
>       
> @jens, how do you think? I'm happy to help with the rpmb-core piece if that's
> useful.
> 

Hi Jens, 

noticed your email chnaged, congrats on the new company! When you get a chance,
would you mind taking a look at the proposal above? Your suggestion on the rpmb-
core side would be much appreciated.


Kind regards,
Bean

> 
> the INIT_LIST_HEAD() move is a separate patch (NULL deref on the early-return
> probe paths, since hba->rpmbs is left zeroed). Please split it into new patch
> with a Fixes tag.
> 
> 
> Kind regards, 
> Bean
> 
>