[PATCH v2 0/2] rpmb: add virtio RPMB driver

[PATCH v2 0/2] rpmb: add virtio RPMB driver

[Haruka Kuze]

This adds a driver on top of the rpmb subsystem for virtio-rpmb devices.

Alex Bennée had an earlier virtio-rpmb driver in his RPMB series [1]
but that stalled due to the user space API.

The one that merged later has no user space API and OP-TEE routes frames
through it so this driver only needs to implement route_frames().

I added patch 1 to fix a use-after-free that I hit while testing
removing the device (OP-TEE can still route frames after driver unregisters
the rpmb device).

I tested this with UML (virtio_uml) and a vhost-user backend I wrote
for this on top of rust-vmm's vhost-device with KASAN, lockdep:

 - key programming
 - write counter
 - single and multi block write and read
 - MAC and counter failures
 - max_wr_cnt
 - unbinding the driver when the rpmb device is held

With OP-TEE 4.11 on QEMU virt arm64 the secure_storage example TA keeps
its objects in RPMB through the driver and an object written to in one
boot is found in the one after.

I have added myself in MAINTAINERS. I would like to maintain the driver.

Changes in v2:
- Use guard() and scoped_guard() for the locks in both patches (Greg)
- Call virtio_device_ready() before vr->vq is set, so a request on
  restore can not kick the queue before DRIVER_OK (sashiko)
- v1: https://lore.kernel.org/all/20261029081317.2264887-1-harukak@zohomail.eu/

- haruka

[1] https://lore.kernel.org/all/20220405093759.1126835-1-alex.bennee@linaro.org/

Haruka Kuze (2):
  rpmb: stop the routing after unregister
  rpmb: add a virtio RPMB device driver

 MAINTAINERS                      |   7 +
 drivers/misc/Kconfig             |  10 ++
 drivers/misc/Makefile            |   1 +
 drivers/misc/rpmb-core.c         |  14 +-
 drivers/misc/virtio_rpmb.c       | 286 +++++++++++++++++++++++++++++++
 include/linux/rpmb.h             |   4 +
 include/uapi/linux/virtio_rpmb.h |  56 ++++++
 7 files changed, 377 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/virtio_rpmb.c
 create mode 100644 include/uapi/linux/virtio_rpmb.h


base-commit: c339f4010dea8b0d754f84ace70cd1b089878ef8
-- 
2.55.0

[PATCH v2 0/2] rpmb: add virtio RPMB driver

[Haruka Kuze via OP-TEE]

This adds a driver on top of the rpmb subsystem for virtio-rpmb devices.

Alex Bennée had an earlier virtio-rpmb driver in his RPMB series [1]
but that stalled due to the user space API.

The one that merged later has no user space API and OP-TEE routes frames
through it so this driver only needs to implement route_frames().

I added patch 1 to fix a use-after-free that I hit while testing
removing the device (OP-TEE can still route frames after driver unregisters
the rpmb device).

I tested this with UML (virtio_uml) and a vhost-user backend I wrote
for this on top of rust-vmm's vhost-device with KASAN, lockdep:

 - key programming
 - write counter
 - single and multi block write and read
 - MAC and counter failures
 - max_wr_cnt
 - unbinding the driver when the rpmb device is held

With OP-TEE 4.11 on QEMU virt arm64 the secure_storage example TA keeps
its objects in RPMB through the driver and an object written to in one
boot is found in the one after.

I have added myself in MAINTAINERS. I would like to maintain the driver.

Changes in v2:
- Use guard() and scoped_guard() for the locks in both patches (Greg)
- Call virtio_device_ready() before vr->vq is set, so a request on
  restore can not kick the queue before DRIVER_OK (sashiko)
- v1: https://lore.kernel.org/all/20261029081317.2264887-1-harukak@zohomail.eu/

- haruka

[1] https://lore.kernel.org/all/20220405093759.1126835-1-alex.bennee@linaro.org/

Haruka Kuze (2):
  rpmb: stop the routing after unregister
  rpmb: add a virtio RPMB device driver

 MAINTAINERS                      |   7 +
 drivers/misc/Kconfig             |  10 ++
 drivers/misc/Makefile            |   1 +
 drivers/misc/rpmb-core.c         |  14 +-
 drivers/misc/virtio_rpmb.c       | 286 +++++++++++++++++++++++++++++++
 include/linux/rpmb.h             |   4 +
 include/uapi/linux/virtio_rpmb.h |  56 ++++++
 7 files changed, 377 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/virtio_rpmb.c
 create mode 100644 include/uapi/linux/virtio_rpmb.h


base-commit: c339f4010dea8b0d754f84ace70cd1b089878ef8
-- 
2.55.0

[PATCH v2 1/2] rpmb: stop the routing after unregister

[Haruka Kuze]

From: Haruka Kuze <harukak@zohomail.eu>

OP-TEE has a reference of RPMB device it used. So, it can route
frames through the device after rpmb_dev_unregister() is called.
Therefore, the parent device passed to route_frames() can be
already freed, for example, mmc_blk_remove_rpmb_part() drops the
last reference after rpmb unregister.

This patch clears the route_frames when the device is going to
be removed by getting new rwsem and makes rpmb_route_frames()
return -ENODEV if the device is gone.

Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB) subsystem")
Assisted-by: LLM
Signed-off-by: Haruka Kuze <harukak@zohomail.eu>
---
 drivers/misc/rpmb-core.c | 14 +++++++++++++-
 include/linux/rpmb.h     |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/rpmb-core.c b/drivers/misc/rpmb-core.c
index ecf14acf230a..0ae018d1a276 100644
--- a/drivers/misc/rpmb-core.c
+++ b/drivers/misc/rpmb-core.c
@@ -45,7 +45,7 @@ EXPORT_SYMBOL_GPL(rpmb_dev_put);
  * @rsp:	rpmb response frames
  * @rsp_len:	length of rpmb response frames in bytes
  *
- * Returns: < 0 on failure
+ * Returns: < 0 on failure, -ENODEV if the device has been unregistered
  */
 int rpmb_route_frames(struct rpmb_dev *rdev, u8 *req,
 		      unsigned int req_len, u8 *rsp, unsigned int rsp_len)
@@ -53,6 +53,10 @@ int rpmb_route_frames(struct rpmb_dev *rdev, u8 *req,
 	if (!req || !req_len || !rsp || !rsp_len)
 		return -EINVAL;
 
+	guard(rwsem_read)(&rdev->lock);
+	if (!rdev->descr.route_frames)
+		return -ENODEV;
+
 	return rdev->descr.route_frames(rdev->dev.parent, req, req_len,
 					rsp, rsp_len);
 }
@@ -126,6 +130,10 @@ EXPORT_SYMBOL_GPL(rpmb_interface_unregister);
  * This function should be called from the release function of the
  * underlying device used when the RPMB device was registered.
  *
