[PATCH v4 0/3] rpmb: Fix request serialisation and teardown races

[PATCH v4 0/3] rpmb: Fix request serialisation and teardown races

[Stanley Jhu]

This merges two series that were both last posted as v3:

  [PATCH v3] rpmb: core: Guard frame requests and teardown with mutex
  https://lore.kernel.org/all/20260910015515.1991789-1-stanleyjhu@google.com/

  [PATCH v3 0/2] scsi: ufs: rpmb: Fix bus registration and device lifecycle
  https://lore.kernel.org/all/20260910015503.1991119-1-stanleyjhu@google.com/

They turned out to be one problem. The UFS patches make RPMB registration
succeed for the first time, and registering RPMB devices without the core
fix triggers a use-after-free on any unbind that races an in-flight
request. Landing them as two independent series would leave that window
open in between.

The order is chosen so that no commit enables RPMB registration before the
lifetime handling and the serialisation are in place:

  1/3 fixes the generic core. It fixes the teardown race on eMMC today
      and carries a stable tag. It has no effect on UFS, where nothing
      registers yet.
  2/3 fixes the UFS device lifetime. Still nothing registers.
  3/3 removes the never registered bus, which is what finally makes UFS
      RPMB devices appear.

drivers/misc/rpmb-core.c and drivers/ufs/ are not in the same tree. 1/3
has no build or runtime dependency on the other two and can be taken on
its own; 2/3 and 3/3 must not land before it.

Verified on QEMU arm64 with KASAN, PROVE_LOCKING and SLUB_DEBUG_ON, against
a UFS device advertising four 4 MiB RPMB regions. Two kthreads on different
CPUs issue RPMB_GET_WRITE_COUNTER against the same region 20000 times each
and compare the nonce echoed back; a third thread holds an rpmb_dev
reference and keeps issuing requests across a host unbind. OP-TEE is the
only in-kernel consumer of rpmb_route_frames(), so an out-of-tree module
stands in for it.

  tree                  rpmb_dev  stolen responses  unbind
  --------------------  --------  ----------------  --------------------
  3/3 alone             4         16512 of 40000    KASAN use-after-free
  3/3 and 2/3, no 1/3   4         17030 of 40000    KASAN use-after-free
  all three             4         0 of 40000        clean

The intermediate points were booted and unbound as well. After 1/3 and
after 2/3 no rpmb_dev is registered, so neither test applies to them, and
neither point reports KASAN.

Upstream QEMU answers SECURITY PROTOCOL IN/OUT on the RPMB well known LU
with INVALID OPCODE, so the three rows above also needed a local QEMU
change that implements the authenticated frame state machine. I can post
that to qemu-devel separately, and send the test module to anyone who
wants to reproduce the numbers.

Changes since v3:
- merged the two series and reordered so registration is enabled last
- dropped the incorrect Tested: line from the core patch
- dropped Cc: stable from the UFS patches; the feature has never worked on
  any released kernel, so there is nothing to back port
- rewrote the commit messages around the measured results

Stanley Jhu (3):
  rpmb: core: Guard frame requests and teardown with mutex
  scsi: ufs: rpmb: Decouple device lifecycle from devres to avoid UAF
  scsi: ufs: rpmb: Drop the unregistered ufs_rpmb bus

 drivers/misc/rpmb-core.c    |  34 ++++++++++--
 drivers/ufs/core/ufs-rpmb.c | 102 +++++++++++++++++++-----------------
 include/linux/rpmb.h        |   6 +++
 3 files changed, 90 insertions(+), 52 deletions(-)

-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH v4 1/3] rpmb: core: Guard frame requests and teardown with mutex

[Stanley Jhu]

rpmb_route_frames() has no serialisation. That has two independent
consequences.

Concurrent requests on one rpmb_dev corrupt each other. Two kthreads on
different CPUs issuing RPMB_GET_WRITE_COUNTER against the same UFS RPMB
region, 20000 iterations each:

  thread 0 done: 20000 iterations, mismatches=10889, errors=0
  thread 1 done: 20000 iterations, mismatches=6141, errors=0
  race complete: MISMATCH=17030 ERRORS=0

