[RFC, PATCH 0/1] Replay Protected Memory Block (RPMB) driver

[RFC, PATCH 0/1] Replay Protected Memory Block (RPMB) driver

[Shyam Saini]

Hi everyone,

This is yet another attempt to come up with an RPMB API for the kernel.
This patch is based on patch 1 of last submission except few minor changes.
The last discussion of this was in the thread:

  Subject: [PATCH  v2 1/4] rpmb: add Replay Protected Memory Block (RPMB) subsystem
  Date: Tue,  5 Apr 2022 10:37:56 +0100 [thread overview]
  Message-ID: <20220405093759.1126835-2-alex.bennee@linaro.org>

The patch provides a simple RPMB driver. This is a RFC version and this
single driver can't be used by its own. It would require further work to
make use of API's provided by this driver.

Changes since the last posting:
  drop RPMB char driver
  drop virtio rpmb frontend driver
  drop rpmb: add RPBM access tool
  Rename get_write_count to get_write_counter
  Make return type for rpmb_set_key() function explicit

Alex Bennée (1):
  rpmb: add Replay Protected Memory Block (RPMB) driver

 MAINTAINERS           |   7 +
 drivers/Kconfig       |   1 +
 drivers/Makefile      |   2 +
 drivers/rpmb/Kconfig  |  11 ++
 drivers/rpmb/Makefile |   7 +
 drivers/rpmb/core.c   | 439 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/rpmb.h  | 182 +++++++++++++++++
 7 files changed, 649 insertions(+)
 create mode 100644 drivers/rpmb/Kconfig
 create mode 100644 drivers/rpmb/Makefile
 create mode 100644 drivers/rpmb/core.c
 create mode 100644 include/linux/rpmb.h

-- 
2.34.1

[RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Shyam Saini]

From: Alex Bennée <alex.bennee@linaro.org>

[This is patch 1 from [1] Alex's submission and this RPMB layer was
originally proposed by [2]Thomas Winkler ]

A number of storage technologies support a specialised hardware
partition designed to be resistant to replay attacks. The underlying
HW protocols differ but the operations are common. The RPMB partition
cannot be accessed via standard block layer, but by a set of specific
commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
partition provides authenticated and replay protected access, hence
suitable as a secure storage.

The initial aim of this patch is to provide a simple RPMB Driver which
can be accessed by Linux's optee driver to facilitate fast-path for
RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
Optee OS relies on user-tee supplicant to access eMMC RPMB partition.

A TEE device driver can claim the RPMB interface, for example, via
class_interface_register(). The RPMB driver provides a series of
operations for interacting with the device.

  * program_key - a one time operation for setting up a new device
  * get_capacity - introspect the device capacity
  * get_write_counter - check the write counter
  * write_blocks - write a series of blocks to the RPMB device
  * read_blocks - read a series of blocks from the RPMB device

The detailed operation of implementing the access is left to the TEE
device driver itself.

The framing details and HW specific bits (JDEC vs NVME frames) are
left to the lower level TEE driver to worry about.

Without kernel fast path to RPMB access doesn't work when IMA try to
extend ftpm's PCR registers.

This fast-path would require additional work in linux optee driver and
as well as in MMC driver.

[1] https://lore.kernel.org/lkml/20220405093759.1126835-2-alex.bennee@linaro.org/
[2] https://lore.kernel.org/linux-mmc/1478548394-8184-2-git-send-email-tomas.winkler@intel.com/
[3] https://optee.readthedocs.io/en/latest/architecture/secure_storage.html

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>

Cc: Tomas Winkler <tomas.winkler@intel.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Linus  Walleij <linus.walleij@linaro.org>
Cc: Arnd Bergmann <arnd.bergmann@linaro.org>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Sumit Garg <sumit.garg@linaro.org>
Cc: Tyler Hicks <code@tyhicks.com>
Cc: Srivatsa S. Bhat (Microsoft) <srivatsa@csail.mit.edu>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Allen Pais <apais@linux.microsoft.com>
---
 MAINTAINERS           |   7 +
 drivers/Kconfig       |   1 +
 drivers/Makefile      |   2 +
 drivers/rpmb/Kconfig  |  11 ++
 drivers/rpmb/Makefile |   7 +
 drivers/rpmb/core.c   | 439 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/rpmb.h  | 182 +++++++++++++++++
 7 files changed, 649 insertions(+)
 create mode 100644 drivers/rpmb/Kconfig
 create mode 100644 drivers/rpmb/Makefile
 create mode 100644 drivers/rpmb/core.c
 create mode 100644 include/linux/rpmb.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 139106909db6..52d2020ba1d8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18480,6 +18480,13 @@ T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-de2-rotate.yaml
 F:	drivers/media/platform/sunxi/sun8i-rotate/
 