+ * Once this returns, rpmb_route_frames() no longer calls into
+ * the driver, even though the users might still hold a reference
+ * to the RPMB dev.
+ *
  * Returns: < 0 on failure
  */
 int rpmb_dev_unregister(struct rpmb_dev *rdev)
@@ -133,6 +141,9 @@ int rpmb_dev_unregister(struct rpmb_dev *rdev)
 	if (!rdev)
 		return -EINVAL;
 
+	scoped_guard(rwsem_write, &rdev->lock)
+		rdev->descr.route_frames = NULL;
+
 	device_del(&rdev->dev);
 
 	rpmb_dev_put(rdev);
@@ -165,6 +176,7 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev,
 	if (!rdev)
 		return ERR_PTR(-ENOMEM);
 	rdev->descr = *descr;
+	init_rwsem(&rdev->lock);
 	rdev->descr.dev_id = kmemdup(descr->dev_id, descr->dev_id_len,
 				     GFP_KERNEL);
 	if (!rdev->descr.dev_id) {
diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
index ed3f8e431eff..0a976572d1cd 100644
--- a/include/linux/rpmb.h
+++ b/include/linux/rpmb.h
@@ -7,6 +7,7 @@
 #define __RPMB_H__
 
 #include <linux/device.h>
+#include <linux/rwsem.h>
 #include <linux/types.h>
 
 /**
@@ -51,12 +52,15 @@ struct rpmb_descr {
  * @id               : device_id
  * @list_node        : linked list node
  * @descr            : RPMB description
+ * @lock             : protect @descr.route_frames which is cleared when this
+ *                     device is unregistered
  */
 struct rpmb_dev {
 	struct device dev;
 	int id;
 	struct list_head list_node;
 	struct rpmb_descr descr;
+	struct rw_semaphore lock;
 };
 
 #define to_rpmb_dev(x)		container_of((x), struct rpmb_dev, dev)
-- 
2.55.0

[PATCH v2 1/2] rpmb: stop the routing after unregister

[Haruka Kuze via OP-TEE]

From: Haruka Kuze <harukak@zohomail.eu>

OP-TEE has a reference of RPMB device it used. So, it can route
frames through the device after rpmb_dev_unregister() is called.
Therefore, the parent device passed to route_frames() can be
already freed, for example, mmc_blk_remove_rpmb_part() drops the
last reference after rpmb unregister.

This patch clears the route_frames when the device is going to
be removed by getting new rwsem and makes rpmb_route_frames()
return -ENODEV if the device is gone.

Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB) subsystem")
Assisted-by: LLM
Signed-off-by: Haruka Kuze <harukak@zohomail.eu>
---
 drivers/misc/rpmb-core.c | 14 +++++++++++++-
 include/linux/rpmb.h     |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/rpmb-core.c b/drivers/misc/rpmb-core.c
index ecf14acf230a..0ae018d1a276 100644
--- a/drivers/misc/rpmb-core.c
+++ b/drivers/misc/rpmb-core.c
@@ -45,7 +45,7 @@ EXPORT_SYMBOL_GPL(rpmb_dev_put);
  * @rsp:	rpmb response frames
  * @rsp_len:	length of rpmb response frames in bytes
  *
- * Returns: < 0 on failure
+ * Returns: < 0 on failure, -ENODEV if the device has been unregistered
  */
 int rpmb_route_frames(struct rpmb_dev *rdev, u8 *req,
 		      unsigned int req_len, u8 *rsp, unsigned int rsp_len)
@@ -53,6 +53,10 @@ int rpmb_route_frames(struct rpmb_dev *rdev, u8 *req,
 	if (!req || !req_len || !rsp || !rsp_len)
 		return -EINVAL;
 
+	guard(rwsem_read)(&rdev->lock);
+	if (!rdev->descr.route_frames)
+		return -ENODEV;
+
 	return rdev->descr.route_frames(rdev->dev.parent, req, req_len,
 					rsp, rsp_len);
 }
@@ -126,6 +130,10 @@ EXPORT_SYMBOL_GPL(rpmb_interface_unregister);
  * This function should be called from the release function of the
  * underlying device used when the RPMB device was registered.
  *
+ * Once this returns, rpmb_route_frames() no longer calls into
+ * the driver, even though the users might still hold a reference
+ * to the RPMB dev.
+ *
  * Returns: < 0 on failure
  */
 int rpmb_dev_unregister(struct rpmb_dev *rdev)
@@ -133,6 +141,9 @@ int rpmb_dev_unregister(struct rpmb_dev *rdev)
 	if (!rdev)
 		return -EINVAL;
 
+	scoped_guard(rwsem_write, &rdev->lock)
+		rdev->descr.route_frames = NULL;
+
 	device_del(&rdev->dev);
 
 	rpmb_dev_put(rdev);
@@ -165,6 +176,7 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev,
 	if (!rdev)
 		return ERR_PTR(-ENOMEM);
 	rdev->descr = *descr;