A mismatch is a response whose echoed nonce belongs to the other thread,
so 43% of requests returned somebody else's frame. The errors count is
transport failures, so nothing failed and the corruption is silent to
the caller. An authenticated RPMB operation is not one command. The UFS
provider issues SECURITY PROTOCOL OUT carrying the request, then
SECURITY PROTOCOL IN to collect the response, with a result read request
in between for write-type operations. JESD220F 12.4.7 states that any
request other than a result read overwrites the result register of the
region, so a request landing in the middle of another initiator's
sequence destroys its response. The same chapter states that a region
processes one authenticated operation at a time, so the rpmb_dev is the
granularity the device itself assumes.

A request can also still be in flight when the provider tears down. The
core reaches the provider through rdev->dev.parent, and the provider is
free to release that device as soon as rpmb_dev_unregister() returns.
Reference counting on the rpmb_dev does not prevent this:
rpmb_dev_unregister() calls device_del(), which drops the reference that
device_add() took on the parent, so the parent can be freed while the
rpmb_dev is still alive and still routable. Unbinding a UFS host while a
consumer holds an rpmb_dev reference and keeps issuing requests:

  BUG: KASAN: slab-use-after-free in ufs_rpmb_route_frames+0x328/0x420
  Read of size 8 at addr fff00000c833bce8 by task rpmb_hold/100
  Call trace:
   ufs_rpmb_route_frames+0x328/0x420
   rpmb_route_frames+0x64/0xd0
  Freed by task 1:
   kfree+0x2b8/0x5c4
   ufs_rpmb_device_release+0x3c/0x60
   device_release+0xa0/0x1fc
   device_unregister+0x20/0x38
   ufs_rpmb_remove+0x130/0x230
   ufshcd_remove+0x54/0x22c

Both measurements needed patches 2/3 and 3/3 of this series applied,
because UFS RPMB registration fails on mainline. The stable tag is for
eMMC, which registers today. mmc_route_rpmb_frames() packs the whole
sequence into one block request, so eMMC is not exposed to the
interleaving above, but it does have the teardown window:
mmc_blk_remove() reaches rpmb_dev_unregister() through
mmc_blk_remove_parts() near its start, while the queue that
mmc_route_rpmb_frames() submits to is only torn down at the end by
mmc_blk_remove_req(), whose comment notes that it is freeing the queue
that stops new requests being accepted. The eMMC window is from source
reading; I have not reproduced it.

The kerneldoc change is part of the fix. Calling rpmb_dev_unregister()
from a release callback cannot work, because the child rpmb_dev holds a
reference on its parent, so the parent's release callback never runs.
The UFS provider does exactly that today, inert only because its
registration fails; patch 2/3 moves the call to the remove path.

Add a mutex and a dead flag to struct rpmb_dev. rpmb_route_frames()
holds the mutex across the whole sequence and returns -ENODEV once the
flag is set. rpmb_dev_unregister() sets the flag under the mutex before
device_del(), so it cannot return while a request is inside the
provider. Closing the teardown window requires excluding unregistration
for the whole duration of a request, which is the same exclusion that
serialises two requests, so one mutex covers both.

On the same test with this patch applied, MISMATCH=0 and the unbind is
clean.

Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB) subsystem")
Cc: stable@vger.kernel.org
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/misc/rpmb-core.c | 34 +++++++++++++++++++++++++++++-----
 include/linux/rpmb.h     |  6 ++++++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/rpmb-core.c b/drivers/misc/rpmb-core.c
index ecf14acf230a..bbc3c404ad6f 100644
--- a/drivers/misc/rpmb-core.c
+++ b/drivers/misc/rpmb-core.c
@@ -45,16 +45,28 @@ EXPORT_SYMBOL_GPL(rpmb_dev_put);
  * @rsp:	rpmb response frames
  * @rsp_len:	length of rpmb response frames in bytes
  *