+RPMB SUBSYSTEM
+M:	?
+L:	linux-kernel@vger.kernel.org
+S:	Supported
+F:	drivers/rpmb/*
+F:	include/linux/rpmb.h
+
 RPMSG TTY DRIVER
 M:	Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
 L:	linux-remoteproc@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 514ae6b24cb2..9db924f85e07 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -242,5 +242,6 @@ source "drivers/peci/Kconfig"
 source "drivers/hte/Kconfig"
 
 source "drivers/cdx/Kconfig"
+source "drivers/rpmb/Kconfig"
 
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 7241d80a7b29..f8f8d9212567 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -195,3 +195,5 @@ obj-$(CONFIG_PECI)		+= peci/
 obj-$(CONFIG_HTE)		+= hte/
 obj-$(CONFIG_DRM_ACCEL)		+= accel/
 obj-$(CONFIG_CDX_BUS)		+= cdx/
+obj-$(CONFIG_RPMB)             += rpmb/
+
diff --git a/drivers/rpmb/Kconfig b/drivers/rpmb/Kconfig
new file mode 100644
index 000000000000..f2a9ebdc4435
--- /dev/null
+++ b/drivers/rpmb/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2015-2019, Intel Corporation.
+
+config RPMB
+	tristate "RPMB partition interface"
+	help
+	  Unified RPMB partition interface for RPMB capable devices such as
+          eMMC and UFS. Provides interface for in kernel security controllers to
+	  access RPMB partition.
+
+	  If unsure, select N.
diff --git a/drivers/rpmb/Makefile b/drivers/rpmb/Makefile
new file mode 100644
index 000000000000..24d4752a9a53
--- /dev/null
+++ b/drivers/rpmb/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2015-2019, Intel Corporation.
+
+obj-$(CONFIG_RPMB) += rpmb.o
+rpmb-objs += core.o
+
+ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/rpmb/core.c b/drivers/rpmb/core.c
new file mode 100644
index 000000000000..5bb7dbaa5464
--- /dev/null
+++ b/drivers/rpmb/core.c
@@ -0,0 +1,439 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2015 - 2019 Intel Corporation. All rights reserved.
+ * Copyright(c) 2021 - 2022 Linaro Ltd.
+ */
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+
+#include <linux/rpmb.h>
+#include "rpmb-cdev.h"
+
+static DEFINE_IDA(rpmb_ida);
+
+/**
+ * rpmb_dev_get() - increase rpmb device ref counter
+ * @rdev: rpmb device
+ */
+struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev)
+{
+	return get_device(&rdev->dev) ? rdev : NULL;
+}
+EXPORT_SYMBOL_GPL(rpmb_dev_get);
+
+/**
+ * rpmb_dev_put() - decrease rpmb device ref counter
+ * @rdev: rpmb device
+ */
+void rpmb_dev_put(struct rpmb_dev *rdev)
+{
+	put_device(&rdev->dev);
+}
+EXPORT_SYMBOL_GPL(rpmb_dev_put);
+
+/**
+ * rpmb_program_key() - program the RPMB access key
+ * @rdev: rpmb device
+ * @keylen: length of key data
+ * @key: key data
+ *
+ * A successful programming of the key implies it has been set by the
+ * driver and can be used.
+ *
+ * Return:
+ * *        0 on success
+ * *        -EINVAL on wrong parameters
+ * *        -EPERM key already programmed
+ * *        -EOPNOTSUPP if device doesn't support the requested operation
+ * *        < 0 if the operation fails
+ */
+int rpmb_program_key(struct rpmb_dev *rdev, int klen, u8 *key, int rlen, u8 *resp)
+{
+	int err;
+
+	if (!rdev || !key)
+		return -EINVAL;
+
+	mutex_lock(&rdev->lock);
+	err = -EOPNOTSUPP;
+	if (rdev->ops && rdev->ops->program_key) {
+		err = rdev->ops->program_key(rdev->dev.parent, rdev->target,
+					     klen, key, rlen, resp);
+	}
+	mutex_unlock(&rdev->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(rpmb_program_key);
+
+/**
+ * rpmb_get_capacity() - returns the capacity of the rpmb device
+ * @rdev: rpmb device
+ *
+ * Return:
+ * *        capacity of the device in units of 128K, on success
+ * *        -EINVAL on wrong parameters
+ * *        -EOPNOTSUPP if device doesn't support the requested operation
+ * *        < 0 if the operation fails
+ */
+int rpmb_get_capacity(struct rpmb_dev *rdev)
+{
+	int err;
+
+	if (!rdev)
+		return -EINVAL;
+
+	mutex_lock(&rdev->lock);
+	err = -EOPNOTSUPP;
+	if (rdev->ops && rdev->ops->get_capacity)
+		err = rdev->ops->get_capacity(rdev->dev.parent, rdev->target);
+	mutex_unlock(&rdev->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(rpmb_get_capacity);
+
+/**
+ * rpmb_get_write_counter() - returns the write counter of the rpmb device
+ * @rdev: rpmb device
+ * @len: size of request frame
+ * @request: request frame
+ * @rlen: size of response frame
+ * @resp: response frame
+ *
+ * Return:
+ * *        counter
+ * *        -EINVAL on wrong parameters
+ * *        -EOPNOTSUPP if device doesn't support the requested operation
+ * *        < 0 if the operation fails
+ */
+int rpmb_get_write_counter(struct rpmb_dev *rdev, int len, u8 *request, int rlen, u8 *resp)
+{
+	int err;
+
+	if (!rdev)
+		return -EINVAL;
+
+	mutex_lock(&rdev->lock);
+	err = -EOPNOTSUPP;
+	if (rdev->ops && rdev->ops->get_write_counter)
+		err = rdev->ops->get_write_counter(rdev->dev.parent, rdev->target,
+						 len, request, rlen, resp);
+	mutex_unlock(&rdev->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(rpmb_get_write_counter);
+
+/**
+ * rpmb_write_blocks() - write data to RPMB device
+ * @rdev: rpmb device
+ * @addr: block address (index of first block - 256B blocks)
+ * @count: number of 256B blosks
+ * @data: pointer to data to program
+ *
+ * Write a series of blocks to the RPMB device.
+ *
+ * Return:
+ * *        0 on success
+ * *        -EINVAL on wrong parameters
+ * *        -EACCESS no key set
+ * *        -EOPNOTSUPP if device doesn't support the requested operation
+ * *        < 0 if the operation fails
+ */
+int rpmb_write_blocks(struct rpmb_dev *rdev, int len, u8 *request,
+		      int rlen, u8 *response)
+{
+	int err;
+
+	if (!rdev || !len || !request)
+		return -EINVAL;
+
+	mutex_lock(&rdev->lock);
+	err = -EOPNOTSUPP;
+	if (rdev->ops && rdev->ops->write_blocks) {
+		err = rdev->ops->write_blocks(rdev->dev.parent, rdev->target,
+					      len, request, rlen, response);
+	}
+	mutex_unlock(&rdev->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(rpmb_write_blocks);
+
+/**
+ * rpmb_read_blocks() - read data from RPMB device
+ * @rdev: rpmb device
+ * @addr: block address (index of first block - 256B blocks)
+ * @count: number of 256B blocks
+ * @data: pointer to data to read
+ *
+ * Read a series of one or more blocks from the RPMB device.
+ *
+ * Return:
+ * *        0 on success
+ * *        -EINVAL on wrong parameters
+ * *        -EACCESS no key set
+ * *        -EOPNOTSUPP if device doesn't support the requested operation
+ * *        < 0 if the operation fails
+ */
+int rpmb_read_blocks(struct rpmb_dev *rdev, int addr, int count, int len, u8 *data)
+{
+	int err;
+
+	if (!rdev || !count || !data)
+		return -EINVAL;
+
+	mutex_lock(&rdev->lock);
+	err = -EOPNOTSUPP;
+	if (rdev->ops && rdev->ops->read_blocks) {
+		err = rdev->ops->read_blocks(rdev->dev.parent, rdev->target,
+					     addr, count, len, data);
+	}
+	mutex_unlock(&rdev->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(rpmb_read_blocks);
+
+
+static void rpmb_dev_release(struct device *dev)
+{
+	struct rpmb_dev *rdev = to_rpmb_dev(dev);
+
+	ida_simple_remove(&rpmb_ida, rdev->id);
+	kfree(rdev);
+}
+
+struct class rpmb_class = {
+	.name = "rpmb",
+	.owner = THIS_MODULE,
+	.dev_release = rpmb_dev_release,
+};
+EXPORT_SYMBOL(rpmb_class);
+
+/**
+ * rpmb_dev_find_device() - return first matching rpmb device
+ * @data: data for the match function
+ * @match: the matching function
+ *
+ * Return: matching rpmb device or NULL on failure
+ */
+static
+struct rpmb_dev *rpmb_dev_find_device(const void *data,
+				      int (*match)(struct device *dev,
+						   const void *data))
+{
+	struct device *dev;
+
+	dev = class_find_device(&rpmb_class, NULL, data, match);
+
+	return dev ? to_rpmb_dev(dev) : NULL;
+}
+
+struct device_with_target {
+	const struct device *dev;
+	u8 target;
+};
+
+static int match_by_parent(struct device *dev, const void *data)
+{
+	const struct device_with_target *d = data;
+	struct rpmb_dev *rdev = to_rpmb_dev(dev);
+
+	return (d->dev && dev->parent == d->dev && rdev->target == d->target);
+}
+
+/**
+ * rpmb_dev_find_by_device() - retrieve rpmb device from the parent device
+ * @parent: parent device of the rpmb device
+ * @target: RPMB target/region within the physical device
+ *
+ * Return: NULL if there is no rpmb device associated with the parent device
+ */
+struct rpmb_dev *rpmb_dev_find_by_device(struct device *parent, u8 target)
+{
+	struct device_with_target t;
+
+	if (!parent)
+		return NULL;
+
+	t.dev = parent;
+	t.target = target;
+
+	return rpmb_dev_find_device(&t, match_by_parent);
+}
+EXPORT_SYMBOL_GPL(rpmb_dev_find_by_device);
+
+/**
+ * rpmb_dev_unregister() - unregister RPMB partition from the RPMB subsystem
+ * @rdev: the rpmb device to unregister
+ * Return:
+ * *        0 on success
+ * *        -EINVAL on wrong parameters
+ */
+int rpmb_dev_unregister(struct rpmb_dev *rdev)
+{
+	if (!rdev)
+		return -EINVAL;
+
+	mutex_lock(&rdev->lock);
+	rpmb_cdev_del(rdev);
+	device_del(&rdev->dev);
+	mutex_unlock(&rdev->lock);
+
+	rpmb_dev_put(rdev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(rpmb_dev_unregister);
+
+/**
+ * rpmb_dev_unregister_by_device() - unregister RPMB partition
+ *     from the RPMB subsystem
+ * @dev: the parent device of the rpmb device
+ * @target: RPMB target/region within the physical device
+ * Return:
+ * *        0 on success
+ * *        -EINVAL on wrong parameters
+ * *        -ENODEV if a device cannot be find.
+ */
+int rpmb_dev_unregister_by_device(struct device *dev, u8 target)
+{
+	struct rpmb_dev *rdev;
+
+	if (!dev)
+		return -EINVAL;
+
+	rdev = rpmb_dev_find_by_device(dev, target);
+	if (!rdev) {
+		dev_warn(dev, "no disk found %s\n", dev_name(dev->parent));
+		return -ENODEV;
+	}
+
+	rpmb_dev_put(rdev);
+
+	return rpmb_dev_unregister(rdev);
+}
+EXPORT_SYMBOL_GPL(rpmb_dev_unregister_by_device);
+
+/**
+ * rpmb_dev_get_drvdata() - driver data getter
+ * @rdev: rpmb device
+ *
+ * Return: driver private data
+ */
+void *rpmb_dev_get_drvdata(const struct rpmb_dev *rdev)
+{
+	return dev_get_drvdata(&rdev->dev);
+}
+EXPORT_SYMBOL_GPL(rpmb_dev_get_drvdata);
+
+/**
+ * rpmb_dev_set_drvdata() - driver data setter
+ * @rdev: rpmb device
+ * @data: data to store
+ */
+void rpmb_dev_set_drvdata(struct rpmb_dev *rdev, void *data)
+{
+	dev_set_drvdata(&rdev->dev, data);
+}
+EXPORT_SYMBOL_GPL(rpmb_dev_set_drvdata);
+
+/**
+ * rpmb_dev_register - register RPMB partition with the RPMB subsystem
+ * @dev: storage device of the rpmb device
+ * @target: RPMB target/region within the physical device
+ * @ops: device specific operations
+ *
+ * Return: a pointer to rpmb device
+ */
+struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target,
+				   const struct rpmb_ops *ops)
+{
+	struct rpmb_dev *rdev;
+	int id;
+	int ret;
+
+	if (!dev || !ops)
+		return ERR_PTR(-EINVAL);
+
+	if (!ops->program_key)
+		return ERR_PTR(-EINVAL);
+
+	if (!ops->get_capacity)
+		return ERR_PTR(-EINVAL);
+
+	if (!ops->get_write_counter)
+		return ERR_PTR(-EINVAL);
+
+	if (!ops->write_blocks)
+		return ERR_PTR(-EINVAL);
+
+	if (!ops->read_blocks)
+		return ERR_PTR(-EINVAL);
+
+	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
+	if (!rdev)
+		return ERR_PTR(-ENOMEM);
+
+	id = ida_simple_get(&rpmb_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		ret = id;
+		goto exit;
+	}
+
+	mutex_init(&rdev->lock);
+	rdev->ops = ops;
+	rdev->id = id;
+	rdev->target = target;
+
+	dev_set_name(&rdev->dev, "rpmb%d", id);
+	rdev->dev.class = &rpmb_class;
+	rdev->dev.parent = dev;
+
+	rpmb_cdev_prepare(rdev);
+
+	ret = device_register(&rdev->dev);
+	if (ret)
+		goto exit;
+
+	rpmb_cdev_add(rdev);
+
+	dev_dbg(&rdev->dev, "registered device\n");
+
+	return rdev;
+
+exit:
+	if (id >= 0)
+		ida_simple_remove(&rpmb_ida, id);
+	kfree(rdev);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(rpmb_dev_register);
+
+static int __init rpmb_init(void)
+{
+	ida_init(&rpmb_ida);
+	class_register(&rpmb_class);
+	return rpmb_cdev_init();
+}
+
+static void __exit rpmb_exit(void)
+{
+	rpmb_cdev_exit();
+	class_unregister(&rpmb_class);
+	ida_destroy(&rpmb_ida);
+}
+
+subsys_initcall(rpmb_init);
+module_exit(rpmb_exit);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("RPMB class");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
new file mode 100644
index 000000000000..0c9749cf064d
--- /dev/null
+++ b/include/linux/rpmb.h
@@ -0,0 +1,182 @@
+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
+/*
+ * Copyright (C) 2015-2019 Intel Corp. All rights reserved
+ * Copyright (C) 2021-2022 Linaro Ltd
+ */
+#ifndef __RPMB_H__
+#define __RPMB_H__
+
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/cdev.h>
+#include <uapi/linux/rpmb.h>
+#include <linux/kref.h>
+
+#define RPMB_API_VERSION 0x80000001
+
+/**
+ * struct rpmb_ops - RPMB ops to be implemented by underlying block device
+ *
+ * @program_key    : program device key (once only op).
+ * @get_capacity   : rpmb size in 128K units in for region/target.
+ * @get_write_count: return the device write counter
+ * @write_blocks   : write blocks to RPMB device
+ * @read_blocks    : read blocks from RPMB device
+ * @block_size     : block size in half sectors (1 == 256B)
+ * @wr_cnt_max     : maximal number of blocks that can be
+ *                   written in one access.
+ * @rd_cnt_max     : maximal number of blocks that can be
+ *                   read in one access.
+ * @dev_id         : unique device identifier
+ * @dev_id_len     : unique device identifier length
+ */
+struct rpmb_ops {
+	int (*program_key)(struct device *dev, u8 target,
+			   int keylen, u8 *key_frame,
+			   int rlen, u8 *resp);
+	int (*get_capacity)(struct device *dev, u8 target);
+	int (*get_write_count)(struct device *dev, u8 target,
+			       int len, u8 *requests,
+			       int rlen, u8 *resp);
+	int (*write_blocks)(struct device *dev, u8 target,
+			    int len, u8 *requests,
+			    int rlen, u8 *resp);
+	int (*read_blocks)(struct device *dev, u8 target,
+			   int addr, int count,
+			   int len, u8 *data);
+	u16 block_size;
+	u16 wr_cnt_max;
+	u16 rd_cnt_max;
+	const u8 *dev_id;
+	size_t dev_id_len;
+};
+
+/**
+ * struct rpmb_dev - device which can support RPMB partition
+ *
+ * @lock       : the device lock
+ * @dev        : device
+ * @id         : device id
+ * @target     : RPMB target/region within the physical device
+ * @cdev       : character dev
+ * @status     : device status
+ * @ops        : operation exported by rpmb
+ */
+struct rpmb_dev {
+	struct mutex lock; /* device serialization lock */
+	struct device dev;
+	int id;
+	u8 target;
+#ifdef CONFIG_RPMB_INTF_DEV
+	struct cdev cdev;
+	unsigned long status;
+#endif /* CONFIG_RPMB_INTF_DEV */
+	const struct rpmb_ops *ops;
+};
+
+#define to_rpmb_dev(x) container_of((x), struct rpmb_dev, dev)
+
+#if IS_ENABLED(CONFIG_RPMB)
+struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev);
+void rpmb_dev_put(struct rpmb_dev *rdev);
+struct rpmb_dev *rpmb_dev_find_by_device(struct device *parent, u8 target);
+struct rpmb_dev *rpmb_dev_get_by_type(u32 type);
+struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target,
+				   const struct rpmb_ops *ops);
+void *rpmb_dev_get_drvdata(const struct rpmb_dev *rdev);
+void rpmb_dev_set_drvdata(struct rpmb_dev *rdev, void *data);
+int rpmb_dev_unregister(struct rpmb_dev *rdev);
+int rpmb_dev_unregister_by_device(struct device *dev, u8 target);
+
+int rpmb_program_key(struct rpmb_dev *rdev,
+		     int klen, u8 *key, int rlen, u8 *resp);
+int rpmb_get_capacity(struct rpmb_dev *rdev);
+int rpmb_get_write_counter(struct rpmb_dev *rdev,
+			 int len, u8 *request, int rlen, u8 *resp);
+int rpmb_write_blocks(struct rpmb_dev *rdev,
+		      int len, u8 *request, int rlen, u8 *resp);
+int rpmb_read_blocks(struct rpmb_dev *rdev, int addr, int count, int len, u8 *data);
+
+#else
+static inline struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev)
+{
+	return NULL;
+}
+
+static inline void rpmb_dev_put(struct rpmb_dev *rdev) { }
+
+static inline struct rpmb_dev *rpmb_dev_find_by_device(struct device *parent,
+						       u8 target)
+{
+	return NULL;
+}
+
+static inline
+struct rpmb_dev *rpmb_dev_get_by_type(enum rpmb_type type)
+{
+	return NULL;
+}
+
+static inline void *rpmb_dev_get_drvdata(const struct rpmb_dev *rdev)
+{
+	return NULL;
+}
+
+static inline void rpmb_dev_set_drvdata(struct rpmb_dev *rdev, void *data)
+{
+}
+
+static inline struct rpmb_dev *
+rpmb_dev_register(struct device *dev, u8 target, const struct rpmb_ops *ops)
+{
+	return NULL;
+}
+
+static inline int rpmb_dev_unregister(struct rpmb_dev *dev)
+{
+	return 0;
+}
+
+static inline int rpmb_dev_unregister_by_device(struct device *dev, u8 target)
+{
+	return 0;
+}
+
+static inline int rpmb_program_key(struct rpmb_dev *rdev,
+				   int klen, u8 *key,
+				   int rlen, u8 *resp)
+{
+	return 0;
+}
+
+static inline int rpmb_set_key(struct rpmb_dev *rdev, u8 *key, int keylen);
+{
+	return 0;
+}
+
+static inline int rpmb_get_capacity(struct rpmb_dev *rdev)
+{
+	return 0;
+}
+
+static inline int rpmb_get_write_counter(struct rpmb_dev *rdev,
+				       int len, u8 *request, int rlen, u8 *resp)
+{
+	return 0;
+}
+
+static inline int rpmb_write_blocks(struct rpmb_dev *rdev,
+				    int len, u8 *request, int rlen, u8 *resp);
+{
+	return 0;
+}
+
+static inline int rpmb_read_blocks(struct rpmb_dev *rdev, int addr, int count,
+				   int len, u8 *data)
+{
+	return 0;
+}
+
+#endif /* CONFIG_RPMB */
+
+#endif /* __RPMB_H__ */
-- 
2.34.1

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Randy Dunlap]

Hi--

On 7/21/23 18:40, Shyam Saini wrote:
> diff --git a/drivers/rpmb/Kconfig b/drivers/rpmb/Kconfig
> new file mode 100644
> index 000000000000..f2a9ebdc4435
> --- /dev/null
> +++ b/drivers/rpmb/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2015-2019, Intel Corporation.
> +
> +config RPMB
> +	tristate "RPMB partition interface"
> +	help
> +	  Unified RPMB partition interface for RPMB capable devices such as
> +          eMMC and UFS. Provides interface for in kernel security controllers to

Indent the line above with one tab + 2 spaces instead of many spaces.

> +	  access RPMB partition.
> +
> +	  If unsure, select N.

Do we want a separate subdir in drivers/ for various "misc" drivers?
with only one driver in it?

Will there be other drivers in rpmb?

Thanks.
-- 
~Randy

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Shyam Saini]

Hi Randy,

Thank you for the reviews.

> Hi--
>
> On 7/21/23 18:40, Shyam Saini wrote:
>> diff --git a/drivers/rpmb/Kconfig b/drivers/rpmb/Kconfig
>> new file mode 100644
>> index 000000000000..f2a9ebdc4435
>> --- /dev/null
>> +++ b/drivers/rpmb/Kconfig
>> @@ -0,0 +1,11 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2015-2019, Intel Corporation.
>> +
>> +config RPMB
>> +	tristate "RPMB partition interface"
>> +	help
>> +	  Unified RPMB partition interface for RPMB capable devices such as
>> +          eMMC and UFS. Provides interface for in kernel security controllers to
>
> Indent the line above with one tab + 2 spaces instead of many spaces.
>> +	  access RPMB partition.
>> +
>> +	  If unsure, select N.
>
> Do we want a separate subdir in drivers/ for various "misc" drivers?
> with only one driver in it?
>
> Will there be other drivers in rpmb?
>

There may be others as the usage of rpmb driver grow over time but for 
now, this is the only  driver.
Should i keep it like this ?[A

Thanks,
Shyam

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Ulf Hansson]

On Sat, 22 Jul 2023 at 03:41, Shyam Saini
<shyamsaini@linux.microsoft.com> wrote:
>
> From: Alex Bennée <alex.bennee@linaro.org>
>
> [This is patch 1 from [1] Alex's submission and this RPMB layer was
> originally proposed by [2]Thomas Winkler ]
>
> A number of storage technologies support a specialised hardware
> partition designed to be resistant to replay attacks. The underlying
> HW protocols differ but the operations are common. The RPMB partition
> cannot be accessed via standard block layer, but by a set of specific
> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
> partition provides authenticated and replay protected access, hence
> suitable as a secure storage.
>
> The initial aim of this patch is to provide a simple RPMB Driver which
> can be accessed by Linux's optee driver to facilitate fast-path for
> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
>
> A TEE device driver can claim the RPMB interface, for example, via
> class_interface_register(). The RPMB driver provides a series of
> operations for interacting with the device.

I don't quite follow this. More exactly, how will the TEE driver know
what RPMB device it should use?

>
>   * program_key - a one time operation for setting up a new device
>   * get_capacity - introspect the device capacity
>   * get_write_counter - check the write counter
>   * write_blocks - write a series of blocks to the RPMB device
>   * read_blocks - read a series of blocks from the RPMB device
>
> The detailed operation of implementing the access is left to the TEE
> device driver itself.
>
> The framing details and HW specific bits (JDEC vs NVME frames) are
> left to the lower level TEE driver to worry about.
>
> Without kernel fast path to RPMB access doesn't work when IMA try to
> extend ftpm's PCR registers.
>
> This fast-path would require additional work in linux optee driver and
> as well as in MMC driver.
>
> [1] https://lore.kernel.org/lkml/20220405093759.1126835-2-alex.bennee@linaro.org/
> [2] https://lore.kernel.org/linux-mmc/1478548394-8184-2-git-send-email-tomas.winkler@intel.com/
> [3] https://optee.readthedocs.io/en/latest/architecture/secure_storage.html
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
>

[...]

> +/**
> + * rpmb_dev_find_device() - return first matching rpmb device
> + * @data: data for the match function
> + * @match: the matching function
> + *
> + * Return: matching rpmb device or NULL on failure
> + */
> +static
> +struct rpmb_dev *rpmb_dev_find_device(const void *data,
> +                                     int (*match)(struct device *dev,
> +                                                  const void *data))
> +{
> +       struct device *dev;
> +
> +       dev = class_find_device(&rpmb_class, NULL, data, match);
> +
> +       return dev ? to_rpmb_dev(dev) : NULL;
> +}
> +
> +struct device_with_target {
> +       const struct device *dev;
> +       u8 target;
> +};
> +
> +static int match_by_parent(struct device *dev, const void *data)
> +{
> +       const struct device_with_target *d = data;
> +       struct rpmb_dev *rdev = to_rpmb_dev(dev);
> +
> +       return (d->dev && dev->parent == d->dev && rdev->target == d->target);
> +}
> +
> +/**
> + * rpmb_dev_find_by_device() - retrieve rpmb device from the parent device
> + * @parent: parent device of the rpmb device
> + * @target: RPMB target/region within the physical device
> + *
> + * Return: NULL if there is no rpmb device associated with the parent device
> + */
> +struct rpmb_dev *rpmb_dev_find_by_device(struct device *parent, u8 target)
> +{
> +       struct device_with_target t;
> +
> +       if (!parent)
> +               return NULL;
> +
> +       t.dev = parent;
> +       t.target = target;
> +
> +       return rpmb_dev_find_device(&t, match_by_parent);
> +}
> +EXPORT_SYMBOL_GPL(rpmb_dev_find_by_device);

Is this what the TEE driver would be calling to find the rpmb device/partition?

> +
> +/**
> + * rpmb_dev_unregister() - unregister RPMB partition from the RPMB subsystem
> + * @rdev: the rpmb device to unregister
> + * Return:
> + * *        0 on success
> + * *        -EINVAL on wrong parameters
> + */
> +int rpmb_dev_unregister(struct rpmb_dev *rdev)
> +{
> +       if (!rdev)
> +               return -EINVAL;
> +
> +       mutex_lock(&rdev->lock);
> +       rpmb_cdev_del(rdev);

I can't find the function above. I guess it should be included as a
part of the patch too?

> +       device_del(&rdev->dev);
> +       mutex_unlock(&rdev->lock);
> +
> +       rpmb_dev_put(rdev);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(rpmb_dev_unregister);

[...]

> +/**
> + * rpmb_dev_register - register RPMB partition with the RPMB subsystem
> + * @dev: storage device of the rpmb device
> + * @target: RPMB target/region within the physical device
> + * @ops: device specific operations
> + *
> + * Return: a pointer to rpmb device
> + */
> +struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target,
> +                                  const struct rpmb_ops *ops)
> +{
> +       struct rpmb_dev *rdev;
> +       int id;
> +       int ret;
> +
> +       if (!dev || !ops)
> +               return ERR_PTR(-EINVAL);
> +
> +       if (!ops->program_key)
> +               return ERR_PTR(-EINVAL);
> +
> +       if (!ops->get_capacity)
> +               return ERR_PTR(-EINVAL);
> +
> +       if (!ops->get_write_counter)
> +               return ERR_PTR(-EINVAL);
> +
> +       if (!ops->write_blocks)
> +               return ERR_PTR(-EINVAL);
> +
> +       if (!ops->read_blocks)
> +               return ERR_PTR(-EINVAL);
> +
> +       rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
> +       if (!rdev)
> +               return ERR_PTR(-ENOMEM);
> +
> +       id = ida_simple_get(&rpmb_ida, 0, 0, GFP_KERNEL);
> +       if (id < 0) {
> +               ret = id;
> +               goto exit;
> +       }
> +
> +       mutex_init(&rdev->lock);
> +       rdev->ops = ops;
> +       rdev->id = id;
> +       rdev->target = target;
> +
> +       dev_set_name(&rdev->dev, "rpmb%d", id);
> +       rdev->dev.class = &rpmb_class;
> +       rdev->dev.parent = dev;
> +
> +       rpmb_cdev_prepare(rdev);

Ditto.

> +
> +       ret = device_register(&rdev->dev);
> +       if (ret)
> +               goto exit;
> +
> +       rpmb_cdev_add(rdev);

Ditto.

> +
> +       dev_dbg(&rdev->dev, "registered device\n");
> +
> +       return rdev;
> +
> +exit:
> +       if (id >= 0)
> +               ida_simple_remove(&rpmb_ida, id);
> +       kfree(rdev);
> +       return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(rpmb_dev_register);
> +

[...]

Kind regards
Uffe

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Shyam Saini]

[-- Attachment #1: Type: text/plain, Size: 7924 bytes --]


Hi Ulf,

> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
> <shyamsaini@linux.microsoft.com> wrote:
>>
>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
>> originally proposed by [2]Thomas Winkler ]
>>
>> A number of storage technologies support a specialised hardware
>> partition designed to be resistant to replay attacks. The underlying
>> HW protocols differ but the operations are common. The RPMB partition
>> cannot be accessed via standard block layer, but by a set of specific
>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
>> partition provides authenticated and replay protected access, hence
>> suitable as a secure storage.
>>
>> The initial aim of this patch is to provide a simple RPMB Driver which
>> can be accessed by Linux's optee driver to facilitate fast-path for
>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
>>
>> A TEE device driver can claim the RPMB interface, for example, via
>> class_interface_register(). The RPMB driver provides a series of
>> operations for interacting with the device.
>
> I don't quite follow this. More exactly, how will the TEE driver know
> what RPMB device it should use?

I don't have complete code to for this yet, but i think OP-TEE driver
should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
specific implementation for RPMB operations.

Linux optee driver can handle RPMB frames and pass it to RPMB subsystem

[1] U-Boot has mmc specific implementation

I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb, but in case if a
system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
should be declared as secure storage and optee should access that one 
only.

Sumit, do you have suggestions for this ?


>>
>>   * program_key - a one time operation for setting up a new device
>>   * get_capacity - introspect the device capacity
>>   * get_write_counter - check the write counter
>>   * write_blocks - write a series of blocks to the RPMB device
>>   * read_blocks - read a series of blocks from the RPMB device
>>
>> The detailed operation of implementing the access is left to the TEE
>> device driver itself.
>>
>> The framing details and HW specific bits (JDEC vs NVME frames) are
>> left to the lower level TEE driver to worry about.
>>
>> Without kernel fast path to RPMB access doesn't work when IMA try to
>> extend ftpm's PCR registers.
>>
>> This fast-path would require additional work in linux optee driver and
>> as well as in MMC driver.
>>
>> [1] https://lore.kernel.org/lkml/20220405093759.1126835-2-alex.bennee@linaro.org/
>> [2] https://lore.kernel.org/linux-mmc/1478548394-8184-2-git-send-email-tomas.winkler@intel.com/
>> [3] https://optee.readthedocs.io/en/latest/architecture/secure_storage.html
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
>>
>
> [...]
>
>> +/**
>> + * rpmb_dev_find_device() - return first matching rpmb device
>> + * @data: data for the match function
>> + * @match: the matching function
>> + *
>> + * Return: matching rpmb device or NULL on failure
>> + */
>> +static
>> +struct rpmb_dev *rpmb_dev_find_device(const void *data,
>> +                                     int (*match)(struct device *dev,
>> +                                                  const void *data))
>> +{
>> +       struct device *dev;
>> +
>> +       dev = class_find_device(&rpmb_class, NULL, data, match);
>> +
>> +       return dev ? to_rpmb_dev(dev) : NULL;
>> +}
>> +
>> +struct device_with_target {
>> +       const struct device *dev;
>> +       u8 target;
>> +};
>> +
>> +static int match_by_parent(struct device *dev, const void *data)
>> +{
>> +       const struct device_with_target *d = data;
>> +       struct rpmb_dev *rdev = to_rpmb_dev(dev);
>> +
>> +       return (d->dev && dev->parent == d->dev && rdev->target == d->target);
>> +}
>> +
>> +/**
>> + * rpmb_dev_find_by_device() - retrieve rpmb device from the parent device
>> + * @parent: parent device of the rpmb device
>> + * @target: RPMB target/region within the physical device
>> + *
>> + * Return: NULL if there is no rpmb device associated with the parent device
>> + */
>> +struct rpmb_dev *rpmb_dev_find_by_device(struct device *parent, u8 target)
>> +{
>> +       struct device_with_target t;
>> +
>> +       if (!parent)
>> +               return NULL;
>> +
>> +       t.dev = parent;
>> +       t.target = target;
>> +
>> +       return rpmb_dev_find_device(&t, match_by_parent);
>> +}
>> +EXPORT_SYMBOL_GPL(rpmb_dev_find_by_device);
>
> Is this what the TEE driver would be calling to find the rpmb device/partition?

yes, that's the idea.

>> +
>> +/**
>> + * rpmb_dev_unregister() - unregister RPMB partition from the RPMB subsystem
>> + * @rdev: the rpmb device to unregister
>> + * Return:
>> + * *        0 on success
>> + * *        -EINVAL on wrong parameters
>> + */
>> +int rpmb_dev_unregister(struct rpmb_dev *rdev)
>> +{
>> +       if (!rdev)
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&rdev->lock);
>> +       rpmb_cdev_del(rdev);
>
> I can't find the function above. I guess it should be included as a
> part of the patch too?

Sorry for the confusion, this is leftover from original version
Will be removed in next iteration.

>> +       device_del(&rdev->dev);
>> +       mutex_unlock(&rdev->lock);
>> +
>> +       rpmb_dev_put(rdev);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(rpmb_dev_unregister);
>
> [...]
>
>> +/**
>> + * rpmb_dev_register - register RPMB partition with the RPMB subsystem
>> + * @dev: storage device of the rpmb device
>> + * @target: RPMB target/region within the physical device
>> + * @ops: device specific operations
>> + *
>> + * Return: a pointer to rpmb device
>> + */
>> +struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target,
>> +                                  const struct rpmb_ops *ops)
>> +{
>> +       struct rpmb_dev *rdev;
>> +       int id;
>> +       int ret;
>> +
>> +       if (!dev || !ops)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       if (!ops->program_key)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       if (!ops->get_capacity)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       if (!ops->get_write_counter)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       if (!ops->write_blocks)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       if (!ops->read_blocks)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
>> +       if (!rdev)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       id = ida_simple_get(&rpmb_ida, 0, 0, GFP_KERNEL);
>> +       if (id < 0) {
>> +               ret = id;
>> +               goto exit;
>> +       }
>> +
>> +       mutex_init(&rdev->lock);
>> +       rdev->ops = ops;
>> +       rdev->id = id;
>> +       rdev->target = target;
>> +
>> +       dev_set_name(&rdev->dev, "rpmb%d", id);
>> +       rdev->dev.class = &rpmb_class;
>> +       rdev->dev.parent = dev;
>> +
>> +       rpmb_cdev_prepare(rdev);
>
> Ditto.

same as my last comment
>> +
>> +       ret = device_register(&rdev->dev);
>> +       if (ret)
>> +               goto exit;
>> +
>> +       rpmb_cdev_add(rdev);
>
> Ditto.

same as above.

>> +
>> +       dev_dbg(&rdev->dev, "registered device\n");
>> +
>> +       return rdev;
>> +
>> +exit:
>> +       if (id >= 0)
>> +               ida_simple_remove(&rpmb_ida, id);
>> +       kfree(rdev);
>> +       return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(rpmb_dev_register);
>> +
>
> [...]
>


[1] https://source.denx.de/u-boot/u-boot/-/commit/4853ad3e13e21462a86e09caee4ea27ae68e764b

Best Regards,
Shyam

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Jerome Forissier]

On 8/17/23 01:31, Shyam Saini wrote:
> 
> Hi Ulf,
> 
>> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
>> <shyamsaini@linux.microsoft.com> wrote:
>>>
>>> From: Alex Bennée <alex.bennee@linaro.org>
>>>
>>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
>>> originally proposed by [2]Thomas Winkler ]
>>>
>>> A number of storage technologies support a specialised hardware
>>> partition designed to be resistant to replay attacks. The underlying
>>> HW protocols differ but the operations are common. The RPMB partition
>>> cannot be accessed via standard block layer, but by a set of specific
>>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
>>> partition provides authenticated and replay protected access, hence
>>> suitable as a secure storage.
>>>
>>> The initial aim of this patch is to provide a simple RPMB Driver which
>>> can be accessed by Linux's optee driver to facilitate fast-path for
>>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
>>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
>>>
>>> A TEE device driver can claim the RPMB interface, for example, via
>>> class_interface_register(). The RPMB driver provides a series of
>>> operations for interacting with the device.
>>
>> I don't quite follow this. More exactly, how will the TEE driver know
>> what RPMB device it should use?
> 
> I don't have complete code to for this yet, but i think OP-TEE driver
> should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
> specific implementation for RPMB operations.
> 
> Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
> 
> [1] U-Boot has mmc specific implementation
> 
> I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
> CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,

Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
is given and use the specified RPMB instead (the CID is a non-ambiguous way
to identify a RPMB device).

> but in case if a
> system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
> should be declared as secure storage and optee should access that one only.

Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.

> Sumit, do you have suggestions for this ?


-- 
Jerome

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Sumit Garg]

On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
>
>
> On 8/17/23 01:31, Shyam Saini wrote:
> >
> > Hi Ulf,
> >
> >> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
> >> <shyamsaini@linux.microsoft.com> wrote:
> >>>
> >>> From: Alex Bennée <alex.bennee@linaro.org>
> >>>
> >>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
> >>> originally proposed by [2]Thomas Winkler ]
> >>>
> >>> A number of storage technologies support a specialised hardware
> >>> partition designed to be resistant to replay attacks. The underlying
> >>> HW protocols differ but the operations are common. The RPMB partition
> >>> cannot be accessed via standard block layer, but by a set of specific
> >>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
> >>> partition provides authenticated and replay protected access, hence
> >>> suitable as a secure storage.
> >>>
> >>> The initial aim of this patch is to provide a simple RPMB Driver which
> >>> can be accessed by Linux's optee driver to facilitate fast-path for
> >>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
> >>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
> >>>
> >>> A TEE device driver can claim the RPMB interface, for example, via
> >>> class_interface_register(). The RPMB driver provides a series of
> >>> operations for interacting with the device.
> >>
> >> I don't quite follow this. More exactly, how will the TEE driver know
> >> what RPMB device it should use?
> >
> > I don't have complete code to for this yet, but i think OP-TEE driver
> > should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
> > specific implementation for RPMB operations.
> >
> > Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
> >

It would be better to have this OP-TEE use case fully implemented. So
that we can justify it as a valid user for this proposed RPMB
subsystem. If you are looking for any further suggestions then please
let us know.

> > [1] U-Boot has mmc specific implementation
> >
> > I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
> > CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
>
> Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
> is given and use the specified RPMB instead (the CID is a non-ambiguous way
> to identify a RPMB device).
>
> > but in case if a
> > system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
> > should be declared as secure storage and optee should access that one only.
>
> Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
>
> > Sumit, do you have suggestions for this ?
>

I would suggest having an OP-TEE secure DT property that would provide
the RPMB CID which is allocated to the secure world.

-Sumit

>
> --
> Jerome

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Jens Wiklander]

On Mon, Aug 21, 2023 at 12:03 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
> <jerome.forissier@linaro.org> wrote:
> >
> >
> >
> > On 8/17/23 01:31, Shyam Saini wrote:
> > >
> > > Hi Ulf,
> > >
> > >> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
> > >> <shyamsaini@linux.microsoft.com> wrote:
> > >>>
> > >>> From: Alex Bennée <alex.bennee@linaro.org>
> > >>>
> > >>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
> > >>> originally proposed by [2]Thomas Winkler ]
> > >>>
> > >>> A number of storage technologies support a specialised hardware
> > >>> partition designed to be resistant to replay attacks. The underlying
> > >>> HW protocols differ but the operations are common. The RPMB partition
> > >>> cannot be accessed via standard block layer, but by a set of specific
> > >>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
> > >>> partition provides authenticated and replay protected access, hence
> > >>> suitable as a secure storage.
> > >>>
> > >>> The initial aim of this patch is to provide a simple RPMB Driver which
> > >>> can be accessed by Linux's optee driver to facilitate fast-path for
> > >>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
> > >>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
> > >>>
> > >>> A TEE device driver can claim the RPMB interface, for example, via
> > >>> class_interface_register(). The RPMB driver provides a series of
> > >>> operations for interacting with the device.
> > >>
> > >> I don't quite follow this. More exactly, how will the TEE driver know
> > >> what RPMB device it should use?
> > >
> > > I don't have complete code to for this yet, but i think OP-TEE driver
> > > should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
> > > specific implementation for RPMB operations.
> > >
> > > Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
> > >
>
> It would be better to have this OP-TEE use case fully implemented. So
> that we can justify it as a valid user for this proposed RPMB
> subsystem. If you are looking for any further suggestions then please
> let us know.

+1

>
> > > [1] U-Boot has mmc specific implementation
> > >
> > > I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
> > > CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
> >
> > Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
> > is given and use the specified RPMB instead (the CID is a non-ambiguous way
> > to identify a RPMB device).
> >
> > > but in case if a
> > > system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
> > > should be declared as secure storage and optee should access that one only.
> >
> > Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
> >
> > > Sumit, do you have suggestions for this ?
> >
>
> I would suggest having an OP-TEE secure DT property that would provide
> the RPMB CID which is allocated to the secure world.

Another option is for OP-TEE to iterate over all RPMBs with a
programmed key and test if the key OP-TEE would use works. That should
avoid the problem of provisioning a device-unique secure DTB. I'd
expect that the RPMB key is programmed by a trusted provisioning tool
since allowing OP-TEE to program the RPMB key has never been secure,
not unless the OP-TEE binary is rollback protected.

Cheers,
Jens

>
> -Sumit
>
> >
> > --
> > Jerome

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Sumit Garg]