+	init_rwsem(&rdev->lock);
 	rdev->descr.dev_id = kmemdup(descr->dev_id, descr->dev_id_len,
 				     GFP_KERNEL);
 	if (!rdev->descr.dev_id) {
diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
index ed3f8e431eff..0a976572d1cd 100644
--- a/include/linux/rpmb.h
+++ b/include/linux/rpmb.h
@@ -7,6 +7,7 @@
 #define __RPMB_H__
 
 #include <linux/device.h>
+#include <linux/rwsem.h>
 #include <linux/types.h>
 
 /**
@@ -51,12 +52,15 @@ struct rpmb_descr {
  * @id               : device_id
  * @list_node        : linked list node
  * @descr            : RPMB description
+ * @lock             : protect @descr.route_frames which is cleared when this
+ *                     device is unregistered
  */
 struct rpmb_dev {
 	struct device dev;
 	int id;
 	struct list_head list_node;
 	struct rpmb_descr descr;
+	struct rw_semaphore lock;
 };
 
 #define to_rpmb_dev(x)		container_of((x), struct rpmb_dev, dev)
-- 
2.55.0

[PATCH v2 2/2] rpmb: add a virtio RPMB device driver

[Haruka Kuze]

From: Haruka Kuze <harukak@zohomail.eu>

No driver exists for virtio-rpmb devices (virtio ID 28), so OP-TEE
running in a guest can not use them.

This registers the virtio-rpmb devices found with the RPMB sub-system.
Each request is done in one transfer on request queue, and for key
programming and data writing a result read frame is appended so that
the device can return the result.

These frames are copied since the caller's buffers are not always
usable for DMA.

These frames follow the emmc definition, so that the device is
registered as RPMB_TYPE_EMMC. The device has no identifier of own, so
that a fixed dev_id is used for the key derivation for the device.

Based on the virtio-rpmb driver in Alex Bennée's RPMB series.

Link: https://lore.kernel.org/all/20220405093759.1126835-4-alex.bennee@linaro.org/
Signed-off-by: Haruka Kuze <harukak@zohomail.eu>
---
 MAINTAINERS                      |   7 +
 drivers/misc/Kconfig             |  10 ++
 drivers/misc/Makefile            |   1 +
 drivers/misc/virtio_rpmb.c       | 286 +++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_rpmb.h |  56 ++++++
 5 files changed, 360 insertions(+)
 create mode 100644 drivers/misc/virtio_rpmb.c
 create mode 100644 include/uapi/linux/virtio_rpmb.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 748ce4d1c6a2..488003a28534 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29707,6 +29707,13 @@ S:	Maintained
 F:	drivers/char/hw_random/virtio-rng.c
 F:	include/uapi/linux/virtio_rng.h
 
+VIRTIO RPMB DRIVER
+M:	Haruka Kuze <harukak@zohomail.eu>
+L:	virtualization@lists.linux.dev
+S:	Maintained
+F:	drivers/misc/virtio_rpmb.c
+F:	include/uapi/linux/virtio_rpmb.h
+
 VIRTIO RTC DRIVER
 M:	Peter Hilber <peter.hilber@oss.qualcomm.com>
 L:	virtualization@lists.linux.dev
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index c40e89cc5724..eb3518ecc8fe 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -91,6 +91,16 @@ config RPMB
 
 	  If unsure, select N.
 
+config VIRTIO_RPMB
+	tristate "Virtio RPMB driver"
+	depends on VIRTIO && RPMB
+	help
+	  Driver for virtio RPMB devices. The device is registered with the
+	  RPMB subsystem, where in-kernel users such as OP-TEE can use it.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called virtio_rpmb.
+
 config TI_FPC202
 	tristate "TI FPC202 Dual Port Controller"
 	depends on I2C
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7cc3d615d37a..e4390bb8bdc0 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_TIFM_CORE)       	+= tifm_core.o
 obj-$(CONFIG_TIFM_7XX1)       	+= tifm_7xx1.o
 obj-$(CONFIG_PHANTOM)		+= phantom.o
 obj-$(CONFIG_RPMB)		+= rpmb-core.o
+obj-$(CONFIG_VIRTIO_RPMB)	+= virtio_rpmb.o
 obj-$(CONFIG_QCOM_COINCELL)	+= qcom-coincell.o
 obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
 obj-$(CONFIG_SENSORS_BH1770)	+= bh1770glc.o
