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@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxx
> Closes:
> https://lore.kernel.org/all/20260714064356.CF7101F000E9@xxxxxxxxxxxxxxx/
> Signed-off-by: Jiazi Li <jiazi.li@xxxxxxxxxxxxx>
> Signed-off-by: Ao Sun <ao.sun@xxxxxxxxxxxxx>
> ---
> 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