From: Bean Huo <beanhuo@iokpp.de>
To: Stanley Jhu <stanleyjhu@google.com>, jenswi@kernel.org, mkp@kernel.org
Cc: gregkh@linuxfoundation.org, arnd@arndb.de, bvanassche@acm.org,
avri.altman@sandisk.com, alim.akhtar@samsung.com,
beanhuo@micron.com, can.guo@oss.qualcomm.com, ulfh@kernel.org,
linusw@kernel.org, tomas.winkler@intel.com,
shyamsaini@linux.microsoft.com, alex.bennee@linaro.org,
James.Bottomley@HansenPartnership.com,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/3] scsi: ufs: rpmb: Decouple device lifecycle from devres to avoid UAF
Date: Mon, 14 Sep 2026 11:55:25 +0200 [thread overview]
Message-ID: <b1fccd35bf35abf384c31bee6cf3040e02051755.camel@iokpp.de> (raw)
In-Reply-To: <20260913033633.3159296-3-stanleyjhu@google.com>
On Sun, 2026-09-13 at 11:36 +0800, Stanley Jhu wrote:
> struct ufs_rpmb_dev embeds a struct device but is allocated with
> devm_kzalloc() against the host controller. devres frees that memory
> when the host driver detaches, regardless of the device reference count,
> and probe takes no reference on the RPMB well known LU either. An
> in-flight request then runs on a freed scsi_device:
>
> BUG: KASAN: slab-use-after-free in scsi_execute_cmd+0x998/0xab0
> Read of size 8 at addr fff00000c82d4008 by task rpmb_hold/100
> Call trace:
> scsi_execute_cmd+0x998/0xab0
> ufs_sec_submit.isra.0+0x110/0x150
> ufs_rpmb_route_frames+0x148/0x460
> rpmb_route_frames+0x64/0xd0
> Freed by task 1:
> kfree+0x2b8/0x5c4
> scsi_device_dev_release+0x6b8/0xb7c
> __scsi_remove_device+0x1c8/0x318
> scsi_remove_host+0xc0/0x258
> ufshcd_remove+0x1c0/0x22c
>
> The release callback cannot clean this up, because it never runs.
> rpmb_dev_register() makes the rpmb_dev a child of ufs_rpmb->dev, so
> device_add() holds a reference on the parent. ufs_rpmb_remove() only
> calls device_unregister() on that parent, whose count therefore never
> reaches zero, and ufs_rpmb_device_release() is the only caller of
> rpmb_dev_unregister().
>
> Tie the memory to the reference count instead:
>
> - allocate with kzalloc_obj() and free with kfree() in
> ufs_rpmb_device_release()
> - pin the SCSI WLUN with scsi_device_get() in probe and release it with
> scsi_device_put() in the release callback
> - call rpmb_dev_unregister() from ufs_rpmb_remove() and from the probe
> error unwind, before device_unregister(), so the cycle is broken
> - reject requests once the WLUN is offline, rather than submitting to a
> device that SCSI has already removed
>
> On its own this patch changes nothing observable: device_register()
> still fails because the ufs_rpmb bus is never registered, so no RPMB
> device exists. The next patch removes that bus, and the trace above was
> taken with both applied. The ordering is deliberate: no commit in this
> series enables RPMB registration before the lifetime handling is
> correct.
hard to read this commit message. too long, lots of information.
>
> Fixes: b06b8c421485 ("scsi: ufs: core: Add OP-TEE based RPMB driver for UFS
> devices")
> Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
> ---
> drivers/ufs/core/ufs-rpmb.c | 97 +++++++++++++++++++++----------------
> 1 file changed, 55 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index 783ecfc7581d..373b60aba916 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -14,6 +14,7 @@
> #include <linux/module.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> +#include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/rpmb.h>
> #include <linux/string.h>
> @@ -36,13 +37,14 @@ struct ufs_rpmb_dev {
> u8 region_id;
> struct device dev;
> struct rpmb_dev *rdev;
> - struct ufs_hba *hba;
> + struct scsi_device *sdev;
> struct list_head node;
> };
>
> -static int ufs_sec_submit(struct ufs_hba *hba, u16 spsp, void *buffer, size_t
> len, bool send)
> +static int ufs_sec_submit(struct ufs_rpmb_dev *ufs_rpmb, u16 spsp,
> + void *buffer, size_t len, bool send)
> {
> - struct scsi_device *sdev = hba->ufs_rpmb_wlun;
> + struct scsi_device *sdev = ufs_rpmb->sdev;
> struct scsi_failure failure_defs[] = {
> {
> .sense = UNIT_ATTENTION,
> @@ -61,6 +63,9 @@ static int ufs_sec_submit(struct ufs_hba *hba, u16 spsp,
> void *buffer, size_t le
> };
> u8 cdb[12] = { };
>
> + if (!sdev || !scsi_device_online(sdev))
> + return -ENODEV;
> +
> cdb[0] = send ? SECURITY_PROTOCOL_OUT : SECURITY_PROTOCOL_IN;
> cdb[1] = UFS_RPMB_SEC_PROTOCOL;
> put_unaligned_be16(spsp, &cdb[2]);
> @@ -73,13 +78,12 @@ static int ufs_sec_submit(struct ufs_hba *hba, u16 spsp,
> void *buffer, size_t le
>
> /* UFS RPMB route frames implementation */
> static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int
> req_len, u8 *resp,
> - unsigned int resp_len)
> + unsigned int resp_len)
> {
> struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
> struct rpmb_frame *frm_out = (struct rpmb_frame *)req;
> bool need_result_read = true;
> u16 req_type, protocol_id;
> - struct ufs_hba *hba;
> int ret;
>
> if (!ufs_rpmb) {
> @@ -87,8 +91,6 @@ static int ufs_rpmb_route_frames(struct device *dev, u8
> *req, unsigned int req_l
> return -ENODEV;
> }
>
> - hba = ufs_rpmb->hba;
> -
> /* req_resp is at the end of an RPMB frame. */
> if (req_len < sizeof(*frm_out))
> return -EINVAL;
> @@ -121,7 +123,7 @@ static int ufs_rpmb_route_frames(struct device *dev, u8
> *req, unsigned int req_l
>
> protocol_id = ufs_rpmb->region_id << 8 | UFS_RPMB_SEC_PROTOCOL_ID;
>
> - ret = ufs_sec_submit(hba, protocol_id, req, req_len, true);
> + ret = ufs_sec_submit(ufs_rpmb, protocol_id, req, req_len, true);
> if (ret) {
> dev_err(dev, "Command failed with ret=%d\n", ret);
> return ret;
> @@ -132,7 +134,7 @@ static int ufs_rpmb_route_frames(struct device *dev, u8
> *req, unsigned int req_l
>
> memset(frm_resp, 0, sizeof(*frm_resp));
> put_unaligned_be16(RPMB_RESULT_READ, &frm_resp->req_resp);
> - ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, true);
> + ret = ufs_sec_submit(ufs_rpmb, protocol_id, resp, resp_len,
> true);
> if (ret) {
> dev_err(dev, "Result read request failed with
> ret=%d\n", ret);
> return ret;
> @@ -140,7 +142,7 @@ static int ufs_rpmb_route_frames(struct device *dev, u8
> *req, unsigned int req_l
> }
>
> if (!ret) {
> - ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, false);
> + ret = ufs_sec_submit(ufs_rpmb, protocol_id, resp, resp_len,
> false);
> if (ret)
> dev_err(dev, "Response read failed with ret=%d\n",
> ret);
> }
> @@ -150,23 +152,30 @@ static int ufs_rpmb_route_frames(struct device *dev, u8
> *req, unsigned int req_l
>
> static void ufs_rpmb_device_release(struct device *dev)
> {
> - struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
> + struct ufs_rpmb_dev *ufs_rpmb = container_of(dev, struct ufs_rpmb_dev,
> dev);
>
> - rpmb_dev_unregister(ufs_rpmb->rdev);
> + scsi_device_put(ufs_rpmb->sdev);
> + kfree(ufs_rpmb);
> }
>
> /* UFS RPMB device registration */
> int ufs_rpmb_probe(struct ufs_hba *hba)
> {
> + struct rpmb_descr descr = {
> + .type = RPMB_TYPE_UFS,
> + .route_frames = ufs_rpmb_route_frames,
> + .reliable_wr_count = hba->dev_info.rpmb_io_size,
> + };
> + struct scsi_device *sdev = hba->ufs_rpmb_wlun;
> struct ufs_rpmb_dev *ufs_rpmb, *it, *tmp;
> u8 dev_id[UFS_RPMB_ID_LEN];
> struct rpmb_dev *rdev;
> - char *cid = NULL;
> + char *cid;
> int region;
> u32 cap;
> int ret;
>
> - if (!hba->ufs_rpmb_wlun || hba->dev_info.b_advanced_rpmb_en) {
> + if (!sdev || hba->dev_info.b_advanced_rpmb_en) {
> dev_info(hba->dev, "Skip OP-TEE RPMB registration\n");
> return -ENODEV;
> }
> @@ -177,25 +186,28 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> return -EINVAL;
> }
>
> - struct rpmb_descr descr = {
> - .type = RPMB_TYPE_UFS,
> - .route_frames = ufs_rpmb_route_frames,
> - .reliable_wr_count = hba->dev_info.rpmb_io_size,
> - };
> -
> for (region = 0; region < ARRAY_SIZE(hba->dev_info.rpmb_region_size);
> region++) {
> cap = hba->dev_info.rpmb_region_size[region];
> 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;
> }
>
> - ufs_rpmb->hba = hba;
> - ufs_rpmb->dev.parent = &hba->ufs_rpmb_wlun->sdev_gendev;
> + INIT_LIST_HEAD(&ufs_rpmb->node);
> +
> + ret = scsi_device_get(sdev);
> + if (ret) {
> + kfree(ufs_rpmb);
> + goto err_out;
> + }
> +
> + ufs_rpmb->sdev = sdev;
> + ufs_rpmb->region_id = region;
> + ufs_rpmb->dev.parent = &sdev->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);
> @@ -206,16 +218,14 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> ret = device_register(&ufs_rpmb->dev);
> if (ret) {
> dev_err(hba->dev, "Failed to register UFS RPMB device
> %d\n", region);
> - put_device(&ufs_rpmb->dev);
> - goto err_out;
> + goto err_put;
> }
>
> /* Create unique ID by appending region number to device_id */
> cid = kasprintf(GFP_KERNEL, "%s-R%d", hba->dev_info.device_id,
> region);
> if (!cid) {
> - device_unregister(&ufs_rpmb->dev);
> ret = -ENOMEM;
> - goto err_out;
> + goto err_unreg;
> }
>
> blake2b(NULL, 0, cid, strlen(cid), dev_id, UFS_RPMB_ID_LEN);
> @@ -226,29 +236,33 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
>
> /* Register RPMB device */
> rdev = rpmb_dev_register(&ufs_rpmb->dev, &descr);
> + kfree(cid);
> if (IS_ERR(rdev)) {
> dev_err(hba->dev, "Failed to register UFS RPMB
> device.\n");
> - device_unregister(&ufs_rpmb->dev);
> ret = PTR_ERR(rdev);
> - goto err_out;
> + goto err_unreg;
> }
>
> - kfree(cid);
> - cid = NULL;
> -
> ufs_rpmb->rdev = rdev;
> - ufs_rpmb->region_id = region;
> -
> list_add_tail(&ufs_rpmb->node, &hba->rpmbs);
>
> dev_info(hba->dev, "UFS RPMB region %d registered
> (capacity=%u)\n", region, cap);
> }
>
> return 0;
> +
> +err_unreg:
> + device_unregister(&ufs_rpmb->dev);
> + goto err_out;
> +err_put:
> + put_device(&ufs_rpmb->dev);
> err_out:
> - kfree(cid);
> list_for_each_entry_safe(it, tmp, &hba->rpmbs, node) {
> - list_del(&it->node);
> + list_del_init(&it->node);
> + if (it->rdev) {
> + rpmb_dev_unregister(it->rdev);
> + it->rdev = NULL;
> + }
> device_unregister(&it->dev);
> }
>
> @@ -265,14 +279,13 @@ void ufs_rpmb_remove(struct ufs_hba *hba)
>
> /* Remove all registered RPMB devices */
> list_for_each_entry_safe(ufs_rpmb, tmp, &hba->rpmbs, node) {
> - dev_info(hba->dev, "Removing UFS RPMB region %d\n", ufs_rpmb-
> >region_id);
removing the dev_info() not related to this fix.
> - /* Remove from list first */
> - list_del(&ufs_rpmb->node);
> - /* Unregister device */
> + list_del_init(&ufs_rpmb->node);
why uses list_del_init(), list_del() is not enough?
> + if (ufs_rpmb->rdev) {
every entry on the list has rdev set, so this check is not needed?
> + rpmb_dev_unregister(ufs_rpmb->rdev);
> + ufs_rpmb->rdev = NULL;
> + }
> device_unregister(&ufs_rpmb->dev);
> }
> -
> - dev_info(hba->dev, "All UFS RPMB devices unregistered\n");
removing the dev_info() not related to this fix.
> }
>
> MODULE_LICENSE("GPL v2");
all are nits, feel free to add:
Reviewed-by: Bean Huo <beanhuo@micron.com>
Kind regards,
Bean
next prev parent reply other threads:[~2026-09-14 9:55 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-13 3:36 [PATCH v4 0/3] rpmb: Fix request serialisation and teardown races Stanley Jhu
2026-09-13 3:36 ` [PATCH v4 1/3] rpmb: core: Guard frame requests and teardown with mutex Stanley Jhu
2026-09-13 17:43 ` Bean Huo
2026-09-13 3:36 ` [PATCH v4 2/3] scsi: ufs: rpmb: Decouple device lifecycle from devres to avoid UAF Stanley Jhu
2026-09-13 3:47 ` sashiko-bot
2026-09-13 5:07 ` Stanley Jhu
2026-09-14 9:55 ` Bean Huo [this message]
2026-09-14 14:48 ` Stanley Jhu
2026-09-13 3:36 ` [PATCH v4 3/3] scsi: ufs: rpmb: Drop the unregistered ufs_rpmb bus Stanley Jhu
2026-09-14 10:05 ` Bean Huo
2026-09-14 14:48 ` Stanley Jhu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b1fccd35bf35abf384c31bee6cf3040e02051755.camel@iokpp.de \
--to=beanhuo@iokpp.de \
--cc=James.Bottomley@HansenPartnership.com \
--cc=alex.bennee@linaro.org \
--cc=alim.akhtar@samsung.com \
--cc=arnd@arndb.de \
--cc=avri.altman@sandisk.com \
--cc=beanhuo@micron.com \
--cc=bvanassche@acm.org \
--cc=can.guo@oss.qualcomm.com \
--cc=gregkh@linuxfoundation.org \
--cc=jenswi@kernel.org \
--cc=linusw@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=mkp@kernel.org \
--cc=shyamsaini@linux.microsoft.com \
--cc=stanleyjhu@google.com \
--cc=tomas.winkler@intel.com \
--cc=ulfh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.