+ * Context: Might sleep.
+ *
  * Returns: < 0 on failure
  */
 int rpmb_route_frames(struct rpmb_dev *rdev, u8 *req,
 		      unsigned int req_len, u8 *rsp, unsigned int rsp_len)
 {
-	if (!req || !req_len || !rsp || !rsp_len)
+	int ret;
+
+	if (!rdev || !req || !req_len || !rsp || !rsp_len)
 		return -EINVAL;
 
-	return rdev->descr.route_frames(rdev->dev.parent, req, req_len,
-					rsp, rsp_len);
+	mutex_lock(&rdev->lock);
+	if (rdev->dead) {
+		mutex_unlock(&rdev->lock);
+		return -ENODEV;
+	}
+
+	ret = rdev->descr.route_frames(rdev->dev.parent, req, req_len,
+				       rsp, rsp_len);
+	mutex_unlock(&rdev->lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(rpmb_route_frames);
 
@@ -62,6 +74,7 @@ static void rpmb_dev_release(struct device *dev)
 {
 	struct rpmb_dev *rdev = to_rpmb_dev(dev);
 
+	mutex_destroy(&rdev->lock);
 	ida_free(&rpmb_ida, rdev->id);
 	kfree(rdev->descr.dev_id);
 	kfree(rdev);
@@ -123,8 +136,9 @@ EXPORT_SYMBOL_GPL(rpmb_interface_unregister);
  * rpmb_dev_unregister() - unregister RPMB partition from the RPMB subsystem
  * @rdev: the rpmb device to unregister
  *
- * This function should be called from the release function of the
- * underlying device used when the RPMB device was registered.
+ * This function should be called from the remove or unbind callback of the
+ * underlying device used when the RPMB device was registered, never from
+ * a device release callback.
  *
  * Returns: < 0 on failure
  */
@@ -133,6 +147,14 @@ int rpmb_dev_unregister(struct rpmb_dev *rdev)
 	if (!rdev)
 		return -EINVAL;
 
+	mutex_lock(&rdev->lock);
+	if (rdev->dead) {
+		mutex_unlock(&rdev->lock);
+		return 0;
+	}
+	rdev->dead = true;
+	mutex_unlock(&rdev->lock);
+
 	device_del(&rdev->dev);
 
 	rpmb_dev_put(rdev);
@@ -164,6 +186,7 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev,
 	rdev = kzalloc_obj(*rdev);
 	if (!rdev)
 		return ERR_PTR(-ENOMEM);
+	mutex_init(&rdev->lock);
 	rdev->descr = *descr;
 	rdev->descr.dev_id = kmemdup(descr->dev_id, descr->dev_id_len,
 				     GFP_KERNEL);
@@ -194,6 +217,7 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev,
 err_free_dev_id:
 	kfree(rdev->descr.dev_id);
 err_free_rdev:
+	mutex_destroy(&rdev->lock);
 	kfree(rdev);
 	return ERR_PTR(ret);
 }
diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
index ed3f8e431eff..814ac3e69337 100644
--- a/include/linux/rpmb.h
+++ b/include/linux/rpmb.h
@@ -7,6 +7,7 @@
 #define __RPMB_H__
 
 #include <linux/device.h>
+#include <linux/mutex.h>
 #include <linux/types.h>
 
 /**
@@ -48,15 +49,20 @@ struct rpmb_descr {
  * struct rpmb_dev - device which can support RPMB partition
  *
  * @dev              : device
+ * @lock             : protects in-flight operations against teardown
  * @id               : device_id
  * @list_node        : linked list node
  * @descr            : RPMB description
+ * @dead             : set to true when device is unregistered
  */
 struct rpmb_dev {
 	struct device dev;
+	/* Protects in-flight operations against teardown */
+	struct mutex lock;
 	int id;
 	struct list_head list_node;
 	struct rpmb_descr descr;
+	bool dead;
 };
 
 #define to_rpmb_dev(x)		container_of((x), struct rpmb_dev, dev)
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH v4 2/3] scsi: ufs: rpmb: Decouple device lifecycle from devres to avoid UAF

[Stanley Jhu]

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.

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);
-		/* Remove from list first */
-		list_del(&ufs_rpmb->node);
-		/* Unregister device */
+		list_del_init(&ufs_rpmb->node);
+		if (ufs_rpmb->rdev) {
+			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");
 }
 
 MODULE_LICENSE("GPL v2");
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH v4 3/3] scsi: ufs: rpmb: Drop the unregistered ufs_rpmb bus