diff --git a/drivers/misc/virtio_rpmb.c b/drivers/misc/virtio_rpmb.c
new file mode 100644
index 000000000000..4caca378fecc
--- /dev/null
+++ b/drivers/misc/virtio_rpmb.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Virtio RPMB driver
+ *
+ * Copyright (c) 2018-2019 Intel Corporation.
+ * Copyright (c) 2021-2022 Linaro Ltd.
+ *
+ * Based on the virtio-rpmb driver in Alex Bennée's RPMB series.
+ */
+
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/rpmb.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+#include <linux/virtio.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_rpmb.h>
+
+#define VIRTIO_RPMB_FRAME_SIZE	sizeof(struct virtio_rpmb_frame)
+static_assert(VIRTIO_RPMB_FRAME_SIZE == 512);
+
+/*
+ * This device has no identifier on its own.
+ * This value is used as input when OP-TEE is deriving the RPMB
+ * key, so it shall stay stable.
+ */
+static const u8 virtio_rpmb_dev_id[16] = "virtio-rpmb";
+
+/**
+ * struct virtio_rpmb - virtio RPMB device state
+ * @vdev: virtio device
+ * @vq: request queue, NULL while the device is gone or frozen
+ * @lock: serializes requests and protects @vq
+ * @rdev: RPMB device
+ * @max_wr: maximum number of blocks in one write request, 0 if unlimited
+ * @max_rd: maximum number of blocks in one read request, 0 if unlimited
+ */
+struct virtio_rpmb {
+	struct virtio_device *vdev;
+	struct virtqueue *vq;
+	struct mutex lock;
+	struct rpmb_dev *rdev;
+	u8 max_wr;
+	u8 max_rd;
+};
+
+struct virtio_rpmb_req {
+	struct completion done;
+	unsigned int len;
+};
+
+static void virtio_rpmb_done(struct virtqueue *vq)
+{
+	struct virtio_rpmb_req *req;
+	unsigned int len;
+
+	while ((req = virtqueue_get_buf(vq, &len))) {
+		req->len = len;
+		complete(&req->done);
+	}
+}
+
+static int virtio_rpmb_xfer(struct virtio_rpmb *vr, void *out,
+			    unsigned int out_len, void *in, unsigned int in_len)
+{
+	struct scatterlist out_sg, in_sg, *sgs[2] = { &out_sg, &in_sg };
+	struct virtio_rpmb_req req;
+	int ret;
+
+	sg_init_one(&out_sg, out, out_len);
+	sg_init_one(&in_sg, in, in_len);
+	init_completion(&req.done);
+
+	guard(mutex)(&vr->lock);
+	if (!vr->vq)
+		return -ENODEV;
+
+	ret = virtqueue_add_sgs(vr->vq, sgs, 1, 1, &req, GFP_KERNEL);
+	if (ret)
+		return ret;
+
+	virtqueue_kick(vr->vq);
+	wait_for_completion(&req.done);
+
+	return req.len == in_len ? 0 : -EIO;
+}
+
+static int virtio_rpmb_route_frames(struct device *dev, u8 *req,
+				    unsigned int req_len, u8 *resp,
+				    unsigned int resp_len)
+{
+	struct virtio_rpmb *vr = dev_to_virtio(dev)->priv;
+	struct virtio_rpmb_frame *frm = (struct virtio_rpmb_frame *)req;
+	struct virtio_rpmb_frame *result;
+	unsigned int out_len = req_len;
+	bool result_read = false;
+	void *out, *in;
+	int ret;
+
+	if (req_len < VIRTIO_RPMB_FRAME_SIZE)
+		return -EINVAL;
+
+	switch (be16_to_cpu(frm->req_resp)) {
+	case VIRTIO_RPMB_REQ_PROGRAM_KEY:
+		if (req_len != VIRTIO_RPMB_FRAME_SIZE ||
+		    resp_len != VIRTIO_RPMB_FRAME_SIZE)
+			return -EINVAL;
+		result_read = true;
+		break;
+	case VIRTIO_RPMB_REQ_GET_WRITE_COUNTER:
+		if (req_len != VIRTIO_RPMB_FRAME_SIZE ||
+		    resp_len != VIRTIO_RPMB_FRAME_SIZE)
+			return -EINVAL;
+		break;
+	case VIRTIO_RPMB_REQ_DATA_WRITE:
+		if (req_len % VIRTIO_RPMB_FRAME_SIZE ||
+		    resp_len != VIRTIO_RPMB_FRAME_SIZE)
+			return -EINVAL;
+		if (vr->max_wr && req_len / VIRTIO_RPMB_FRAME_SIZE > vr->max_wr)
+			return -EINVAL;
+		result_read = true;
+		break;
+	case VIRTIO_RPMB_REQ_DATA_READ:
+		if (req_len != VIRTIO_RPMB_FRAME_SIZE ||
+		    !resp_len || resp_len % VIRTIO_RPMB_FRAME_SIZE)
+			return -EINVAL;
+		if (vr->max_rd && resp_len / VIRTIO_RPMB_FRAME_SIZE > vr->max_rd)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * The caller's buffers may be unsuitable for DMA, so
+	 * copy the frames.  Writes are followed by result read
+	 * request in the same rpmb xfer, which makes the
+	 * device return the result.
+	 */
+	if (result_read)
+		out_len += VIRTIO_RPMB_FRAME_SIZE;
+
+	out = kmalloc(out_len, GFP_KERNEL);
+	if (!out)
+		return -ENOMEM;
+	in = kzalloc(resp_len, GFP_KERNEL);
+	if (!in) {
+		kfree(out);
+		return -ENOMEM;
+	}
+
+	memcpy(out, req, req_len);
+	if (result_read) {
+		result = out + req_len;
+		memset(result, 0, sizeof(*result));
+		result->block_count = cpu_to_be16(1);
+		result->req_resp = cpu_to_be16(VIRTIO_RPMB_REQ_RESULT_READ);
+	}
+
+	ret = virtio_rpmb_xfer(vr, out, out_len, in, resp_len);
+	if (!ret)
+		memcpy(resp, in, resp_len);
+
+	kfree(in);
+	kfree(out);
+	return ret;
+}
+
+static int virtio_rpmb_init_vq(struct virtio_rpmb *vr)
+{
+	struct virtqueue *vq;
+
+	vq = virtio_find_single_vq(vr->vdev, virtio_rpmb_done, "request");
+	if (IS_ERR(vq))
+		return PTR_ERR(vq);
+
+	virtio_device_ready(vr->vdev);
+
+	scoped_guard(mutex, &vr->lock)
+		vr->vq = vq;
+	return 0;
+}
+
+static void virtio_rpmb_del_vq(struct virtio_rpmb *vr)
+{
+	struct virtio_device *vdev = vr->vdev;
+
+	scoped_guard(mutex, &vr->lock)
+		vr->vq = NULL;
+
+	virtio_reset_device(vdev);
+	vdev->config->del_vqs(vdev);
+}
+
+static int virtio_rpmb_probe(struct virtio_device *vdev)
+{
+	struct rpmb_descr descr = {
+		/* Frames and their semantics are that of eMMC */
+		.type = RPMB_TYPE_EMMC,
+		.route_frames = virtio_rpmb_route_frames,
+		.dev_id = (u8 *)virtio_rpmb_dev_id,
+		.dev_id_len = sizeof(virtio_rpmb_dev_id),
+	};
+	struct virtio_rpmb *vr;
+	u8 capacity;
+	int ret;
+
+	capacity = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
+						capacity));
+	if (!capacity || capacity > 0x80)
+		return dev_err_probe(&vdev->dev, -EINVAL,
+				     "invalid capacity %u\n", capacity);
+
+	vr = devm_kzalloc(&vdev->dev, sizeof(*vr), GFP_KERNEL);
+	if (!vr)
+		return -ENOMEM;
+
+	vdev->priv = vr;
+	vr->vdev = vdev;
+	mutex_init(&vr->lock);
+	vr->max_wr = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
+						  max_wr_cnt));
+	vr->max_rd = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
+						  max_rd_cnt));
+
+	ret = virtio_rpmb_init_vq(vr);
+	if (ret)
+		return ret;
+
+	/*
+	 * eMMC reports the reliable wr count in 512 byte sectors, but the device
+	 * limit is in 256 byte blocks.
+	 */
+	descr.capacity = capacity;
+	descr.reliable_wr_count = (vr->max_wr ?: U8_MAX) / 2;
+
+	vr->rdev = rpmb_dev_register(&vdev->dev, &descr);
+	if (IS_ERR(vr->rdev)) {
+		virtio_rpmb_del_vq(vr);
+		return PTR_ERR(vr->rdev);
+	}
+
+	return 0;
+}
+
+static void virtio_rpmb_remove(struct virtio_device *vdev)
+{
+	struct virtio_rpmb *vr = vdev->priv;
+
+	rpmb_dev_unregister(vr->rdev);
+	virtio_rpmb_del_vq(vr);
+}
+
+static int virtio_rpmb_freeze(struct virtio_device *vdev)
+{
+	virtio_rpmb_del_vq(vdev->priv);
+	return 0;
+}
+
+static int virtio_rpmb_restore(struct virtio_device *vdev)
+{
+	return virtio_rpmb_init_vq(vdev->priv);
+}
+
+static const struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_RPMB, VIRTIO_DEV_ANY_ID },
+	{}
+};
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+static struct virtio_driver virtio_rpmb_driver = {
+	.driver.name	= KBUILD_MODNAME,
+	.id_table	= id_table,
+	.probe		= virtio_rpmb_probe,
+	.remove		= virtio_rpmb_remove,
+	.freeze		= pm_sleep_ptr(virtio_rpmb_freeze),
+	.restore	= pm_sleep_ptr(virtio_rpmb_restore),
+};
+module_virtio_driver(virtio_rpmb_driver);
+
+MODULE_DESCRIPTION("Virtio RPMB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/virtio_rpmb.h b/include/uapi/linux/virtio_rpmb.h
new file mode 100644
index 000000000000..66f45202857a
--- /dev/null
+++ b/include/uapi/linux/virtio_rpmb.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
+/*
+ * Virtio RPMB Device
+ *
+ * The Virtio RPMB Specification:
+ * https://github.com/oasis-tcs/virtio-spec/tree/master/device-types/rpmb
+ */
+
+#ifndef _UAPI_LINUX_VIRTIO_RPMB_H
+#define _UAPI_LINUX_VIRTIO_RPMB_H
+
+#include <linux/types.h>
+
+struct virtio_rpmb_config {
+	__u8 capacity;
+	__u8 max_wr_cnt;
+	__u8 max_rd_cnt;
+};
+
+/* Request types (req_resp) */
+#define VIRTIO_RPMB_REQ_PROGRAM_KEY		0x0001
+#define VIRTIO_RPMB_REQ_GET_WRITE_COUNTER	0x0002
+#define VIRTIO_RPMB_REQ_DATA_WRITE		0x0003
+#define VIRTIO_RPMB_REQ_DATA_READ		0x0004
+#define VIRTIO_RPMB_REQ_RESULT_READ		0x0005
+
+/* Response types (req_resp) */
+#define VIRTIO_RPMB_RESP_PROGRAM_KEY		0x0100
+#define VIRTIO_RPMB_RESP_GET_COUNTER		0x0200
+#define VIRTIO_RPMB_RESP_DATA_WRITE		0x0300
+#define VIRTIO_RPMB_RESP_DATA_READ		0x0400
+
+/* Operation results (result) */
+#define VIRTIO_RPMB_RES_OK			0x0000
+#define VIRTIO_RPMB_RES_GENERAL_FAILURE		0x0001
+#define VIRTIO_RPMB_RES_AUTH_FAILURE		0x0002
+#define VIRTIO_RPMB_RES_COUNT_FAILURE		0x0003
+#define VIRTIO_RPMB_RES_ADDR_FAILURE		0x0004
+#define VIRTIO_RPMB_RES_WRITE_FAILURE		0x0005
+#define VIRTIO_RPMB_RES_READ_FAILURE		0x0006
+#define VIRTIO_RPMB_RES_NO_AUTH_KEY		0x0007
+#define VIRTIO_RPMB_RES_WRITE_COUNTER_EXPIRED	0x0080
+
+struct virtio_rpmb_frame {
+	__u8 stuff[196];
+	__u8 key_mac[32];
+	__u8 data[256];
+	__u8 nonce[16];
+	__be32 write_counter;
+	__be16 address;
+	__be16 block_count;
+	__be16 result;
+	__be16 req_resp;
+};
+
+#endif /* _UAPI_LINUX_VIRTIO_RPMB_H */
-- 
2.55.0

[PATCH v2 2/2] rpmb: add a virtio RPMB device driver

[Haruka Kuze via OP-TEE]

From: Haruka Kuze <harukak@zohomail.eu>

No driver exists for virtio-rpmb devices (virtio ID 28), so OP-TEE
running in a guest can not use them.

This registers the virtio-rpmb devices found with the RPMB sub-system.
Each request is done in one transfer on request queue, and for key
programming and data writing a result read frame is appended so that
the device can return the result.

These frames are copied since the caller's buffers are not always
usable for DMA.

These frames follow the emmc definition, so that the device is
registered as RPMB_TYPE_EMMC. The device has no identifier of own, so
that a fixed dev_id is used for the key derivation for the device.

Based on the virtio-rpmb driver in Alex Bennée's RPMB series.

Link: https://lore.kernel.org/all/20220405093759.1126835-4-alex.bennee@linaro.org/
Signed-off-by: Haruka Kuze <harukak@zohomail.eu>
---
 MAINTAINERS                      |   7 +
 drivers/misc/Kconfig             |  10 ++
 drivers/misc/Makefile            |   1 +
 drivers/misc/virtio_rpmb.c       | 286 +++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_rpmb.h |  56 ++++++
 5 files changed, 360 insertions(+)
 create mode 100644 drivers/misc/virtio_rpmb.c
 create mode 100644 include/uapi/linux/virtio_rpmb.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 748ce4d1c6a2..488003a28534 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29707,6 +29707,13 @@ S:	Maintained
 F:	drivers/char/hw_random/virtio-rng.c
 F:	include/uapi/linux/virtio_rng.h
 
+VIRTIO RPMB DRIVER
+M:	Haruka Kuze <harukak@zohomail.eu>
+L:	virtualization@lists.linux.dev
+S:	Maintained
+F:	drivers/misc/virtio_rpmb.c
+F:	include/uapi/linux/virtio_rpmb.h
+
 VIRTIO RTC DRIVER
 M:	Peter Hilber <peter.hilber@oss.qualcomm.com>
 L:	virtualization@lists.linux.dev
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index c40e89cc5724..eb3518ecc8fe 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -91,6 +91,16 @@ config RPMB
 
 	  If unsure, select N.
 
+config VIRTIO_RPMB
+	tristate "Virtio RPMB driver"
+	depends on VIRTIO && RPMB
+	help
+	  Driver for virtio RPMB devices. The device is registered with the
+	  RPMB subsystem, where in-kernel users such as OP-TEE can use it.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called virtio_rpmb.
+
 config TI_FPC202
 	tristate "TI FPC202 Dual Port Controller"
 	depends on I2C
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7cc3d615d37a..e4390bb8bdc0 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_TIFM_CORE)       	+= tifm_core.o
 obj-$(CONFIG_TIFM_7XX1)       	+= tifm_7xx1.o
 obj-$(CONFIG_PHANTOM)		+= phantom.o
 obj-$(CONFIG_RPMB)		+= rpmb-core.o