On Mon, 21 Aug 2023 at 16:49, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Mon, Aug 21, 2023 at 12:03 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
> > <jerome.forissier@linaro.org> wrote:
> > >
> > >
> > >
> > > On 8/17/23 01:31, Shyam Saini wrote:
> > > >
> > > > Hi Ulf,
> > > >
> > > >> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
> > > >> <shyamsaini@linux.microsoft.com> wrote:
> > > >>>
> > > >>> From: Alex Bennée <alex.bennee@linaro.org>
> > > >>>
> > > >>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
> > > >>> originally proposed by [2]Thomas Winkler ]
> > > >>>
> > > >>> A number of storage technologies support a specialised hardware
> > > >>> partition designed to be resistant to replay attacks. The underlying
> > > >>> HW protocols differ but the operations are common. The RPMB partition
> > > >>> cannot be accessed via standard block layer, but by a set of specific
> > > >>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
> > > >>> partition provides authenticated and replay protected access, hence
> > > >>> suitable as a secure storage.
> > > >>>
> > > >>> The initial aim of this patch is to provide a simple RPMB Driver which
> > > >>> can be accessed by Linux's optee driver to facilitate fast-path for
> > > >>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
> > > >>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
> > > >>>
> > > >>> A TEE device driver can claim the RPMB interface, for example, via
> > > >>> class_interface_register(). The RPMB driver provides a series of
> > > >>> operations for interacting with the device.
> > > >>
> > > >> I don't quite follow this. More exactly, how will the TEE driver know
> > > >> what RPMB device it should use?
> > > >
> > > > I don't have complete code to for this yet, but i think OP-TEE driver
> > > > should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
> > > > specific implementation for RPMB operations.
> > > >
> > > > Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
> > > >
> >
> > It would be better to have this OP-TEE use case fully implemented. So
> > that we can justify it as a valid user for this proposed RPMB
> > subsystem. If you are looking for any further suggestions then please
> > let us know.
>
> +1
>
> >
> > > > [1] U-Boot has mmc specific implementation
> > > >
> > > > I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
> > > > CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
> > >
> > > Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
> > > is given and use the specified RPMB instead (the CID is a non-ambiguous way
> > > to identify a RPMB device).
> > >
> > > > but in case if a
> > > > system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
> > > > should be declared as secure storage and optee should access that one only.
> > >
> > > Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
> > >
> > > > Sumit, do you have suggestions for this ?
> > >
> >
> > I would suggest having an OP-TEE secure DT property that would provide
> > the RPMB CID which is allocated to the secure world.
>
> Another option is for OP-TEE to iterate over all RPMBs with a
> programmed key and test if the key OP-TEE would use works.

