* [PATCH v3] scsi: ufs: core: Fix UFS RPMB device teardown order @ 2026-07-21 8:41 Ao Sun 2026-07-22 9:43 ` Bean Huo 0 siblings, 1 reply; 3+ messages in thread From: Ao Sun @ 2026-07-21 8:41 UTC (permalink / raw) To: alim.akhtar, avri.altman, bvanassche, James.Bottomley, martin.petersen, peter.wang, beanhuo, can.guo Cc: linux-scsi, linux-kernel, Jiazi Li, Hongyan Xia, Ao Sun, sashiko-bot 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] scsi: ufs: core: Fix UFS RPMB device teardown order 2026-07-21 8:41 [PATCH v3] scsi: ufs: core: Fix UFS RPMB device teardown order Ao Sun @ 2026-07-22 9:43 ` Bean Huo 2026-07-27 21:18 ` Bean Huo 0 siblings, 1 reply; 3+ messages in thread From: Bean Huo @ 2026-07-22 9:43 UTC (permalink / raw) To: jens.wiklander, Ao Sun, alim.akhtar, avri.altman, bvanassche, James.Bottomley, martin.petersen, peter.wang, beanhuo, can.guo Cc: linux-scsi, linux-kernel, Jiazi Li, Hongyan Xia, sashiko-bot 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] scsi: ufs: core: Fix UFS RPMB device teardown order 2026-07-22 9:43 ` Bean Huo @ 2026-07-27 21:18 ` Bean Huo 0 siblings, 0 replies; 3+ messages in thread From: Bean Huo @ 2026-07-27 21:18 UTC (permalink / raw) To: jens.wiklander, Ao Sun, alim.akhtar, avri.altman, bvanassche, James.Bottomley, martin.petersen, peter.wang, beanhuo, can.guo Cc: linux-scsi, linux-kernel, Jiazi Li, Hongyan Xia, sashiko-bot 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 > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-07-27 21:18 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-07-21 8:41 [PATCH v3] scsi: ufs: core: Fix UFS RPMB device teardown order Ao Sun 2026-07-22 9:43 ` Bean Huo 2026-07-27 21:18 ` Bean Huo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®