+obj-$(CONFIG_VIRTIO_RPMB)	+= virtio_rpmb.o
 obj-$(CONFIG_QCOM_COINCELL)	+= qcom-coincell.o
 obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
 obj-$(CONFIG_SENSORS_BH1770)	+= bh1770glc.o
diff --git a/drivers/misc/virtio_rpmb.c b/drivers/misc/virtio_rpmb.c
new file mode 100644
index 000000000000..4caca378fecc
--- /dev/null
+++ b/drivers/misc/virtio_rpmb.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Virtio RPMB driver
+ *
+ * Copyright (c) 2018-2019 Intel Corporation.
+ * Copyright (c) 2021-2022 Linaro Ltd.
+ *
+ * Based on the virtio-rpmb driver in Alex Bennée's RPMB series.
+ */
+
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/rpmb.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+#include <linux/virtio.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_rpmb.h>
+
+#define VIRTIO_RPMB_FRAME_SIZE	sizeof(struct virtio_rpmb_frame)
+static_assert(VIRTIO_RPMB_FRAME_SIZE == 512);
+
+/*
+ * This device has no identifier on its own.
+ * This value is used as input when OP-TEE is deriving the RPMB
+ * key, so it shall stay stable.
+ */
+static const u8 virtio_rpmb_dev_id[16] = "virtio-rpmb";
+
+/**
+ * struct virtio_rpmb - virtio RPMB device state
+ * @vdev: virtio device
+ * @vq: request queue, NULL while the device is gone or frozen
+ * @lock: serializes requests and protects @vq
+ * @rdev: RPMB device
+ * @max_wr: maximum number of blocks in one write request, 0 if unlimited
+ * @max_rd: maximum number of blocks in one read request, 0 if unlimited
+ */
+struct virtio_rpmb {
+	struct virtio_device *vdev;
+	struct virtqueue *vq;
+	struct mutex lock;
+	struct rpmb_dev *rdev;
+	u8 max_wr;
+	u8 max_rd;
+};
+
+struct virtio_rpmb_req {
+	struct completion done;
+	unsigned int len;
+};
+
+static void virtio_rpmb_done(struct virtqueue *vq)
+{
+	struct virtio_rpmb_req *req;
+	unsigned int len;
+
+	while ((req = virtqueue_get_buf(vq, &len))) {
+		req->len = len;
+		complete(&req->done);
+	}
+}
+
+static int virtio_rpmb_xfer(struct virtio_rpmb *vr, void *out,
+			    unsigned int out_len, void *in, unsigned int in_len)
+{
+	struct scatterlist out_sg, in_sg, *sgs[2] = { &out_sg, &in_sg };
+	struct virtio_rpmb_req req;
+	int ret;
+
+	sg_init_one(&out_sg, out, out_len);
+	sg_init_one(&in_sg, in, in_len);
+	init_completion(&req.done);
+
+	guard(mutex)(&vr->lock);
+	if (!vr->vq)
+		return -ENODEV;
+
+	ret = virtqueue_add_sgs(vr->vq, sgs, 1, 1, &req, GFP_KERNEL);
+	if (ret)
+		return ret;
+
+	virtqueue_kick(vr->vq);
+	wait_for_completion(&req.done);
+
+	return req.len == in_len ? 0 : -EIO;
+}
+
+static int virtio_rpmb_route_frames(struct device *dev, u8 *req,
+				    unsigned int req_len, u8 *resp,
+				    unsigned int resp_len)
+{
+	struct virtio_rpmb *vr = dev_to_virtio(dev)->priv;
+	struct virtio_rpmb_frame *frm = (struct virtio_rpmb_frame *)req;
+	struct virtio_rpmb_frame *result;
+	unsigned int out_len = req_len;
+	bool result_read = false;
+	void *out, *in;
+	int ret;
+
+	if (req_len < VIRTIO_RPMB_FRAME_SIZE)
+		return -EINVAL;
+
+	switch (be16_to_cpu(frm->req_resp)) {
+	case VIRTIO_RPMB_REQ_PROGRAM_KEY:
+		if (req_len != VIRTIO_RPMB_FRAME_SIZE ||
+		    resp_len != VIRTIO_RPMB_FRAME_SIZE)
+			return -EINVAL;
+		result_read = true;
+		break;
+	case VIRTIO_RPMB_REQ_GET_WRITE_COUNTER:
+		if (req_len != VIRTIO_RPMB_FRAME_SIZE ||
+		    resp_len != VIRTIO_RPMB_FRAME_SIZE)
+			return -EINVAL;
+		break;
+	case VIRTIO_RPMB_REQ_DATA_WRITE:
+		if (req_len % VIRTIO_RPMB_FRAME_SIZE ||
+		    resp_len != VIRTIO_RPMB_FRAME_SIZE)
+			return -EINVAL;
+		if (vr->max_wr && req_len / VIRTIO_RPMB_FRAME_SIZE > vr->max_wr)
+			return -EINVAL;
+		result_read = true;
+		break;
+	case VIRTIO_RPMB_REQ_DATA_READ:
+		if (req_len != VIRTIO_RPMB_FRAME_SIZE ||
+		    !resp_len || resp_len % VIRTIO_RPMB_FRAME_SIZE)
+			return -EINVAL;
+		if (vr->max_rd && resp_len / VIRTIO_RPMB_FRAME_SIZE > vr->max_rd)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * The caller's buffers may be unsuitable for DMA, so
+	 * copy the frames.  Writes are followed by result read
+	 * request in the same rpmb xfer, which makes the
+	 * device return the result.
+	 */
+	if (result_read)
+		out_len += VIRTIO_RPMB_FRAME_SIZE;
+
+	out = kmalloc(out_len, GFP_KERNEL);
+	if (!out)
+		return -ENOMEM;
+	in = kzalloc(resp_len, GFP_KERNEL);
+	if (!in) {
+		kfree(out);
+		return -ENOMEM;
+	}
+
+	memcpy(out, req, req_len);
+	if (result_read) {
+		result = out + req_len;
+		memset(result, 0, sizeof(*result));
+		result->block_count = cpu_to_be16(1);
+		result->req_resp = cpu_to_be16(VIRTIO_RPMB_REQ_RESULT_READ);
+	}
+
+	ret = virtio_rpmb_xfer(vr, out, out_len, in, resp_len);
+	if (!ret)
+		memcpy(resp, in, resp_len);
+
+	kfree(in);
+	kfree(out);
+	return ret;
+}
+
+static int virtio_rpmb_init_vq(struct virtio_rpmb *vr)
+{
+	struct virtqueue *vq;
+
+	vq = virtio_find_single_vq(vr->vdev, virtio_rpmb_done, "request");
+	if (IS_ERR(vq))
+		return PTR_ERR(vq);
+
+	virtio_device_ready(vr->vdev);
+
+	scoped_guard(mutex, &vr->lock)
+		vr->vq = vq;
+	return 0;
+}
+
+static void virtio_rpmb_del_vq(struct virtio_rpmb *vr)
+{
+	struct virtio_device *vdev = vr->vdev;
+
+	scoped_guard(mutex, &vr->lock)
+		vr->vq = NULL;
+
+	virtio_reset_device(vdev);
+	vdev->config->del_vqs(vdev);
+}
+
+static int virtio_rpmb_probe(struct virtio_device *vdev)
+{
+	struct rpmb_descr descr = {
+		/* Frames and their semantics are that of eMMC */
+		.type = RPMB_TYPE_EMMC,
+		.route_frames = virtio_rpmb_route_frames,
+		.dev_id = (u8 *)virtio_rpmb_dev_id,
+		.dev_id_len = sizeof(virtio_rpmb_dev_id),
+	};
+	struct virtio_rpmb *vr;
+	u8 capacity;
+	int ret;
+
+	capacity = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
+						capacity));
+	if (!capacity || capacity > 0x80)
+		return dev_err_probe(&vdev->dev, -EINVAL,
+				     "invalid capacity %u\n", capacity);
+
+	vr = devm_kzalloc(&vdev->dev, sizeof(*vr), GFP_KERNEL);
+	if (!vr)
+		return -ENOMEM;
+
+	vdev->priv = vr;
+	vr->vdev = vdev;
+	mutex_init(&vr->lock);
+	vr->max_wr = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
+						  max_wr_cnt));
+	vr->max_rd = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
+						  max_rd_cnt));
+
+	ret = virtio_rpmb_init_vq(vr);
+	if (ret)
+		return ret;
+
+	/*
+	 * eMMC reports the reliable wr count in 512 byte sectors, but the device
+	 * limit is in 256 byte blocks.
+	 */
+	descr.capacity = capacity;
+	descr.reliable_wr_count = (vr->max_wr ?: U8_MAX) / 2;
+
+	vr->rdev = rpmb_dev_register(&vdev->dev, &descr);
+	if (IS_ERR(vr->rdev)) {
+		virtio_rpmb_del_vq(vr);
+		return PTR_ERR(vr->rdev);
+	}
+
+	return 0;
+}
+
+static void virtio_rpmb_remove(struct virtio_device *vdev)
+{
+	struct virtio_rpmb *vr = vdev->priv;
+
+	rpmb_dev_unregister(vr->rdev);
+	virtio_rpmb_del_vq(vr);
+}
+
+static int virtio_rpmb_freeze(struct virtio_device *vdev)
+{
+	virtio_rpmb_del_vq(vdev->priv);
+	return 0;
+}
+
+static int virtio_rpmb_restore(struct virtio_device *vdev)
+{
+	return virtio_rpmb_init_vq(vdev->priv);
+}
+
+static const struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_RPMB, VIRTIO_DEV_ANY_ID },
+	{}
+};
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+static struct virtio_driver virtio_rpmb_driver = {
+	.driver.name	= KBUILD_MODNAME,
+	.id_table	= id_table,
+	.probe		= virtio_rpmb_probe,
+	.remove		= virtio_rpmb_remove,
+	.freeze		= pm_sleep_ptr(virtio_rpmb_freeze),
+	.restore	= pm_sleep_ptr(virtio_rpmb_restore),
+};
+module_virtio_driver(virtio_rpmb_driver);
+
+MODULE_DESCRIPTION("Virtio RPMB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/virtio_rpmb.h b/include/uapi/linux/virtio_rpmb.h
new file mode 100644
index 000000000000..66f45202857a
--- /dev/null
+++ b/include/uapi/linux/virtio_rpmb.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
+/*
+ * Virtio RPMB Device
+ *
+ * The Virtio RPMB Specification:
+ * https://github.com/oasis-tcs/virtio-spec/tree/master/device-types/rpmb
+ */
+
+#ifndef _UAPI_LINUX_VIRTIO_RPMB_H
+#define _UAPI_LINUX_VIRTIO_RPMB_H
+
+#include <linux/types.h>
+
+struct virtio_rpmb_config {
+	__u8 capacity;
+	__u8 max_wr_cnt;
+	__u8 max_rd_cnt;
+};
+
+/* Request types (req_resp) */
+#define VIRTIO_RPMB_REQ_PROGRAM_KEY		0x0001
+#define VIRTIO_RPMB_REQ_GET_WRITE_COUNTER	0x0002
+#define VIRTIO_RPMB_REQ_DATA_WRITE		0x0003
+#define VIRTIO_RPMB_REQ_DATA_READ		0x0004
+#define VIRTIO_RPMB_REQ_RESULT_READ		0x0005
+
+/* Response types (req_resp) */
+#define VIRTIO_RPMB_RESP_PROGRAM_KEY		0x0100
+#define VIRTIO_RPMB_RESP_GET_COUNTER		0x0200
+#define VIRTIO_RPMB_RESP_DATA_WRITE		0x0300
+#define VIRTIO_RPMB_RESP_DATA_READ		0x0400
+
+/* Operation results (result) */
+#define VIRTIO_RPMB_RES_OK			0x0000
+#define VIRTIO_RPMB_RES_GENERAL_FAILURE		0x0001
+#define VIRTIO_RPMB_RES_AUTH_FAILURE		0x0002
+#define VIRTIO_RPMB_RES_COUNT_FAILURE		0x0003
+#define VIRTIO_RPMB_RES_ADDR_FAILURE		0x0004
+#define VIRTIO_RPMB_RES_WRITE_FAILURE		0x0005
+#define VIRTIO_RPMB_RES_READ_FAILURE		0x0006
+#define VIRTIO_RPMB_RES_NO_AUTH_KEY		0x0007
+#define VIRTIO_RPMB_RES_WRITE_COUNTER_EXPIRED	0x0080
+
+struct virtio_rpmb_frame {
+	__u8 stuff[196];
+	__u8 key_mac[32];
+	__u8 data[256];
+	__u8 nonce[16];
+	__be32 write_counter;
+	__be16 address;
+	__be16 block_count;
+	__be16 result;
+	__be16 req_resp;
+};
+
+#endif /* _UAPI_LINUX_VIRTIO_RPMB_H */
-- 
2.55.0

Re: [PATCH v2 1/2] rpmb: stop the routing after unregister

[sashiko-bot]

> From: Haruka Kuze <harukak@zohomail.eu>
> 
> OP-TEE has a reference of RPMB device it used. So, it can route
> frames through the device after rpmb_dev_unregister() is called.
> Therefore, the parent device passed to route_frames() can be
> already freed, for example, mmc_blk_remove_rpmb_part() drops the
> last reference after rpmb unregister.
> 
> This patch clears the route_frames when the device is going to
> be removed by getting new rwsem and makes rpmb_route_frames()
> return -ENODEV if the device is gone.
> 
> Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB) subsystem")
> Assisted-by: LLM
> Signed-off-by: Haruka Kuze <harukak@zohomail.eu>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029124132.2354314-1-harukak@zohomail.eu?part=1