That would require intercepting OP-TEE RPMB frames such that any
"write key" frame is blocked. As we don't want OP-TEE to occupy
unprovisioned RPMB partitions.

> That should
> avoid the problem of provisioning a device-unique secure DTB.

Okay I see the scalability concerns. So how about instead we have a
UFS/eMMC/NVMe controller specific boolean secure RPMB DT property?

> I'd
> expect that the RPMB key is programmed by a trusted provisioning tool
> since allowing OP-TEE to program the RPMB key has never been secure,
> not unless the OP-TEE binary is rollback protected.

Agree but any such RPMB key provisioning tool should either belong to
OP-TEE, u-boot or Linux.

-Sumit

>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > >
> > > --
> > > Jerome

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Ilias Apalodimas]

On Mon, 21 Aug 2023 at 14:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Mon, Aug 21, 2023 at 12:03 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
> > <jerome.forissier@linaro.org> wrote:
> > >
> > >
> > >
> > > On 8/17/23 01:31, Shyam Saini wrote:
> > > >
> > > > Hi Ulf,
> > > >
> > > >> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
> > > >> <shyamsaini@linux.microsoft.com> wrote:
> > > >>>
> > > >>> From: Alex Bennée <alex.bennee@linaro.org>
> > > >>>
> > > >>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
> > > >>> originally proposed by [2]Thomas Winkler ]
> > > >>>
> > > >>> A number of storage technologies support a specialised hardware
> > > >>> partition designed to be resistant to replay attacks. The underlying
> > > >>> HW protocols differ but the operations are common. The RPMB partition
> > > >>> cannot be accessed via standard block layer, but by a set of specific
> > > >>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
> > > >>> partition provides authenticated and replay protected access, hence
> > > >>> suitable as a secure storage.
> > > >>>
> > > >>> The initial aim of this patch is to provide a simple RPMB Driver which
> > > >>> can be accessed by Linux's optee driver to facilitate fast-path for
> > > >>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
> > > >>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
> > > >>>
> > > >>> A TEE device driver can claim the RPMB interface, for example, via
> > > >>> class_interface_register(). The RPMB driver provides a series of
> > > >>> operations for interacting with the device.
> > > >>
> > > >> I don't quite follow this. More exactly, how will the TEE driver know
> > > >> what RPMB device it should use?
> > > >
> > > > I don't have complete code to for this yet, but i think OP-TEE driver
> > > > should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
> > > > specific implementation for RPMB operations.
> > > >
> > > > Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
> > > >
> >
> > It would be better to have this OP-TEE use case fully implemented. So
> > that we can justify it as a valid user for this proposed RPMB
> > subsystem. If you are looking for any further suggestions then please
> > let us know.
>
> +1
>
> >
> > > > [1] U-Boot has mmc specific implementation
> > > >
> > > > I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
> > > > CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
> > >
> > > Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
> > > is given and use the specified RPMB instead (the CID is a non-ambiguous way
> > > to identify a RPMB device).
> > >
> > > > but in case if a
> > > > system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
> > > > should be declared as secure storage and optee should access that one only.
> > >
> > > Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
> > >
> > > > Sumit, do you have suggestions for this ?
> > >
> >
> > I would suggest having an OP-TEE secure DT property that would provide
> > the RPMB CID which is allocated to the secure world.
>
> Another option is for OP-TEE to iterate over all RPMBs with a
> programmed key and test if the key OP-TEE would use works. That should
> avoid the problem of provisioning a device-unique secure DTB. I'd
> expect that the RPMB key is programmed by a trusted provisioning tool
> since allowing OP-TEE to program the RPMB key has never been secure,
> not unless the OP-TEE binary is rollback protected.