[Stanley Jhu]

ufs_rpmb_probe() assigns ufs_rpmb_bus_type to dev.bus, but that bus is
never passed to bus_register(). bus_add_device() rejects devices on an
unregistered bus, so device_register() has always failed:

  bus_add_device: cannot add device 'ufs_rpmb0' to unregistered bus
  'ufs_rpmb'
  ufshcd 0000:00:02.0: Failed to register UFS RPMB device 0

ufs_rpmb_probe() unwinds on the first failure, so no RPMB region has
ever been registered and /sys/bus/ufs_rpmb/devices/ has never been
populated.

ufs_rpmb_bus_type declares no .match and no .probe, and no driver binds
to it. RPMB devices are exposed to consumers through /sys/class/rpmb/,
which rpmb_dev_register() already sets up. Drop the bus rather than
register it: device_register() works with dev.bus left NULL given a
parent and a release callback, both of which ufs_rpmb_probe() sets.

With the bus gone, on a device advertising four RPMB regions:

  ufshcd 0000:00:02.0: UFS RPMB region 0 registered (capacity=32)
  ufshcd 0000:00:02.0: UFS RPMB region 1 registered (capacity=32)
  ufshcd 0000:00:02.0: UFS RPMB region 2 registered (capacity=32)
  ufshcd 0000:00:02.0: UFS RPMB region 3 registered (capacity=32)

/sys/class/rpmb then holds rpmb0 to rpmb3, and unbinding the host
removes them.

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 | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
index 373b60aba916..684fb37705c7 100644
--- a/drivers/ufs/core/ufs-rpmb.c
+++ b/drivers/ufs/core/ufs-rpmb.c
@@ -28,10 +28,6 @@
 #define UFS_RPMB_SEC_PROTOCOL		0xEC	/* JEDEC UFS application */
 #define UFS_RPMB_SEC_PROTOCOL_ID	0x01	/* JEDEC UFS RPMB protocol ID, CDB byte3 */
 
-static const struct bus_type ufs_rpmb_bus_type = {
-	.name = "ufs_rpmb",
-};
-
 /* UFS RPMB device structure */
 struct ufs_rpmb_dev {
 	u8 region_id;
@@ -208,7 +204,6 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
 		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);
 
-- 
2.55.0.1007.g17ff1f9808-goog

Re: [PATCH v4 2/3] scsi: ufs: rpmb: Decouple device lifecycle from devres to avoid UAF

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Module Reference Leak / Unload Deadlock
--

commit c2fd812022915196c82ed205c8c33614922be2b5
Author: Stanley Jhu <stanleyjhu@google.com>

scsi: ufs: rpmb: Decouple device lifecycle from devres to avoid UAF

This commit addresses a use-after-free issue by tying the ufs_rpmb_dev
memory to its reference count instead of devres. It pins the SCSI WLUN
during probe, cleans up properly during release to break the lifecycle
cycle, and rejects requests once the WLUN is offline.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index 783ecfc7581d7..373b60aba9166 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
[ ... ]
> @@ -177,25 +186,28 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
[ ... ]
>  		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);

[Severity: Medium]
Does this code introduce a module reference leak that prevents driver unloading?

When ufs_rpmb_probe() calls scsi_device_get(), the following call chain occurs:

ufs_rpmb_probe()
  scsi_device_get(sdev)
    try_module_get(sdev->host->hostt->module)

Since this is called during the driver's probe sequence, and the SCSI host
template module is likely this exact same module (ufshcd-core), it ends up
taking a reference on itself. 

Because the reference won't be dropped until the device is released, and the
device won't be released while the module is held active, doesn't this create
a lifecycle deadlock where rmmod will permanently fail with -EBUSY?

