The user space API is achieved via a number of synchronous IOCTLs.
* RPMB_IOC_VER_CMD - simple versioning API
* RPMB_IOC_CAP_CMD - query of underlying capabilities
* RPMB_IOC_PKEY_CMD - one time programming of access key
* RPMB_IOC_COUNTER_CMD - query the write counter
* RPMB_IOC_WBLOCKS_CMD - write blocks to device
* RPMB_IOC_RBLOCKS_CMD - read blocks from device
The operations which require authenticated frames or will respond with
MAC hashes of nonce filled frames that userspace will need to verify
share a common command frame format. The other operations can be
considered generic and allow for common handling.
[AJB: here the are key difference is the avoiding a single ioctl where
all the frame data is put together by user space. User space is still
the only place where certain operations can be verified due to the
need of a secret key]
Signed-off-by: Alex Bennée <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Ilias Apalodimas <[email protected]>
Cc: Tomas Winkler <[email protected]>
Cc: Alexander Usyskin <[email protected]>
Cc: Avri Altman <[email protected]>
---
v2
- drop the key api stuff
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
MAINTAINERS | 1 +
drivers/rpmb/Kconfig | 7 +
drivers/rpmb/Makefile | 1 +
drivers/rpmb/cdev.c | 309 ++++++++++++++++++
drivers/rpmb/core.c | 7 +-
drivers/rpmb/rpmb-cdev.h | 17 +
include/linux/rpmb.h | 10 +
include/uapi/linux/rpmb.h | 99 ++++++
9 files changed, 451 insertions(+), 1 deletion(-)
create mode 100644 drivers/rpmb/cdev.c
create mode 100644 drivers/rpmb/rpmb-cdev.h
create mode 100644 include/uapi/linux/rpmb.h
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index e6fce2cbd99e..874d01f11caf 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -355,6 +355,7 @@ Code Seq# Include File Comments
0xB6 all linux/fpga-dfl.h
0xB7 all uapi/linux/remoteproc_cdev.h <mailto:[email protected]>
0xB7 all uapi/linux/nsfs.h <mailto:Andrei Vagin <[email protected]>>
+0xB8 80-8F uapi/linux/rpmb.h <mailto:[email protected]>
0xC0 00-0F linux/usb/iowarrior.h
0xCA 00-0F uapi/misc/cxl.h
0xCA 10-2F uapi/misc/ocxl.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ab02b589005..0a744da21817 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16749,6 +16749,7 @@ M: ?
L: [email protected]
S: Supported
F: drivers/rpmb/*
+F: include/uapi/linux/rpmb.h
F: include/linux/rpmb.h
RPMSG TTY DRIVER
diff --git a/drivers/rpmb/Kconfig b/drivers/rpmb/Kconfig
index f2a9ebdc4435..126f336fe9ea 100644
--- a/drivers/rpmb/Kconfig
+++ b/drivers/rpmb/Kconfig
@@ -9,3 +9,10 @@ config RPMB
access RPMB partition.
If unsure, select N.
+
+config RPMB_INTF_DEV
+ bool "RPMB character device interface /dev/rpmbN"
+ depends on RPMB
+ help
+ Say yes here if you want to access RPMB from user space
+ via character device interface /dev/rpmb%d
diff --git a/drivers/rpmb/Makefile b/drivers/rpmb/Makefile
index 24d4752a9a53..f54b3f30514b 100644
--- a/drivers/rpmb/Makefile
+++ b/drivers/rpmb/Makefile
@@ -3,5 +3,6 @@
obj-$(CONFIG_RPMB) += rpmb.o
rpmb-objs += core.o
+rpmb-$(CONFIG_RPMB_INTF_DEV) += cdev.o
ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/rpmb/cdev.c b/drivers/rpmb/cdev.c
new file mode 100644
index 000000000000..d9beeba53432
--- /dev/null
+++ b/drivers/rpmb/cdev.c
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2015 - 2019 Intel Corporation.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/compat.h>
+#include <linux/slab.h>
+#include <linux/capability.h>
+
+#include <linux/rpmb.h>
+
+#include "rpmb-cdev.h"
+
+static dev_t rpmb_devt;
+#define RPMB_MAX_DEVS MINORMASK
+
+#define RPMB_DEV_OPEN 0 /** single open bit (position) */
+
+/**
+ * rpmb_open - the open function
+ *
+ * @inode: pointer to inode structure
+ * @fp: pointer to file structure
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int rpmb_open(struct inode *inode, struct file *fp)
+{
+ struct rpmb_dev *rdev;
+
+ rdev = container_of(inode->i_cdev, struct rpmb_dev, cdev);
+ if (!rdev)
+ return -ENODEV;
+
+ /* the rpmb is single open! */
+ if (test_and_set_bit(RPMB_DEV_OPEN, &rdev->status))
+ return -EBUSY;
+
+ mutex_lock(&rdev->lock);
+
+ fp->private_data = rdev;
+
+ mutex_unlock(&rdev->lock);
+
+ return nonseekable_open(inode, fp);
+}
+
+/**
+ * rpmb_release - the cdev release function
+ *
+ * @inode: pointer to inode structure
+ * @fp: pointer to file structure
+ *
+ * Return: 0 always.
+ */
+static int rpmb_release(struct inode *inode, struct file *fp)
+{
+ struct rpmb_dev *rdev = fp->private_data;
+
+ clear_bit(RPMB_DEV_OPEN, &rdev->status);
+
+ return 0;
+}
+
+static long rpmb_ioctl_ver_cmd(struct rpmb_dev *rdev,
+ struct rpmb_ioc_ver_cmd __user *ptr)
+{
+ struct rpmb_ioc_ver_cmd ver = {
+ .api_version = RPMB_API_VERSION,
+ };
+
+ return copy_to_user(ptr, &ver, sizeof(ver)) ? -EFAULT : 0;
+}
+
+static long rpmb_ioctl_cap_cmd(struct rpmb_dev *rdev,
+ struct rpmb_ioc_cap_cmd __user *ptr)
+{
+ struct rpmb_ioc_cap_cmd cap;
+
+ cap.target = rdev->target;
+ cap.block_size = rdev->ops->block_size;
+ cap.wr_cnt_max = rdev->ops->wr_cnt_max;
+ cap.rd_cnt_max = rdev->ops->rd_cnt_max;
+ cap.capacity = rpmb_get_capacity(rdev);
+ cap.reserved = 0;
+
+ return copy_to_user(ptr, &cap, sizeof(cap)) ? -EFAULT : 0;
+}
+
+static long rpmb_ioctl_pkey_cmd(struct rpmb_dev *rdev, struct rpmb_ioc_reqresp_cmd __user *ptr)
+{
+ struct rpmb_ioc_reqresp_cmd cmd;
+ u8 *request, *resp = NULL;
+ long ret;
+
+ if (copy_from_user(&cmd, ptr, sizeof(struct rpmb_ioc_reqresp_cmd)))
+ return -EFAULT;
+
+ request = kmalloc(cmd.len, GFP_KERNEL);
+
+ if (!request)
+ return -ENOMEM;
+
+ if (cmd.rlen && cmd.response) {
+ resp = kmalloc(cmd.rlen, GFP_KERNEL);
+ if (!resp) {
+ kfree(request);
+ return -ENOMEM;
+ }
+ }
+
+ if (copy_from_user(request, cmd.request, cmd.len))
+ ret = -EFAULT;
+ else
+ ret = rpmb_program_key(rdev, cmd.len, request, cmd.rlen, resp);
+
+ if (!ret)
+ if (copy_to_user(cmd.response, resp, cmd.rlen))
+ ret = -EFAULT;
+
+ kfree(request);
+ kfree(resp);
+
+ return ret;
+}
+
+static long rpmb_ioctl_counter_cmd(struct rpmb_dev *rdev, struct rpmb_ioc_reqresp_cmd __user *ptr)
+{
+ struct rpmb_ioc_reqresp_cmd cmd;
+ u8 *request, *resp = NULL;
+ long count;
+
+ if (copy_from_user(&cmd, ptr, sizeof(struct rpmb_ioc_reqresp_cmd)))
+ return -EFAULT;
+
+ request = kmalloc(cmd.len, GFP_KERNEL);
+
+ if (!request)
+ return -ENOMEM;
+
+ if (cmd.rlen && cmd.response) {
+ resp = kmalloc(cmd.rlen, GFP_KERNEL);
+ if (!resp) {
+ kfree(request);
+ return -ENOMEM;
+ }
+ }
+
+ if (copy_from_user(request, cmd.request, cmd.len)) {
+ count = -EFAULT;
+ } else {
+ count = rpmb_get_write_count(rdev, cmd.len, request, cmd.rlen, resp);
+ if (resp)
+ if (copy_to_user(cmd.response, resp, cmd.rlen))
+ count = -EFAULT;
+ }
+
+ kfree(request);
+ kfree(resp);
+
+ return count;
+}
+
+static long rpmb_ioctl_wblocks_cmd(struct rpmb_dev *rdev,
+ struct rpmb_ioc_reqresp_cmd __user *ptr)
+{
+ struct rpmb_ioc_reqresp_cmd cmd;
+ u8 *data, *resp = NULL;
+
+ long ret;
+
+ if (copy_from_user(&cmd, ptr, sizeof(struct rpmb_ioc_reqresp_cmd)))
+ return -EFAULT;
+
+ data = kmalloc(cmd.len, GFP_KERNEL);
+
+ if (!data)
+ return -ENOMEM;
+
+ if (cmd.rlen && cmd.response) {
+ resp = kmalloc(cmd.rlen, GFP_KERNEL);
+ if (!resp) {
+ kfree(data);
+ return -ENOMEM;
+ }
+ }
+
+ if (copy_from_user(data, cmd.request, cmd.len))
+ ret = -EFAULT;
+ else
+ ret = rpmb_write_blocks(rdev, cmd.len, data, cmd.rlen, resp);
+
+ if (resp)
+ if (copy_to_user(cmd.response, resp, cmd.rlen))
+ ret = -EFAULT;
+
+ kfree(data);
+ kfree(resp);
+
+ return ret;
+}
+
+static long rpmb_ioctl_rblocks_cmd(struct rpmb_dev *rdev,
+ struct rpmb_ioc_rblocks_cmd __user *ptr)
+{
+ struct rpmb_ioc_rblocks_cmd rblocks;
+ long ret;
+ u8 *data;
+
+ if (copy_from_user(&rblocks, ptr, sizeof(struct rpmb_ioc_rblocks_cmd)))
+ return -EFAULT;
+
+ if (rblocks.count > rdev->ops->rd_cnt_max)
+ return -EINVAL;
+
+ if (!rblocks.len || !rblocks.data)
+ return -EINVAL;
+
+ data = kmalloc(rblocks.len, GFP_KERNEL);
+
+ if (!data)
+ return -ENOMEM;
+
+ ret = rpmb_read_blocks(rdev, rblocks.addr, rblocks.count, rblocks.len, data);
+
+ if (ret == 0)
+ ret = copy_to_user(rblocks.data, data, rblocks.len);
+
+ kfree(data);
+ return ret;
+}
+
+/**
+ * rpmb_ioctl - rpmb ioctl dispatcher
+ *
+ * @fp: a file pointer
+ * @cmd: ioctl command RPMB_IOC_SEQ_CMD RPMB_IOC_VER_CMD RPMB_IOC_CAP_CMD
+ * @arg: ioctl data: rpmb_ioc_ver_cmd rpmb_ioc_cap_cmd pmb_ioc_seq_cmd
+ *
+ * Return: 0 on success; < 0 on error
+ */
+static long rpmb_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+ struct rpmb_dev *rdev = fp->private_data;
+ void __user *ptr = (void __user *)arg;
+
+ switch (cmd) {
+ case RPMB_IOC_VER_CMD:
+ return rpmb_ioctl_ver_cmd(rdev, ptr);
+ case RPMB_IOC_CAP_CMD:
+ return rpmb_ioctl_cap_cmd(rdev, ptr);
+ case RPMB_IOC_PKEY_CMD:
+ return rpmb_ioctl_pkey_cmd(rdev, ptr);
+ case RPMB_IOC_COUNTER_CMD:
+ return rpmb_ioctl_counter_cmd(rdev, ptr);
+ case RPMB_IOC_WBLOCKS_CMD:
+ return rpmb_ioctl_wblocks_cmd(rdev, ptr);
+ case RPMB_IOC_RBLOCKS_CMD:
+ return rpmb_ioctl_rblocks_cmd(rdev, ptr);
+ default:
+ dev_err(&rdev->dev, "unsupported ioctl 0x%x.\n", cmd);
+ return -ENOIOCTLCMD;
+ }
+}
+
+static const struct file_operations rpmb_fops = {
+ .open = rpmb_open,
+ .release = rpmb_release,
+ .unlocked_ioctl = rpmb_ioctl,
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+};
+
+void rpmb_cdev_prepare(struct rpmb_dev *rdev)
+{
+ rdev->dev.devt = MKDEV(MAJOR(rpmb_devt), rdev->id);
+ rdev->cdev.owner = THIS_MODULE;
+ cdev_init(&rdev->cdev, &rpmb_fops);
+}
+
+void rpmb_cdev_add(struct rpmb_dev *rdev)
+{
+ cdev_add(&rdev->cdev, rdev->dev.devt, 1);
+}
+
+void rpmb_cdev_del(struct rpmb_dev *rdev)
+{
+ if (rdev->dev.devt)
+ cdev_del(&rdev->cdev);
+}
+
+int __init rpmb_cdev_init(void)
+{
+ int ret;
+
+ ret = alloc_chrdev_region(&rpmb_devt, 0, RPMB_MAX_DEVS, "rpmb");
+ if (ret < 0)
+ pr_err("unable to allocate char dev region\n");
+
+ return ret;
+}
+
+void __exit rpmb_cdev_exit(void)
+{
+ unregister_chrdev_region(rpmb_devt, RPMB_MAX_DEVS);
+}
diff --git a/drivers/rpmb/core.c b/drivers/rpmb/core.c
index 50b358a14db6..969536ba53cc 100644
--- a/drivers/rpmb/core.c
+++ b/drivers/rpmb/core.c
@@ -12,6 +12,7 @@
#include <linux/slab.h>
#include <linux/rpmb.h>
+#include "rpmb-cdev.h"
static DEFINE_IDA(rpmb_ida);
@@ -282,6 +283,7 @@ int rpmb_dev_unregister(struct rpmb_dev *rdev)
return -EINVAL;
mutex_lock(&rdev->lock);
+ rpmb_cdev_del(rdev);
device_del(&rdev->dev);
mutex_unlock(&rdev->lock);
@@ -401,6 +403,8 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target,
if (ret)
goto exit;
+ rpmb_cdev_add(rdev);
+
dev_dbg(&rdev->dev, "registered device\n");
return rdev;
@@ -417,11 +421,12 @@ static int __init rpmb_init(void)
{
ida_init(&rpmb_ida);
class_register(&rpmb_class);
- return 0;
+ return rpmb_cdev_init();
}
static void __exit rpmb_exit(void)
{
+ rpmb_cdev_exit();
class_unregister(&rpmb_class);
ida_destroy(&rpmb_ida);
}
diff --git a/drivers/rpmb/rpmb-cdev.h b/drivers/rpmb/rpmb-cdev.h
new file mode 100644
index 000000000000..e59ff0c05e9d
--- /dev/null
+++ b/drivers/rpmb/rpmb-cdev.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
+/*
+ * Copyright (C) 2015-2018 Intel Corp. All rights reserved
+ */
+#ifdef CONFIG_RPMB_INTF_DEV
+int __init rpmb_cdev_init(void);
+void __exit rpmb_cdev_exit(void);
+void rpmb_cdev_prepare(struct rpmb_dev *rdev);
+void rpmb_cdev_add(struct rpmb_dev *rdev);
+void rpmb_cdev_del(struct rpmb_dev *rdev);
+#else
+static inline int __init rpmb_cdev_init(void) { return 0; }
+static inline void __exit rpmb_cdev_exit(void) {}
+static inline void rpmb_cdev_prepare(struct rpmb_dev *rdev) {}
+static inline void rpmb_cdev_add(struct rpmb_dev *rdev) {}
+static inline void rpmb_cdev_del(struct rpmb_dev *rdev) {}
+#endif /* CONFIG_RPMB_INTF_DEV */
diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
index 4ed5e299623e..3b0731c07528 100644
--- a/include/linux/rpmb.h
+++ b/include/linux/rpmb.h
@@ -8,8 +8,12 @@
#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
*
@@ -54,6 +58,8 @@ struct rpmb_ops {
* @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 {
@@ -61,6 +67,10 @@ struct rpmb_dev {
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;
};
diff --git a/include/uapi/linux/rpmb.h b/include/uapi/linux/rpmb.h
new file mode 100644
index 000000000000..2f4ee7279b1b
--- /dev/null
+++ b/include/uapi/linux/rpmb.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
+/*
+ * Copyright (C) 2015-2018 Intel Corp. All rights reserved
+ * Copyright (C) 2021-2022 Linaro Ltd
+ */
+#ifndef _UAPI_LINUX_RPMB_H_
+#define _UAPI_LINUX_RPMB_H_
+
+#include <linux/types.h>
+
+/**
+ * struct rpmb_ioc_ver_cmd - rpmb api version
+ *
+ * @api_version: rpmb API version.
+ */
+struct rpmb_ioc_ver_cmd {
+ __u32 api_version;
+} __packed;
+
+enum rpmb_auth_method {
+ RPMB_HMAC_ALGO_SHA_256 = 0,
+};
+
+/**
+ * struct rpmb_ioc_cap_cmd - rpmb capabilities
+ *
+ * @target: rpmb target/region within RPMB partition.
+ * @capacity: storage capacity (in units of 128K)
+ * @block_size: storage data block size (in units of 256B)
+ * @wr_cnt_max: maximal number of block that can be written in a single request.
+ * @rd_cnt_max: maximal number of block that can be read in a single request.
+ * @auth_method: authentication method: currently always HMAC_SHA_256
+ * @reserved: reserved to align to 4 bytes.
+ */
+struct rpmb_ioc_cap_cmd {
+ __u16 target;
+ __u16 capacity;
+ __u16 block_size;
+ __u16 wr_cnt_max;
+ __u16 rd_cnt_max;
+ __u16 auth_method;
+ __u16 reserved;
+} __packed;
+
+/**
+ * struct rpmb_ioc_reqresp_cmd - general purpose reqresp
+ *
+ * Most RPMB operations consist of a set of request frames and an
+ * optional response frame. If a response is requested the user must
+ * allocate enough space for the response, otherwise the fields should
+ * be set to 0/NULL.
+ *
+ * It is used for programming the key, reading the counter and writing
+ * blocks to the device. If the frames are malformed they may be
+ * rejected by the underlying driver or the device itself.
+ *
+ * Assuming the transaction succeeds it is still up to user space to
+ * validate the response and check MAC values correspond to the
+ * programmed keys.
+ *
+ * @len: length of write counter request
+ * @request: ptr to device specific request frame
+ * @rlen: length of response frame
+ * @resp: ptr to device specific response frame
+ */
+struct rpmb_ioc_reqresp_cmd {
+ __u32 len;
+ __u8 __user *request;
+ __u32 rlen;
+ __u8 __user *response;
+} __packed;
+
+/**
+ * struct rpmb_ioc_rblocks_cmd - read blocks from RPMB
+ *
+ * @addr: index into device (units of 256B blocks)
+ * @count: number of 256B blocks
+ * @len: length of response frame
+ * @data: block data (in device specific framing)
+ *
+ * Reading blocks from an RPMB device doesn't require any specific
+ * authentication. However the result still needs to be validated by
+ * user space.
+ */
+struct rpmb_ioc_rblocks_cmd {
+ __u32 addr;
+ __u32 count;
+ __u32 len;
+ __u8 __user *data;
+} __packed;
+
+#define RPMB_IOC_VER_CMD _IOR(0xB8, 80, struct rpmb_ioc_ver_cmd)
+#define RPMB_IOC_CAP_CMD _IOR(0xB8, 81, struct rpmb_ioc_cap_cmd)
+#define RPMB_IOC_PKEY_CMD _IOWR(0xB8, 82, struct rpmb_ioc_reqresp_cmd)
+#define RPMB_IOC_COUNTER_CMD _IOWR(0xB8, 84, struct rpmb_ioc_reqresp_cmd)
+#define RPMB_IOC_WBLOCKS_CMD _IOWR(0xB8, 85, struct rpmb_ioc_reqresp_cmd)
+#define RPMB_IOC_RBLOCKS_CMD _IOR(0xB8, 86, struct rpmb_ioc_rblocks_cmd)
+
+#endif /* _UAPI_LINUX_RPMB_H_ */
--
2.30.2
Hello,
On 05.04.22 11:37, Alex Bennée wrote:
> The user space API is achieved via a number of synchronous IOCTLs.
>
> * RPMB_IOC_VER_CMD - simple versioning API
> * RPMB_IOC_CAP_CMD - query of underlying capabilities
> * RPMB_IOC_PKEY_CMD - one time programming of access key
> * RPMB_IOC_COUNTER_CMD - query the write counter
> * RPMB_IOC_WBLOCKS_CMD - write blocks to device
> * RPMB_IOC_RBLOCKS_CMD - read blocks from device
>
> The operations which require authenticated frames or will respond with
> MAC hashes of nonce filled frames that userspace will need to verify
> share a common command frame format. The other operations can be
> considered generic and allow for common handling.
>
> [AJB: here the are key difference is the avoiding a single ioctl where
> all the frame data is put together by user space. User space is still
> the only place where certain operations can be verified due to the
> need of a secret key]
I have less problems to understand this reworked ioctl() interface as I
had with the older one. Nice.
> diff --git a/drivers/rpmb/cdev.c b/drivers/rpmb/cdev.c
> ...
> +static long rpmb_ioctl_cap_cmd(struct rpmb_dev *rdev,
> + struct rpmb_ioc_cap_cmd __user *ptr)
> +{
> + struct rpmb_ioc_cap_cmd cap;
> +
> + cap.target = rdev->target;
> + cap.block_size = rdev->ops->block_size;
> + cap.wr_cnt_max = rdev->ops->wr_cnt_max;
> + cap.rd_cnt_max = rdev->ops->rd_cnt_max;
> + cap.capacity = rpmb_get_capacity(rdev);
> + cap.reserved = 0;
auth_method is still part of the structure but not set. Means arbitrary
data from the stack is copied to user land.
> +
> + return copy_to_user(ptr, &cap, sizeof(cap)) ? -EFAULT : 0;
> +}
> ...
> +/**
> + * struct rpmb_ioc_cap_cmd - rpmb capabilities
> + *
> + * @target: rpmb target/region within RPMB partition.
> + * @capacity: storage capacity (in units of 128K)
> + * @block_size: storage data block size (in units of 256B)
> + * @wr_cnt_max: maximal number of block that can be written in a single request.
> + * @rd_cnt_max: maximal number of block that can be read in a single request.
> + * @auth_method: authentication method: currently always HMAC_SHA_256
> + * @reserved: reserved to align to 4 bytes.
> + */
> +struct rpmb_ioc_cap_cmd {
> + __u16 target;
> + __u16 capacity;
> + __u16 block_size;
> + __u16 wr_cnt_max;
> + __u16 rd_cnt_max;
> + __u16 auth_method;
> + __u16 reserved;
> +} __packed;
> ...+
--
Dipl.-Ing. Harald Mommer
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
Phone: +49 (30) 60 98 540-0 <== Zentrale
Fax: +49 (30) 60 98 540-99
E-Mail: [email protected]
http://www.opensynergy.com
Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah
On Tue, Apr 5, 2022 at 11:37 AM Alex Bennée <[email protected]> wrote:
> +static int rpmb_open(struct inode *inode, struct file *fp)
> +{
> + struct rpmb_dev *rdev;
> +
> + rdev = container_of(inode->i_cdev, struct rpmb_dev, cdev);
> + if (!rdev)
> + return -ENODEV;
> +
> + /* the rpmb is single open! */
> + if (test_and_set_bit(RPMB_DEV_OPEN, &rdev->status))
> + return -EBUSY;
> +
> + mutex_lock(&rdev->lock);
> +
> + fp->private_data = rdev;
> +
> + mutex_unlock(&rdev->lock);
The mutex here doesn't really do anything, as the file structure is not
reachable from any other context.
I'm not sure the single-open protection gains you anything either,
as it does not prevent the file structure to be shared between
processes.
> +static long rpmb_ioctl_ver_cmd(struct rpmb_dev *rdev,
> + struct rpmb_ioc_ver_cmd __user *ptr)
> +{
> + struct rpmb_ioc_ver_cmd ver = {
> + .api_version = RPMB_API_VERSION,
> + };
> +
> + return copy_to_user(ptr, &ver, sizeof(ver)) ? -EFAULT : 0;
> +}
API version ioctls are generally a bad idea, I think this should be skipped.
The way to handle incompatible API changes is to introduce new ioctl
commands.
> +static const struct file_operations rpmb_fops = {
> + .open = rpmb_open,
> + .release = rpmb_release,
> + .unlocked_ioctl = rpmb_ioctl,
> diff --git a/drivers/rpmb/rpmb-cdev.h b/drivers/rpmb/rpmb-cdev.h
> new file mode 100644
> index 000000000000..e59ff0c05e9d
> --- /dev/null
> +++ b/drivers/rpmb/rpmb-cdev.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
> +/*
> + * Copyright (C) 2015-2018 Intel Corp. All rights reserved
> + */
> +#ifdef CONFIG_RPMB_INTF_DEV
> +int __init rpmb_cdev_init(void);
> +void __exit rpmb_cdev_exit(void);
> +void rpmb_cdev_prepare(struct rpmb_dev *rdev);
> +void rpmb_cdev_add(struct rpmb_dev *rdev);
> +void rpmb_cdev_del(struct rpmb_dev *rdev);
> +#else
> +static inline int __init rpmb_cdev_init(void) { return 0; }
> +static inline void __exit rpmb_cdev_exit(void) {}
> +static inline void rpmb_cdev_prepare(struct rpmb_dev *rdev) {}
> +static inline void rpmb_cdev_add(struct rpmb_dev *rdev) {}
> +static inline void rpmb_cdev_del(struct rpmb_dev *rdev) {}
> +#endif /* CONFIG_RPMB_INTF_DEV */
What is the purpose of the fallback? Would you ever build the code without
the interface?
> +/**
> + * struct rpmb_ioc_ver_cmd - rpmb api version
> + *
> + * @api_version: rpmb API version.
> + */
> +struct rpmb_ioc_ver_cmd {
> + __u32 api_version;
> +} __packed;
Best remove this completely.
> +enum rpmb_auth_method {
> + RPMB_HMAC_ALGO_SHA_256 = 0,
> +};
> +
> +/**
> + * struct rpmb_ioc_cap_cmd - rpmb capabilities
> + *
> + * @target: rpmb target/region within RPMB partition.
> + * @capacity: storage capacity (in units of 128K)
> + * @block_size: storage data block size (in units of 256B)
> + * @wr_cnt_max: maximal number of block that can be written in a single request.
> + * @rd_cnt_max: maximal number of block that can be read in a single request.
> + * @auth_method: authentication method: currently always HMAC_SHA_256
> + * @reserved: reserved to align to 4 bytes.
> + */
> +struct rpmb_ioc_cap_cmd {
> + __u16 target;
> + __u16 capacity;
> + __u16 block_size;
> + __u16 wr_cnt_max;
> + __u16 rd_cnt_max;
> + __u16 auth_method;
> + __u16 reserved;
> +} __packed;
I would remove the __packed attribute here. If you want to keep it, then
it needs to have a __aligned() attribute as well.
It seems strange to have an odd number of 16-bit fields in this, maybe
make it two reserved fields to have a multiple of 32-bit.
> + * struct rpmb_ioc_reqresp_cmd - general purpose reqresp
> + *
> + * Most RPMB operations consist of a set of request frames and an
> + * optional response frame. If a response is requested the user must
> + * allocate enough space for the response, otherwise the fields should
> + * be set to 0/NULL.
> + *
> + * It is used for programming the key, reading the counter and writing
> + * blocks to the device. If the frames are malformed they may be
> + * rejected by the underlying driver or the device itself.
> + *
> + * Assuming the transaction succeeds it is still up to user space to
> + * validate the response and check MAC values correspond to the
> + * programmed keys.
> + *
> + * @len: length of write counter request
> + * @request: ptr to device specific request frame
> + * @rlen: length of response frame
> + * @resp: ptr to device specific response frame
> + */
> +struct rpmb_ioc_reqresp_cmd {
> + __u32 len;
> + __u8 __user *request;
> + __u32 rlen;
> + __u8 __user *response;
> +} __packed;
Try to avoid indirect pointers in ioctl structures, they break
compatibility with
32-bit processes. I think you can do this with a variable-length structure that
has the two length fields followed by the actual data. If there is a reasonable
upper bound for the length, it could also just be a fixed-length buffer.
If you end up having to use a pointer, make it a __u64 value and cast between
that and the actual pointer value.
Whatever you do though, don't use misaligned pointers or implicit padding in the
structure. You can have explicit padding by adding an extra __u32 before
a __u64 pointer value, and then remove the __packed attribute, or reorder the
members so each pointer is naturally aligned.
Arnd