+1 to that.  Overall we shound't 'trust' to do the programming. For
example, in OP-TEE if you compile it with device programming
capabilities, you can easily convince OP-TEE to send you the symmetric
key by swapping the supplicant with a malicious application.

Thanks
/Ilias
>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > >
> > > --
> > > Jerome

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Sumit Garg]

On Mon, 21 Aug 2023 at 17:26, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Mon, 21 Aug 2023 at 14:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Mon, Aug 21, 2023 at 12:03 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
> > > <jerome.forissier@linaro.org> wrote:
> > > >
> > > >
> > > >
> > > > On 8/17/23 01:31, Shyam Saini wrote:
> > > > >
> > > > > Hi Ulf,
> > > > >
> > > > >> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
> > > > >> <shyamsaini@linux.microsoft.com> wrote:
> > > > >>>
> > > > >>> From: Alex Bennée <alex.bennee@linaro.org>
> > > > >>>
> > > > >>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
> > > > >>> originally proposed by [2]Thomas Winkler ]
> > > > >>>
> > > > >>> A number of storage technologies support a specialised hardware
> > > > >>> partition designed to be resistant to replay attacks. The underlying
> > > > >>> HW protocols differ but the operations are common. The RPMB partition
> > > > >>> cannot be accessed via standard block layer, but by a set of specific
> > > > >>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
> > > > >>> partition provides authenticated and replay protected access, hence
> > > > >>> suitable as a secure storage.
> > > > >>>
> > > > >>> The initial aim of this patch is to provide a simple RPMB Driver which
> > > > >>> can be accessed by Linux's optee driver to facilitate fast-path for
> > > > >>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
> > > > >>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
> > > > >>>
> > > > >>> A TEE device driver can claim the RPMB interface, for example, via
> > > > >>> class_interface_register(). The RPMB driver provides a series of
> > > > >>> operations for interacting with the device.
> > > > >>
> > > > >> I don't quite follow this. More exactly, how will the TEE driver know
> > > > >> what RPMB device it should use?
> > > > >
> > > > > I don't have complete code to for this yet, but i think OP-TEE driver
> > > > > should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
> > > > > specific implementation for RPMB operations.
> > > > >
> > > > > Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
> > > > >
> > >
> > > It would be better to have this OP-TEE use case fully implemented. So
> > > that we can justify it as a valid user for this proposed RPMB
> > > subsystem. If you are looking for any further suggestions then please
> > > let us know.
> >
> > +1
> >
> > >
> > > > > [1] U-Boot has mmc specific implementation
> > > > >
> > > > > I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
> > > > > CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
> > > >
> > > > Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
> > > > is given and use the specified RPMB instead (the CID is a non-ambiguous way
> > > > to identify a RPMB device).
> > > >
> > > > > but in case if a
> > > > > system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
> > > > > should be declared as secure storage and optee should access that one only.
> > > >
> > > > Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
> > > >
> > > > > Sumit, do you have suggestions for this ?
> > > >
> > >
> > > I would suggest having an OP-TEE secure DT property that would provide
> > > the RPMB CID which is allocated to the secure world.
> >
> > Another option is for OP-TEE to iterate over all RPMBs with a
> > programmed key and test if the key OP-TEE would use works. That should
> > avoid the problem of provisioning a device-unique secure DTB. I'd
> > expect that the RPMB key is programmed by a trusted provisioning tool
> > since allowing OP-TEE to program the RPMB key has never been secure,
> > not unless the OP-TEE binary is rollback protected.
>
> +1 to that.  Overall we shound't 'trust' to do the programming. For
> example, in OP-TEE if you compile it with device programming
> capabilities, you can easily convince OP-TEE to send you the symmetric
> key by swapping the supplicant with a malicious application.
>