> +		if (ret) {
> +			kfree(ufs_rpmb);
> +			goto err_out;
> +		}
> +
> +		ufs_rpmb->sdev = sdev;
> +		ufs_rpmb->region_id = region;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913033633.3159296-1-stanleyjhu@google.com?part=2

Re: [PATCH v4 2/3] scsi: ufs: rpmb: Decouple device lifecycle from devres to avoid UAF

[Stanley Jhu]

On Sun, 13 Sep 2026 03:47:25 +0000, sashiko-bot@kernel.org wrote:
> When ufs_rpmb_probe() calls scsi_device_get(), the following call chain occurs:
> ufs_rpmb_probe() -> scsi_device_get(sdev) -> try_module_get(sdev->host->hostt->module).
> Since this is called during the driver's probe sequence, and the SCSI host
> template module is likely this exact same module (ufshcd-core), it ends up
> taking a reference on itself. Because the reference won't be dropped until the
> device is released, and the device won't be released while the module is held
> active, doesn't this create a lifecycle deadlock where rmmod will permanently
> fail with -EBUSY?

The release point is not tied to module_exit, but to the glue driver's unbind
phase. Because ufshcd-core cannot be removed while a host glue driver (such as
ufshcd-pci) is active due to symbol dependencies, teardown inherently begins
with the glue driver. Unloading the glue driver initiates a hardware unbind
cascade that destroys the RPMB devices, synchronously dropping all dynamic
references on ufshcd-core. Once the glue driver finishes unloading and
relinquishes its static symbol dependency, ufshcd-core is left with zero
references and unloads cleanly.

This lifecycle was empirically verified under QEMU ARM64 with modular builds
(CONFIG_SCSI_UFSHCD=m, CONFIG_SCSI_UFSHCD_PCI=m), where the usecount comprises
both the static symbol dependency held by the glue driver (+1) and the dynamic
references acquired by the instantiated RPMB devices (+4):

  Phase                                      ufshcd_core usecount
  -----------------------------------------  --------------------
  1. insmod ufshcd-core                      0
  2. insmod ufshcd-pci (probes 4 RPMB devs)  5 (1 static symbol + 4
dynamic device)
  3. sysfs unbind 0000:00:02.0               1 (4 dynamic device refs dropped)
  4. sysfs rebind 0000:00:02.0               5 (4 dynamic device refs acquired)
  5. rmmod ufshcd_pci                        0 (RC=0; synchronous unbind)
  6. rmmod ufshcd_core                       0 (RC=0; unloaded cleanly)
  Kernel splats (KASAN, lockdep, WARN, BUG)  none

Therefore, the cyclic dependency concern raised by the analyzer is a false
positive, and the teardown sequence safely completes without deadlocks.

Thanks,
Stanley Jhu

Re: [PATCH v4 1/3] rpmb: core: Guard frame requests and teardown with mutex

[Bean Huo]