Re: [PATCH v2 2/2] rpmb: add a virtio RPMB device driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rpmb: virtio_rpmb: incorrect reliable_wr_count calculation in virtio_rpmb_probe()
- [High] rpmb: virtio_rpmb: race on virtqueue in virtio_rpmb_done()
- [Medium] virtio_rpmb: avoid duplicate del_vqs() in virtio_rpmb_del_vq() after failed restore
--

commit df2c2241a3933ce84e1976592ab925a107995087
Author: Haruka Kuze <harukak@zohomail.eu>

rpmb: add a virtio RPMB device driver

This commit adds a driver for virtio RPMB devices (virtio ID 28) and
registers them with the RPMB subsystem as RPMB_TYPE_EMMC so in-kernel users
such as OP-TEE can access them.

Link: https://lore.kernel.org/all/20220405093759.1126835-4-alex.bennee@linaro.org/

> diff --git a/drivers/misc/virtio_rpmb.c b/drivers/misc/virtio_rpmb.c
> new file mode 100644
> index 0000000000000..4caca378fecc4
> --- /dev/null
> +++ b/drivers/misc/virtio_rpmb.c

[ ... ]

> +static void virtio_rpmb_done(struct virtqueue *vq)
> +{
> +	struct virtio_rpmb_req *req;
> +	unsigned int len;
> +
> +	while ((req = virtqueue_get_buf(vq, &len))) {
> +		req->len = len;
> +		complete(&req->done);
> +	}
> +}
> +
> +static int virtio_rpmb_xfer(struct virtio_rpmb *vr, void *out,
> +			    unsigned int out_len, void *in, unsigned int in_len)
> +{
> +	struct scatterlist out_sg, in_sg, *sgs[2] = { &out_sg, &in_sg };
> +	struct virtio_rpmb_req req;
> +	int ret;
> +
> +	sg_init_one(&out_sg, out, out_len);
> +	sg_init_one(&in_sg, in, in_len);
> +	init_completion(&req.done);
> +
> +	guard(mutex)(&vr->lock);
> +	if (!vr->vq)
> +		return -ENODEV;
> +
> +	ret = virtqueue_add_sgs(vr->vq, sgs, 1, 1, &req, GFP_KERNEL);
> +	if (ret)
> +		return ret;
> +
> +	virtqueue_kick(vr->vq);
> +	wait_for_completion(&req.done);
> +
> +	return req.len == in_len ? 0 : -EIO;
> +}