Agree, with your overall intent, that OP-TEE shouldn't expose RPMB key
in plain form. But with suggested OP-TEE RPMB frames routing via
kernel, tee-supplicant won't be used for RPMB accesses.

-Sumit

> Thanks
> /Ilias
> >
> > Cheers,
> > Jens
> >
> > >
> > > -Sumit
> > >
> > > >
> > > > --
> > > > Jerome

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Shyam Saini]

[-- Attachment #1: Type: text/plain, Size: 4435 bytes --]


> On Mon, 21 Aug 2023 at 17:26, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> On Mon, 21 Aug 2023 at 14:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>
>>> On Mon, Aug 21, 2023 at 12:03 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>>>>
>>>> On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
>>>> <jerome.forissier@linaro.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 8/17/23 01:31, Shyam Saini wrote:
>>>>>>
>>>>>> Hi Ulf,
>>>>>>
>>>>>>> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
>>>>>>> <shyamsaini@linux.microsoft.com> wrote:
>>>>>>>>
>>>>>>>> From: Alex Bennée <alex.bennee@linaro.org>
>>>>>>>>
>>>>>>>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
>>>>>>>> originally proposed by [2]Thomas Winkler ]
>>>>>>>>
>>>>>>>> A number of storage technologies support a specialised hardware
>>>>>>>> partition designed to be resistant to replay attacks. The underlying
>>>>>>>> HW protocols differ but the operations are common. The RPMB partition
>>>>>>>> cannot be accessed via standard block layer, but by a set of specific
>>>>>>>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
>>>>>>>> partition provides authenticated and replay protected access, hence
>>>>>>>> suitable as a secure storage.
>>>>>>>>
>>>>>>>> The initial aim of this patch is to provide a simple RPMB Driver which
>>>>>>>> can be accessed by Linux's optee driver to facilitate fast-path for
>>>>>>>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
>>>>>>>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
>>>>>>>>
>>>>>>>> A TEE device driver can claim the RPMB interface, for example, via
>>>>>>>> class_interface_register(). The RPMB driver provides a series of
>>>>>>>> operations for interacting with the device.
>>>>>>>
>>>>>>> I don't quite follow this. More exactly, how will the TEE driver know
>>>>>>> what RPMB device it should use?
>>>>>>
>>>>>> I don't have complete code to for this yet, but i think OP-TEE driver
>>>>>> should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
>>>>>> specific implementation for RPMB operations.
>>>>>>
>>>>>> Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
>>>>>>
>>>>
>>>> It would be better to have this OP-TEE use case fully implemented. So
>>>> that we can justify it as a valid user for this proposed RPMB
>>>> subsystem. If you are looking for any further suggestions then please
>>>> let us know.
>>>
>>> +1
>>>
>>>>
>>>>>> [1] U-Boot has mmc specific implementation
>>>>>>
>>>>>> I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
>>>>>> CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
>>>>>
>>>>> Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
>>>>> is given and use the specified RPMB instead (the CID is a non-ambiguous way
>>>>> to identify a RPMB device).
>>>>>
>>>>>> but in case if a
>>>>>> system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
>>>>>> should be declared as secure storage and optee should access that one only.
>>>>>
>>>>> Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
>>>>>
>>>>>> Sumit, do you have suggestions for this ?
>>>>>
>>>>
>>>> I would suggest having an OP-TEE secure DT property that would provide
>>>> the RPMB CID which is allocated to the secure world.
>>>
>>> Another option is for OP-TEE to iterate over all RPMBs with a
>>> programmed key and test if the key OP-TEE would use works. That should
>>> avoid the problem of provisioning a device-unique secure DTB. I'd
>>> expect that the RPMB key is programmed by a trusted provisioning tool
>>> since allowing OP-TEE to program the RPMB key has never been secure,
>>> not unless the OP-TEE binary is rollback protected.
>>
>> +1 to that.  Overall we shound't 'trust' to do the programming. For
>> example, in OP-TEE if you compile it with device programming
>> capabilities, you can easily convince OP-TEE to send you the symmetric
>> key by swapping the supplicant with a malicious application.
>>
>
> Agree, with your overall intent, that OP-TEE shouldn't expose RPMB key
> in plain form. But with suggested OP-TEE RPMB frames routing via
> kernel, tee-supplicant won't be used for RPMB accesses.

do we plan to disable access to RPMB devices, once we have this RPMB 
driver in place. User space tools like mmc-utils/nvme/ufs utils
can still access RPMB and programme the key and should
RPMB driver deny access to RPMB ?

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Linus Walleij]

On Tue, Aug 22, 2023 at 9:07 PM Shyam Saini
<shyamsaini@linux.microsoft.com> wrote:

> do we plan to disable access to RPMB devices, once we have this RPMB
> driver in place. User space tools like mmc-utils/nvme/ufs utils
> can still access RPMB and programme the key and should
> RPMB driver deny access to RPMB ?

We don't break userspace. Just not. This is not an option.

The RPMB subsystem simply has to provide the rpmb character
device the same way the MMC subsystem did, or provide an
in-kernel backend to the MMC subsystem so that it can provide
the same device. Whatever solution is best.

No deprecation and deletion and breaking userspace. Ever.

Yours,
Linus Walleij

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Sumit Garg]

On Wed, 23 Aug 2023 at 13:34, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Aug 22, 2023 at 9:07 PM Shyam Saini
> <shyamsaini@linux.microsoft.com> wrote:
>
> > do we plan to disable access to RPMB devices, once we have this RPMB
> > driver in place. User space tools like mmc-utils/nvme/ufs utils
> > can still access RPMB and programme the key and should
> > RPMB driver deny access to RPMB ?
>
> We don't break userspace. Just not. This is not an option.
>
> The RPMB subsystem simply has to provide the rpmb character
> device the same way the MMC subsystem did, or provide an
> in-kernel backend to the MMC subsystem so that it can provide
> the same device. Whatever solution is best.
>
> No deprecation and deletion and breaking userspace. Ever.
>

Agree, that's the golden rule of thumb we follow in kernel
development. Also, we still need to allow cases where trusted
provisioning user-space tools can program OP-TEE RPMB key during
factory provisioning. And once that is provisioned, I don't think
there is much harm in still exposing the RPMB device to user-space
since it can't do anything malicious without access to OP-TEE RPMB
key.

-Sumit

> Yours,
> Linus Walleij

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Jerome Forissier]