On Sun, 2026-09-13 at 11:36 +0800, Stanley Jhu wrote:
> rpmb_route_frames() has no serialisation. That has two independent
> consequences.
> 
> Concurrent requests on one rpmb_dev corrupt each other. Two kthreads on
> different CPUs issuing RPMB_GET_WRITE_COUNTER against the same UFS RPMB
> region, 20000 iterations each:
> 
>   thread 0 done: 20000 iterations, mismatches=10889, errors=0
>   thread 1 done: 20000 iterations, mismatches=6141, errors=0
>   race complete: MISMATCH=17030 ERRORS=0
> 
> A mismatch is a response whose echoed nonce belongs to the other thread,
> so 43% of requests returned somebody else's frame. The errors count is
> transport failures, so nothing failed and the corruption is silent to
> the caller. An authenticated RPMB operation is not one command. The UFS
> provider issues SECURITY PROTOCOL OUT carrying the request, then
> SECURITY PROTOCOL IN to collect the response, with a result read request
> in between for write-type operations. JESD220F 12.4.7 states that any
> request other than a result read overwrites the result register of the
> region, so a request landing in the middle of another initiator's
> sequence destroys its response. The same chapter states that a region
> processes one authenticated operation at a time, so the rpmb_dev is the
> granularity the device itself assumes.
> 
> A request can also still be in flight when the provider tears down. The
> core reaches the provider through rdev->dev.parent, and the provider is
> free to release that device as soon as rpmb_dev_unregister() returns.
> Reference counting on the rpmb_dev does not prevent this:
> rpmb_dev_unregister() calls device_del(), which drops the reference that
> device_add() took on the parent, so the parent can be freed while the
> rpmb_dev is still alive and still routable. Unbinding a UFS host while a
> consumer holds an rpmb_dev reference and keeps issuing requests:
> 
>   BUG: KASAN: slab-use-after-free in ufs_rpmb_route_frames+0x328/0x420
>   Read of size 8 at addr fff00000c833bce8 by task rpmb_hold/100
>   Call trace:
>    ufs_rpmb_route_frames+0x328/0x420
>    rpmb_route_frames+0x64/0xd0
>   Freed by task 1:
>    kfree+0x2b8/0x5c4
>    ufs_rpmb_device_release+0x3c/0x60
>    device_release+0xa0/0x1fc
>    device_unregister+0x20/0x38
>    ufs_rpmb_remove+0x130/0x230
>    ufshcd_remove+0x54/0x22c
> 
> Both measurements needed patches 2/3 and 3/3 of this series applied,
> because UFS RPMB registration fails on mainline. The stable tag is for
> eMMC, which registers today. mmc_route_rpmb_frames() packs the whole
> sequence into one block request, so eMMC is not exposed to the
> interleaving above, but it does have the teardown window:
> mmc_blk_remove() reaches rpmb_dev_unregister() through
> mmc_blk_remove_parts() near its start,while the queue that
> mmc_route_rpmb_frames() submits to is only torn down at the end by
> mmc_blk_remove_req(), whose comment notes that it is freeing the queue
> that stops new requests being accepted. The eMMC window is from source
> reading; I have not reproduced it.
> 
> The kerneldoc change is part of the fix. Calling rpmb_dev_unregister()
> from a release callback cannot work, because the child rpmb_dev holds a
> reference on its parent, so the parent's release callback never runs.
> The UFS provider does exactly that today, inert only because its
> registration fails; patch 2/3 moves the call to the remove path.
> 
> Add a mutex and a dead flag to struct rpmb_dev. rpmb_route_frames()
> holds the mutex across the whole sequence and returns -ENODEV once the
> flag is set. rpmb_dev_unregister() sets the flag under the mutex before
> device_del(), so it cannot return while a request is inside the
> provider. Closing the teardown window requires excluding unregistration
> for the whole duration of a request, which is the same exclusion that
> serialises two requests, so one mutex covers both.
> 
> On the same test with this patch applied, MISMATCH=0 and the unbind is
> clean.
> 

Hi Stanley,

The fix looks right to me, the commit message is too long, it is easier to read
the code changes than the message.


> Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB)
> subsystem")
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
> ---
>  drivers/misc/rpmb-core.c | 34 +++++++++++++++++++++++++++++-----
>  include/linux/rpmb.h     |  6 ++++++
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/rpmb-core.c b/drivers/misc/rpmb-core.c
> index ecf14acf230a..bbc3c404ad6f 100644
> --- a/drivers/misc/rpmb-core.c
> +++ b/drivers/misc/rpmb-core.c
> @@ -45,16 +45,28 @@ EXPORT_SYMBOL_GPL(rpmb_dev_put);
>   * @rsp:       rpmb response frames
>   * @rsp_len:   length of rpmb response frames in bytes
>   *
> + * Context: Might sleep.
> + *
>   * Returns: < 0 on failure
>   */
>  int rpmb_route_frames(struct rpmb_dev *rdev, u8 *req,
>                       unsigned int req_len, u8 *rsp, unsigned int rsp_len)
>  {
> -       if (!req || !req_len || !rsp || !rsp_len)
> +       int ret;
> +
> +       if (!rdev || !req || !req_len || !rsp || !rsp_len)
>                 return -EINVAL;
>  
> -       return rdev->descr.route_frames(rdev->dev.parent, req, req_len,
> -                                       rsp, rsp_len);
> +       mutex_lock(&rdev->lock);
> +       if (rdev->dead) {
> +               I (&rdev->lock);
> +               return -ENODEV;
> +       }
> +
> +       ret = rdev->descr.route_frames(rdev->dev.parent, req, req_len,
> +                                      rsp, rsp_len);
> +       mutex_unlock(&rdev->lock);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(rpmb_route_frames);
>  
> @@ -62,6 +74,7 @@ static void rpmb_dev_release(struct device *dev)
>  {
>         struct rpmb_dev *rdev = to_rpmb_dev(dev);
>  
> +       mutex_destroy(&rdev->lock);
>         ida_free(&rpmb_ida, rdev->id);
>         kfree(rdev->descr.dev_id);
>         kfree(rdev);
> @@ -123,8 +136,9 @@ EXPORT_SYMBOL_GPL(rpmb_interface_unregister);
>   * rpmb_dev_unregister() - unregister RPMB partition from the RPMB subsystem
>   * @rdev: the rpmb device to unregister
>   *
> - * This function should be called from the release function of the
> - * underlying device used when the RPMB device was registered.
> + * This function should be called from the remove or unbind callback of the
> + * underlying device used when the RPMB device was registered, never from
> + * a device release callback.
>   *
>   * Returns: < 0 on failure
>   */
> @@ -133,6 +147,14 @@ int rpmb_dev_unregister(struct rpmb_dev *rdev)
>         if (!rdev)
>                 return -EINVAL;
>  
> +       mutex_lock(&rdev->lock);
> +       if (rdev->dead) {
> +               mutex_unlock(&rdev->lock);
> +               return 0;
> +       }
> +       rdev->dead = true;
> +       mutex_unlock(&rdev->lock);
> +
>         device_del(&rdev->dev);
>  
>         rpmb_dev_put(rdev);
> @@ -164,6 +186,7 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev,
>         rdev = kzalloc_obj(*rdev);
>         if (!rdev)
>                 return ERR_PTR(-ENOMEM);
> +       mutex_init(&rdev->lock);
>         rdev->descr = *descr;
>         rdev->descr.dev_id = kmemdup(descr->dev_id, descr->dev_id_len,
>                                      GFP_KERNEL);
> @@ -194,6 +217,7 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev,
>  err_free_dev_id:
>         kfree(rdev->descr.dev_id);
>  err_free_rdev:
> +       mutex_destroy(&rdev->lock);
>         kfree(rdev);
>         return ERR_PTR(ret);
>  }
> diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
> index ed3f8e431eff..814ac3e69337 100644
> --- a/include/linux/rpmb.h
> +++ b/include/linux/rpmb.h
> @@ -7,6 +7,7 @@
>  #define __RPMB_H__
>  
>  #include <linux/device.h>
> +#include <linux/mutex.h>
>  #include <linux/types.h>
>  
>  /**
> @@ -48,15 +49,20 @@ struct rpmb_descr {
>   * struct rpmb_dev - device which can support RPMB partition
>   *
>   * @dev              : device
> + * @lock             : protects in-flight operations against teardown
>   * @id               : device_id
>   * @list_node        : linked list node
>   * @descr            : RPMB description
> + * @dead             : set to true when device is unregistered
>   */
>  struct rpmb_dev {
>         struct device dev;
> +       /* Protects in-flight operations against teardown */
> +       struct mutex lock;

This lock also makes route_frames calls run one at a time, not only protect
against teardown.


>         int id;
>         struct list_head list_node;
>         struct rpmb_descr descr;
> +       bool dead;
>  };
>  
>  #define to_rpmb_dev(x)         container_of((x), struct rpmb_dev, dev)

Both are nits, feel free to add:

Reviewed-by: Bean Huo <beanhuo@micron.com>

Kind regards,
Bean