[Severity: High]
Can virtio_rpmb_done() race with virtio_rpmb_xfer() on the virtqueue state?

In virtio_rpmb_done(), once complete(&req->done) is called on the first loop
iteration, the task waiting in virtio_rpmb_xfer() can wake up, exit the
function, and release vr->lock while virtio_rpmb_done() is still looping to
call virtqueue_get_buf(vq, &len) again:

CPU0                                    CPU1
virtio_rpmb_done()                      virtio_rpmb_xfer()
  req = virtqueue_get_buf(vq, &len);
  complete(&req->done);
                                          wait_for_completion(&req.done);
                                          /* releases vr->lock */
                                        virtio_rpmb_xfer()
                                          /* acquires vr->lock */
  virtqueue_get_buf(vq, &len);            virtqueue_add_sgs(vr->vq, ...);
                                          virtqueue_kick(vr->vq);

Additionally, because virtqueue_add_sgs() exposes the available descriptor
before returning, a polling host or spurious interrupt can trigger
virtio_rpmb_done() concurrently with virtqueue_add_sgs() or virtqueue_kick().

Since virtio_ring requires callers to synchronize virtqueue_get_buf() against
virtqueue_add_sgs() and virtqueue_kick(), could this corrupt virtqueue state
(such as vq->free_head, vq->vq.num_free, and vq->num_added) or trigger the
START_USE(vq) / END_USE(vq) checks in drivers/virtio/virtio_ring.c when DEBUG
is enabled?