On 8/21/23 13:18, Jens Wiklander wrote:
> On Mon, Aug 21, 2023 at 12:03 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>>
>> On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
>> <jerome.forissier@linaro.org> wrote:
>>>
>>>
>>>
>>> On 8/17/23 01:31, Shyam Saini wrote:
>>>>
>>>> Hi Ulf,
>>>>
>>>>> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
>>>>> <shyamsaini@linux.microsoft.com> wrote:
>>>>>>
>>>>>> From: Alex Bennée <alex.bennee@linaro.org>
>>>>>>
>>>>>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
>>>>>> originally proposed by [2]Thomas Winkler ]
>>>>>>
>>>>>> A number of storage technologies support a specialised hardware
>>>>>> partition designed to be resistant to replay attacks. The underlying
>>>>>> HW protocols differ but the operations are common. The RPMB partition
>>>>>> cannot be accessed via standard block layer, but by a set of specific
>>>>>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
>>>>>> partition provides authenticated and replay protected access, hence
>>>>>> suitable as a secure storage.
>>>>>>
>>>>>> The initial aim of this patch is to provide a simple RPMB Driver which
>>>>>> can be accessed by Linux's optee driver to facilitate fast-path for
>>>>>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
>>>>>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
>>>>>>
>>>>>> A TEE device driver can claim the RPMB interface, for example, via
>>>>>> class_interface_register(). The RPMB driver provides a series of
>>>>>> operations for interacting with the device.
>>>>>
>>>>> I don't quite follow this. More exactly, how will the TEE driver know
>>>>> what RPMB device it should use?
>>>>
>>>> I don't have complete code to for this yet, but i think OP-TEE driver
>>>> should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
>>>> specific implementation for RPMB operations.
>>>>
>>>> Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
>>>>
>>
>> It would be better to have this OP-TEE use case fully implemented. So
>> that we can justify it as a valid user for this proposed RPMB
>> subsystem. If you are looking for any further suggestions then please
>> let us know.
> 
> +1
> 
>>
>>>> [1] U-Boot has mmc specific implementation
>>>>
>>>> I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
>>>> CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
>>>
>>> Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
>>> is given and use the specified RPMB instead (the CID is a non-ambiguous way
>>> to identify a RPMB device).
>>>
>>>> but in case if a
>>>> system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
>>>> should be declared as secure storage and optee should access that one only.
>>>
>>> Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
>>>
>>>> Sumit, do you have suggestions for this ?
>>>
>>
>> I would suggest having an OP-TEE secure DT property that would provide
>> the RPMB CID which is allocated to the secure world.
> 
> Another option is for OP-TEE to iterate over all RPMBs with a
> programmed key and test if the key OP-TEE would use works. That should
> avoid the problem of provisioning a device-unique secure DTB. I'd
> expect that the RPMB key is programmed by a trusted provisioning tool
> since allowing OP-TEE to program the RPMB key has never been secure,
> not unless the OP-TEE binary is rollback protected.

+1 if we can assume the same key won't be used for several devices, which
is probably reasonable.

> 
> Cheers,
> Jens
> 
>>
>> -Sumit
>>
>>>
>>> --
>>> Jerome

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Shyam Saini]

[-- Attachment #1: Type: text/plain, Size: 4060 bytes --]


Hi Sumit,

> On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
> <jerome.forissier@linaro.org> wrote:
>>
>>
>>
>> On 8/17/23 01:31, Shyam Saini wrote:
>>>
>>> Hi Ulf,
>>>
>>>> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
>>>> <shyamsaini@linux.microsoft.com> wrote:
>>>>>
>>>>> From: Alex Bennée <alex.bennee@linaro.org>
>>>>>
>>>>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
>>>>> originally proposed by [2]Thomas Winkler ]
>>>>>
>>>>> A number of storage technologies support a specialised hardware
>>>>> partition designed to be resistant to replay attacks. The underlying
>>>>> HW protocols differ but the operations are common. The RPMB partition
>>>>> cannot be accessed via standard block layer, but by a set of specific
>>>>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
>>>>> partition provides authenticated and replay protected access, hence
>>>>> suitable as a secure storage.
>>>>>
>>>>> The initial aim of this patch is to provide a simple RPMB Driver which
>>>>> can be accessed by Linux's optee driver to facilitate fast-path for
>>>>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
>>>>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
>>>>>
>>>>> A TEE device driver can claim the RPMB interface, for example, via
>>>>> class_interface_register(). The RPMB driver provides a series of
>>>>> operations for interacting with the device.
>>>>
>>>> I don't quite follow this. More exactly, how will the TEE driver know
>>>> what RPMB device it should use?
>>>
>>> I don't have complete code to for this yet, but i think OP-TEE driver
>>> should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
>>> specific implementation for RPMB operations.
>>>
>>> Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
>>>
>
> It would be better to have this OP-TEE use case fully implemented. So
> that we can justify it as a valid user for this proposed RPMB
> subsystem. If you are looking for any further suggestions then please
> let us know.

I was looking into UFS/NVMe user-space utils, it seems we may have to 
adapt rpmb frame data structure in optee-os to to handle NVMe/UFS
specific bits.

For nvme rpmb data frame, I think we would need an extra "target" member 
in rpmb data frame structure,
as NVMe can support upto 7 RPMB units, see  [1] "struct rpmb_data_frame_t"
UFS may support upto 3 or 4 RPMB regions.

So even if we use CID to uniquely identify RPMB device either from 
eMMC/NVMe/UFS, we still need identify which RPMB target/unit in case
if the device is NVMe, and which RPMB region if the device UFS.

Also both NVMe/UFS utils have two extra RPMB operations implemented,
Although new request/response operation than eMMC spec:
  1) Authenticated Device Configuration Block Write
  2) Authenticated Device Configuration Block Read

see [2] enum rpmb_request/response_type and [3]enum rpmb_op_type

do we need those implemented as well ?

Please let me know what you think about these.

[1] https://github.com/linux-nvme/nvme-cli/blob/master/nvme-rpmb.c#L252
[2] https://github.com/linux-nvme/nvme-cli/blob/master/nvme-rpmb.c#L230
[3] https://github.com/westerndigitalcorporation/ufs-utils/blob/dev/ufs_rpmb.c#L27

>>> [1] U-Boot has mmc specific implementation
>>>
>>> I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
>>> CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
>>
>> Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
>> is given and use the specified RPMB instead (the CID is a non-ambiguous way
>> to identify a RPMB device).
>>
>>> but in case if a
>>> system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
>>> should be declared as secure storage and optee should access that one only.
>>
>> Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
>>
>>> Sumit, do you have suggestions for this ?
>>
>
> I would suggest having an OP-TEE secure DT property that would provide
> the RPMB CID which is allocated to the secure world.
>
> -Sumit
>
>>
>> --
>> Jerome
>

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Sumit Garg]

On Wed, 23 Aug 2023 at 00:29, Shyam Saini
<shyamsaini@linux.microsoft.com> wrote:
>
>
> Hi Sumit,
>
> > On Mon, 21 Aug 2023 at 15:19, Jerome Forissier
> > <jerome.forissier@linaro.org> wrote:
> >>
> >>
> >>
> >> On 8/17/23 01:31, Shyam Saini wrote:
> >>>
> >>> Hi Ulf,
> >>>
> >>>> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
> >>>> <shyamsaini@linux.microsoft.com> wrote:
> >>>>>
> >>>>> From: Alex Bennée <alex.bennee@linaro.org>
> >>>>>
> >>>>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
> >>>>> originally proposed by [2]Thomas Winkler ]
> >>>>>
> >>>>> A number of storage technologies support a specialised hardware
> >>>>> partition designed to be resistant to replay attacks. The underlying
> >>>>> HW protocols differ but the operations are common. The RPMB partition
> >>>>> cannot be accessed via standard block layer, but by a set of specific
> >>>>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
> >>>>> partition provides authenticated and replay protected access, hence
> >>>>> suitable as a secure storage.
> >>>>>
> >>>>> The initial aim of this patch is to provide a simple RPMB Driver which
> >>>>> can be accessed by Linux's optee driver to facilitate fast-path for
> >>>>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
> >>>>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
> >>>>>
> >>>>> A TEE device driver can claim the RPMB interface, for example, via
> >>>>> class_interface_register(). The RPMB driver provides a series of
> >>>>> operations for interacting with the device.
> >>>>
> >>>> I don't quite follow this. More exactly, how will the TEE driver know
> >>>> what RPMB device it should use?
> >>>
> >>> I don't have complete code to for this yet, but i think OP-TEE driver
> >>> should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
> >>> specific implementation for RPMB operations.
> >>>
> >>> Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
> >>>
> >
> > It would be better to have this OP-TEE use case fully implemented. So
> > that we can justify it as a valid user for this proposed RPMB
> > subsystem. If you are looking for any further suggestions then please
> > let us know.
>
> I was looking into UFS/NVMe user-space utils, it seems we may have to
> adapt rpmb frame data structure in optee-os to to handle NVMe/UFS
> specific bits.
>
> For nvme rpmb data frame, I think we would need an extra "target" member
> in rpmb data frame structure,
> as NVMe can support upto 7 RPMB units, see  [1] "struct rpmb_data_frame_t"
> UFS may support upto 3 or 4 RPMB regions.
>
> So even if we use CID to uniquely identify RPMB device either from
> eMMC/NVMe/UFS, we still need identify which RPMB target/unit in case
> if the device is NVMe, and which RPMB region if the device UFS.
>
> Also both NVMe/UFS utils have two extra RPMB operations implemented,
> Although new request/response operation than eMMC spec:
>   1) Authenticated Device Configuration Block Write
>   2) Authenticated Device Configuration Block Read
>
> see [2] enum rpmb_request/response_type and [3]enum rpmb_op_type
>
> do we need those implemented as well ?

IMO, we should start with eMMC RPMB support first with OP-TEE. This is
what OP-TEE currently supports. And later on we can extend that
framework for UFS and NVMe RPMB.

We need to put extra care here regarding the eMMC RPMB ABI among
OP-TEE and Linux kernel. It should be designed in a way that it is
future compatible for UFS/NMVe. IOW, the bits that you have already
discovered above.

Also, we have to be backwards compatible with eMMC RPMB ABI towards
u-boot too since OP-TEE would use the same ABI whether it is towards
Linux or u-boot.

-Sumit

>
> Please let me know what you think about these.
>
> [1] https://github.com/linux-nvme/nvme-cli/blob/master/nvme-rpmb.c#L252
> [2] https://github.com/linux-nvme/nvme-cli/blob/master/nvme-rpmb.c#L230
> [3] https://github.com/westerndigitalcorporation/ufs-utils/blob/dev/ufs_rpmb.c#L27
>
> >>> [1] U-Boot has mmc specific implementation
> >>>
> >>> I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
> >>> CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
> >>
> >> Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
> >> is given and use the specified RPMB instead (the CID is a non-ambiguous way
> >> to identify a RPMB device).
> >>
> >>> but in case if a
> >>> system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
> >>> should be declared as secure storage and optee should access that one only.
> >>
> >> Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
> >>
> >>> Sumit, do you have suggestions for this ?
> >>
> >
> > I would suggest having an OP-TEE secure DT property that would provide
> > the RPMB CID which is allocated to the secure world.
> >
> > -Sumit
> >
> >>
> >> --
> >> Jerome
> >

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Shyam Saini]

[-- Attachment #1: Type: text/plain, Size: 2433 bytes --]



Hi Jerome,

>
>
> On 8/17/23 01:31, Shyam Saini wrote:
>>
>> Hi Ulf,
>>
>>> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
>>> <shyamsaini@linux.microsoft.com> wrote:
>>>>
>>>> From: Alex Bennée <alex.bennee@linaro.org>
>>>>
>>>> [This is patch 1 from [1] Alex's submission and this RPMB layer was
>>>> originally proposed by [2]Thomas Winkler ]
>>>>
>>>> A number of storage technologies support a specialised hardware
>>>> partition designed to be resistant to replay attacks. The underlying
>>>> HW protocols differ but the operations are common. The RPMB partition
>>>> cannot be accessed via standard block layer, but by a set of specific
>>>> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a
>>>> partition provides authenticated and replay protected access, hence
>>>> suitable as a secure storage.
>>>>
>>>> The initial aim of this patch is to provide a simple RPMB Driver which
>>>> can be accessed by Linux's optee driver to facilitate fast-path for
>>>> RPMB access to optee OS(secure OS) during the boot time. [1] Currently,
>>>> Optee OS relies on user-tee supplicant to access eMMC RPMB partition.
>>>>
>>>> A TEE device driver can claim the RPMB interface, for example, via
>>>> class_interface_register(). The RPMB driver provides a series of
>>>> operations for interacting with the device.
>>>
>>> I don't quite follow this. More exactly, how will the TEE driver know
>>> what RPMB device it should use?
>>
>> I don't have complete code to for this yet, but i think OP-TEE driver
>> should register with RPMB subsystem and then we can have eMMC/UFS/NVMe
>> specific implementation for RPMB operations.
>>
>> Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
>>
>> [1] U-Boot has mmc specific implementation
>>
>> I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option
>> CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb,
>
> Correct. Note that tee-supplicant will ignore this device ID if --rmb-cid
> is given and use the specified RPMB instead (the CID is a non-ambiguous way
> to identify a RPMB device).

Thanks, but we may still need to address with multiple RPMB 
targets/regions in case of UFS/NVMe.

>> but in case if a
>> system has multiple RPMB devices such as UFS/eMMC/NVMe, one them
>> should be declared as secure storage and optee should access that one only.
>
> Indeed, that would be an equivalent of tee-supplicant's --rpmb-cid.
>
>> Sumit, do you have suggestions for this ?

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Bart Van Assche]

On 7/21/23 18:40, Shyam Saini wrote:
> +config RPMB
> +	tristate "RPMB partition interface"
> +	help
> +	  Unified RPMB partition interface for RPMB capable devices such as
> +          eMMC and UFS. Provides interface for in kernel security controllers to
> +	  access RPMB partition.
> +
> +	  If unsure, select N.

Please also mention NVMe.

Please change the word "partition" into "unit" to avoid confusion with 
the concept "LBA range partition".

> +static DEFINE_IDA(rpmb_ida);

How are accesses to this IDA serialized?

> +/**
> + * rpmb_get_capacity() - returns the capacity of the rpmb device
> + * @rdev: rpmb device
> + *
> + * Return:
> + * *        capacity of the device in units of 128K, on success
> + * *        -EINVAL on wrong parameters
> + * *        -EOPNOTSUPP if device doesn't support the requested operation
> + * *        < 0 if the operation fails
> + */

Why in units of 128 KiB?

> +/**
> + * rpmb_dev_find_by_device() - retrieve rpmb device from the parent device
> + * @parent: parent device of the rpmb device
> + * @target: RPMB target/region within the physical device
> + *
> + * Return: NULL if there is no rpmb device associated with the parent device
> + */

Can an NVMe controller have multiple RPMB units? From the NVMe 
specification: "The controller may support multiple RPMB targets."

Can rpmb_dev_find_by_device() be used if multiple RPMB units are 
associated with a single controller?

> +/**
> + * rpmb_dev_register - register RPMB partition with the RPMB subsystem
> + * @dev: storage device of the rpmb device
> + * @target: RPMB target/region within the physical device
> + * @ops: device specific operations
> + *
> + * Return: a pointer to rpmb device
> + */
> +struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target,
> +				   const struct rpmb_ops *ops)
> +{
> +	struct rpmb_dev *rdev;
> +	int id;
> +	int ret;
> +
> +	if (!dev || !ops)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!ops->program_key)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!ops->get_capacity)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!ops->get_write_counter)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!ops->write_blocks)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!ops->read_blocks)
> +		return ERR_PTR(-EINVAL);
> +
> +	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
> +	if (!rdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	id = ida_simple_get(&rpmb_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0) {
> +		ret = id;
> +		goto exit;
> +	}
> +
> +	mutex_init(&rdev->lock);
> +	rdev->ops = ops;
> +	rdev->id = id;
> +	rdev->target = target;
> +
> +	dev_set_name(&rdev->dev, "rpmb%d", id);
> +	rdev->dev.class = &rpmb_class;
> +	rdev->dev.parent = dev;
> +
> +	rpmb_cdev_prepare(rdev);
> +
> +	ret = device_register(&rdev->dev);
> +	if (ret)
> +		goto exit;
> +
> +	rpmb_cdev_add(rdev);
> +
> +	dev_dbg(&rdev->dev, "registered device\n");
> +
> +	return rdev;
> +
> +exit:
> +	if (id >= 0)
> +		ida_simple_remove(&rpmb_ida, id);
> +	kfree(rdev);
> +	return ERR_PTR(ret);
> +}

How is user space software supposed to map an NVMe RPMB target ID to an 
RPMB device name?

> +MODULE_AUTHOR("Intel Corporation");

Shouldn't this be the name of a person instead of the name of a company?

Thanks,

Bart.

Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver

[Shyam Saini]

Hi Bart,

sorry for not replying earlier, as I am very new to NVMe/UFS spec and was 
figuring out few details about them.

> On 7/21/23 18:40, Shyam Saini wrote:
>>  +config RPMB
>>  +	tristate "RPMB partition interface"
>>  +	help
>>  +	  Unified RPMB partition interface for RPMB capable devices such as
>>  +          eMMC and UFS. Provides interface for in kernel security
>>  controllers to
>>  +	  access RPMB partition.
>>  +
>>  +	  If unsure, select N.
>
> Please also mention NVMe.

Sure,
> Please change the word "partition" into "unit" to avoid confusion with the 
> concept "LBA range partition".

sure, in next iteration

>>  +static DEFINE_IDA(rpmb_ida);
>
> How are accesses to this IDA serialized?

I will look into that.

>>  +/**
>>  + * rpmb_get_capacity() - returns the capacity of the rpmb device
>>  + * @rdev: rpmb device
>>  + *
>>  + * Return:
>>  + * *        capacity of the device in units of 128K, on success
>>  + * *        -EINVAL on wrong parameters
>>  + * *        -EOPNOTSUPP if device doesn't support the requested operation
>>  + * *        < 0 if the operation fails
>>  + */
>
> Why in units of 128 KiB?

I think UFS/eMMC RPMB spec suggests size of RPMB multiple of 128K
and NVMe spec suggests RPMB Data Area to be multiple of 128K as well.

>>  +/**
>>  + * rpmb_dev_find_by_device() - retrieve rpmb device from the parent
>>  device
>>  + * @parent: parent device of the rpmb device
>>  + * @target: RPMB target/region within the physical device
>>  + *
>>  + * Return: NULL if there is no rpmb device associated with the parent
>>  device
>>  + */
>
> Can an NVMe controller have multiple RPMB units? From the NVMe specification: 
> "The controller may support multiple RPMB targets."

That we have to figure, I see NVMe device can have upto 7 RPMB
targets/units

> Can rpmb_dev_find_by_device() be used if multiple RPMB units are associated 
> with a single controller?

That's not finalised yet, but we some ideas from Optee folks on the other 
replies.

>>  +/**
>>  + * rpmb_dev_register - register RPMB partition with the RPMB subsystem
>>  + * @dev: storage device of the rpmb device
>>  + * @target: RPMB target/region within the physical device
>>  + * @ops: device specific operations
>>  + *
>>  + * Return: a pointer to rpmb device
>>  + */
>>  +struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target,
>>  +				   const struct rpmb_ops *ops)
>>  +{
>>  +	struct rpmb_dev *rdev;
>>  +	int id;
>>  +	int ret;
>>  +
>>  +	if (!dev || !ops)
>>  +		return ERR_PTR(-EINVAL);
>>  +
>>  +	if (!ops->program_key)
>>  +		return ERR_PTR(-EINVAL);
>>  +
>>  +	if (!ops->get_capacity)
>>  +		return ERR_PTR(-EINVAL);
>>  +
>>  +	if (!ops->get_write_counter)
>>  +		return ERR_PTR(-EINVAL);
>>  +
>>  +	if (!ops->write_blocks)
>>  +		return ERR_PTR(-EINVAL);
>>  +
>>  +	if (!ops->read_blocks)
>>  +		return ERR_PTR(-EINVAL);
>>  +
>>  +	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
>>  +	if (!rdev)
>>  +		return ERR_PTR(-ENOMEM);
>>  +
>>  +	id = ida_simple_get(&rpmb_ida, 0, 0, GFP_KERNEL);
>>  +	if (id < 0) {
>>  +		ret = id;
>>  +		goto exit;
>>  +	}
>>  +
>>  +	mutex_init(&rdev->lock);
>>  +	rdev->ops = ops;
>>  +	rdev->id = id;
>>  +	rdev->target = target;
>>  +
>>  +	dev_set_name(&rdev->dev, "rpmb%d", id);
>>  +	rdev->dev.class = &rpmb_class;
>>  +	rdev->dev.parent = dev;
>>  +
>>  +	rpmb_cdev_prepare(rdev);
>>  +
>>  +	ret = device_register(&rdev->dev);
>>  +	if (ret)
>>  +		goto exit;
>>  +
>>  +	rpmb_cdev_add(rdev);
>>  +
>>  +	dev_dbg(&rdev->dev, "registered device\n");
>>  +
>>  +	return rdev;
>>  +
>>  +exit:
>>  +	if (id >= 0)
>>  +		ida_simple_remove(&rpmb_ida, id);
>>  +	kfree(rdev);
>>  +	return ERR_PTR(ret);
>>  +}
>
> How is user space software supposed to map an NVMe RPMB target ID to an RPMB 
> device name?

I am not sure, this driver aims to provide in kernel RPMB access APIs, 
user space support may be added later on, but i will look if the current 
RFC version has any implication on future user-space support.

>>  +MODULE_AUTHOR("Intel Corporation");
>
> Shouldn't this be the name of a person instead of the name of a company?
>

Thanks, I will address that in next iteration.

Please keep posted your reviews and feedback.

Best Regards,
Shyam