[ ... ]

> +static void virtio_rpmb_del_vq(struct virtio_rpmb *vr)
> +{
> +	struct virtio_device *vdev = vr->vdev;
> +
> +	scoped_guard(mutex, &vr->lock)
> +		vr->vq = NULL;
> +
> +	virtio_reset_device(vdev);
> +	vdev->config->del_vqs(vdev);
> +}

[Severity: Medium]
What happens if virtio_rpmb_restore() fails after virtio_rpmb_freeze() has
already torn down the virtqueue?

During suspend, virtio_rpmb_freeze() calls virtio_rpmb_del_vq(), which sets
vr->vq = NULL, resets the device, and calls vdev->config->del_vqs(vdev).

If virtio_rpmb_restore() -> virtio_rpmb_init_vq() -> virtio_find_single_vq()
later fails during resume, vr->vq remains NULL while the driver stays bound
to the device. When virtio_rpmb_remove() or a subsequent virtio_rpmb_freeze()
runs, virtio_rpmb_del_vq() will unconditionally call
vdev->config->del_vqs(vdev) a second time without a matching find_vqs(),
which on transports such as virtio_mmio calls free_irq() on an already-freed
interrupt.

Should virtio_rpmb_del_vq() return early if vr->vq is already NULL?

> +
> +static int virtio_rpmb_probe(struct virtio_device *vdev)
> +{
> +	struct rpmb_descr descr = {
> +		/* Frames and their semantics are that of eMMC */
> +		.type = RPMB_TYPE_EMMC,
> +		.route_frames = virtio_rpmb_route_frames,
> +		.dev_id = (u8 *)virtio_rpmb_dev_id,
> +		.dev_id_len = sizeof(virtio_rpmb_dev_id),
> +	};
> +	struct virtio_rpmb *vr;
> +	u8 capacity;
> +	int ret;
> +
> +	capacity = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
> +						capacity));
> +	if (!capacity || capacity > 0x80)
> +		return dev_err_probe(&vdev->dev, -EINVAL,
> +				     "invalid capacity %u\n", capacity);
> +
> +	vr = devm_kzalloc(&vdev->dev, sizeof(*vr), GFP_KERNEL);
> +	if (!vr)
> +		return -ENOMEM;
> +
> +	vdev->priv = vr;
> +	vr->vdev = vdev;
> +	mutex_init(&vr->lock);
> +	vr->max_wr = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
> +						  max_wr_cnt));
> +	vr->max_rd = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
> +						  max_rd_cnt));
> +
> +	ret = virtio_rpmb_init_vq(vr);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * eMMC reports the reliable wr count in 512 byte sectors, but the device
> +	 * limit is in 256 byte blocks.
> +	 */
> +	descr.capacity = capacity;
> +	descr.reliable_wr_count = (vr->max_wr ?: U8_MAX) / 2;

[Severity: High]
Should descr.reliable_wr_count be set without dividing vr->max_wr by 2 here?

Each 256-byte RPMB data block is carried inside a single 512-byte
virtio_rpmb_frame (one 512-byte eMMC sector per 256-byte RPMB data payload).
In virtio_rpmb_route_frames(), vr->max_wr is checked directly against the
number of 512-byte frames:

drivers/misc/virtio_rpmb.c:virtio_rpmb_route_frames() {
	...
	case VIRTIO_RPMB_REQ_DATA_WRITE:
		if (req_len % VIRTIO_RPMB_FRAME_SIZE ||
		    resp_len != VIRTIO_RPMB_FRAME_SIZE)
			return -EINVAL;
		if (vr->max_wr && req_len / VIRTIO_RPMB_FRAME_SIZE > vr->max_wr)
			return -EINVAL;
	...
}

In the RPMB subsystem, descr.reliable_wr_count is also the number of RPMB
frames (256-byte data blocks) per write request, matching mmc_blk_rpmb_add()
in drivers/mmc/core/block.c:

drivers/mmc/core/block.c:mmc_blk_rpmb_add() {
	...
	struct rpmb_descr descr = {
		.type = RPMB_TYPE_EMMC,
		.route_frames = mmc_route_rpmb_frames,
		.reliable_wr_count = card->ext_csd.enhanced_rpmb_supported ?
				     2 : 32,
	...
}

Dividing by 2 in virtio_rpmb_probe() halves the advertised write count, and
when a device reports max_wr_cnt == 1, integer division truncates
descr.reliable_wr_count to 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029124132.2354314-1-harukak@zohomail.eu?part=2