[RFC,2/5] char: rpmb: provide a user space interface

Commit Message

Alex Bennée March 3, 2021, 1:54 p.m. UTC

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 keys used for programming and writing blocks to the device are
key_serial_t handles as provided by the keyctl() interface.

[AJB: here there are two key differences between this and the original
proposal. The first is the dropping of the sequence of preformated
frames in favour of explicit actions. The second is the introduction
of key_serial_t and the keyring API for referencing the key to use]

Signed-off-by: Alex Bennée <[e-mail]>
Cc: Ulf Hansson <[e-mail]>
Cc: Linus  Walleij <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Ilias Apalodimas <[e-mail]>
Cc: Tomas Winkler <[e-mail]>
Cc: Alexander Usyskin <[e-mail]>
Cc: Avri Altman <[e-mail]>
---
 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 MAINTAINERS                                   |   1 +
 drivers/char/rpmb/Kconfig                     |   7 +
 drivers/char/rpmb/Makefile                    |   1 +
 drivers/char/rpmb/cdev.c                      | 246 ++++++++++++++++++
 drivers/char/rpmb/core.c                      |  10 +-
 drivers/char/rpmb/rpmb-cdev.h                 |  17 ++
 include/linux/rpmb.h                          |  10 +
 include/uapi/linux/rpmb.h                     |  68 +++++
 9 files changed, 357 insertions(+), 4 deletions(-)
 create mode 100644 drivers/char/rpmb/cdev.c
 create mode 100644 drivers/char/rpmb/rpmb-cdev.h
 create mode 100644 include/uapi/linux/rpmb.h

Comments

Winkler, Tomas March 4, 2021, 7:01 a.m. UTC | #1

> 
> 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 keys used for programming and writing blocks to the device are
> key_serial_t handles as provided by the keyctl() interface.
> 
> [AJB: here there are two key differences between this and the original
> proposal. The first is the dropping of the sequence of preformated frames in
> favour of explicit actions. The second is the introduction of key_serial_t and
> the keyring API for referencing the key to use]

Putting it gently I'm not sure this is good idea, from the security point of view.
The key has to be possession of the one that signs the frames as they are, it doesn't mean it is linux kernel keyring, it can be other party on different system.
With this approach you will make the other usecases not applicable. It is less then trivial to move key securely from one system to another.
We all wished it can be abstracted more but the frames has to come already signed, and the key material is inside the frame. 
Thanks
Tomas


> 
> Signed-off-by: Alex Bennée <[e-mail]>
> Cc: Ulf Hansson <[e-mail]>
> Cc: Linus  Walleij <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Ilias Apalodimas <[e-mail]>
> Cc: Tomas Winkler <[e-mail]>
> Cc: Alexander Usyskin <[e-mail]>
> Cc: Avri Altman <[e-mail]>
> ---
>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>  MAINTAINERS                                   |   1 +
>  drivers/char/rpmb/Kconfig                     |   7 +
>  drivers/char/rpmb/Makefile                    |   1 +
>  drivers/char/rpmb/cdev.c                      | 246 ++++++++++++++++++
>  drivers/char/rpmb/core.c                      |  10 +-
>  drivers/char/rpmb/rpmb-cdev.h                 |  17 ++
>  include/linux/rpmb.h                          |  10 +
>  include/uapi/linux/rpmb.h                     |  68 +++++
>  9 files changed, 357 insertions(+), 4 deletions(-)  create mode 100644
> drivers/char/rpmb/cdev.c  create mode 100644 drivers/char/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 a4c75a28c839..0ff2d4d81bb0 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -344,6 +344,7 @@ Code  Seq#    Include File
> Comments
>  0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:linux-
> [e-mail]>
>  0xB6  all    linux/fpga-dfl.h
>  0xB7  all    uapi/linux/remoteproc_cdev.h                            <mailto:linux-
> [e-mail]>
> +0xB8  80-8F  uapi/linux/rpmb.h                                       <mailto:linux-
> [e-mail]>
>  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 076f3983526c..c60b41b6e6bd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15374,6 +15374,7 @@ M:	?
>  L:	[e-mail]
>  S:	Supported
>  F:	drivers/char/rpmb/*
> +F:	include/uapi/linux/rpmb.h
>  F:	include/linux/rpmb.h
> 
>  RTL2830 MEDIA DRIVER
> diff --git a/drivers/char/rpmb/Kconfig b/drivers/char/rpmb/Kconfig index
> 431c2823cf70..9068664a399a 100644
> --- a/drivers/char/rpmb/Kconfig
> +++ b/drivers/char/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 && KEYS
> +	help
> +	  Say yes here if you want to access RPMB from user space
> +	  via character device interface /dev/rpmb%d
> diff --git a/drivers/char/rpmb/Makefile b/drivers/char/rpmb/Makefile index
> 24d4752a9a53..f54b3f30514b 100644
> --- a/drivers/char/rpmb/Makefile
> +++ b/drivers/char/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/char/rpmb/cdev.c b/drivers/char/rpmb/cdev.c new file
> mode 100644 index 000000000000..55f66720fd03
> --- /dev/null
> +++ b/drivers/char/rpmb/cdev.c
> @@ -0,0 +1,246 @@
> +// 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.auth_method = rdev->ops->auth_method;
> +	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, key_serial_t
> +__user *k) {
> +	key_serial_t keyid;
> +
> +	if (get_user(keyid, k))
> +		return -EFAULT;
> +	else
> +		return rpmb_program_key(rdev, keyid); }
> +
> +static long rpmb_ioctl_counter_cmd(struct rpmb_dev *rdev, int __user
> +*ptr) {
> +	int count = rpmb_get_write_count(rdev);
> +
> +	if (count > 0)
> +		return put_user(count, ptr);
> +	else
> +		return count;
> +}
> +
> +static long rpmb_ioctl_wblocks_cmd(struct rpmb_dev *rdev,
> +				   struct rpmb_ioc_blocks_cmd __user *ptr) {
> +	struct rpmb_ioc_blocks_cmd wblocks;
> +	int sz;
> +	long ret;
> +	u8 *data;
> +
> +	if (copy_from_user(&wblocks, ptr, sizeof(struct
> rpmb_ioc_blocks_cmd)))
> +		return -EFAULT;
> +
> +	/* Don't write more blocks device supports */
> +	if (wblocks.count > rdev->ops->wr_cnt_max)
> +		return -EINVAL;
> +
> +	sz = wblocks.count * 256;
> +	data = kmalloc(sz, GFP_KERNEL);
> +
> +	if (!data)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(data, wblocks.data, sz))
> +		ret = -EFAULT;
> +	else
> +		ret = rpmb_write_blocks(rdev, wblocks.key, wblocks.addr,
> +wblocks.count, data);
> +
> +	kfree(data);
> +	return ret;
> +}
> +
> +static long rpmb_ioctl_rblocks_cmd(struct rpmb_dev *rdev,
> +				   struct rpmb_ioc_blocks_cmd __user *ptr) {
> +	struct rpmb_ioc_blocks_cmd rblocks;
> +	int sz;
> +	long ret;
> +	u8 *data;
> +
> +	if (copy_from_user(&rblocks, ptr, sizeof(struct
> rpmb_ioc_blocks_cmd)))
> +		return -EFAULT;
> +
> +	if (rblocks.count > rdev->ops->rd_cnt_max)
> +		return -EINVAL;
> +
> +	sz = rblocks.count * 256;
> +	data = kmalloc(sz, GFP_KERNEL);
> +
> +	if (!data)
> +		return -ENOMEM;
> +
> +	ret = rpmb_read_blocks(rdev, rblocks.addr, rblocks.count, data);
> +
> +	if (ret == 0)
> +		ret = copy_to_user(rblocks.data, data, sz);
> +
> +	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/char/rpmb/core.c b/drivers/char/rpmb/core.c index
> a2e21c14986a..e26d605e48e1 100644
> --- a/drivers/char/rpmb/core.c
> +++ b/drivers/char/rpmb/core.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
> 
>  #include <linux/rpmb.h>
> +#include "rpmb-cdev.h"
> 
>  static DEFINE_IDA(rpmb_ida);
> 
> @@ -277,6 +278,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);
> 
> @@ -371,9 +373,6 @@ struct rpmb_dev *rpmb_dev_register(struct device
> *dev, u8 target,
>  	if (!ops->read_blocks)
>  		return ERR_PTR(-EINVAL);
> 
> -	if (ops->type == RPMB_TYPE_ANY || ops->type >
> RPMB_TYPE_MAX)
> -		return ERR_PTR(-EINVAL);
> -
>  	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
>  	if (!rdev)
>  		return ERR_PTR(-ENOMEM);
> @@ -396,6 +395,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;
> @@ -412,11 +413,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/char/rpmb/rpmb-cdev.h b/drivers/char/rpmb/rpmb-
> cdev.h new file mode 100644 index 000000000000..e59ff0c05e9d
> --- /dev/null
> +++ b/drivers/char/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
> 718ba7c91ecd..fe44f60efe31 100644
> --- a/include/linux/rpmb.h
> +++ b/include/linux/rpmb.h
> @@ -8,9 +8,13 @@
> 
>  #include <linux/types.h>
>  #include <linux/device.h>
> +#include <linux/cdev.h>
> +#include <uapi/linux/rpmb.h>
>  #include <linux/kref.h>
>  #include <linux/key.h>
> 
> +#define RPMB_API_VERSION 0x80000001
> +
>  /**
>   * struct rpmb_ops - RPMB ops to be implemented by underlying block
> device
>   *
> @@ -51,6 +55,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 block layer
>   */
>  struct rpmb_dev {
> @@ -58,6 +64,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..3957b785cdd5
> --- /dev/null
> +++ b/include/uapi/linux/rpmb.h
> @@ -0,0 +1,68 @@
> +/* 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 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;
> +} __attribute__((packed));
> +
> +/**
> + * struct rpmb_ioc_blocks_cmd - read/write blocks to/from RPMB
> + *
> + * @keyid: key_serial_t of key to use
> + * @addr: index into device (units of 256B blocks)
> + * @count: number of 256B blocks
> + * @data: pointer to data to write/read  */ struct rpmb_ioc_blocks_cmd
> +{
> +	__s32 key; /* key_serial_t */
> +	__u32 addr;
> +	__u32 count;
> +	__u8 __user *data;
> +} __attribute__((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    _IOW(0xB8, 82, key_serial_t)
> +#define RPMB_IOC_COUNTER_CMD _IOR(0xB8, 84, int) #define
> +RPMB_IOC_WBLOCKS_CMD _IOW(0xB8, 85, struct rpmb_ioc_blocks_cmd)
> #define
> +RPMB_IOC_RBLOCKS_CMD _IOR(0xB8, 86, struct rpmb_ioc_blocks_cmd)
> +
> +#endif /* _UAPI_LINUX_RPMB_H_ */
> --
> 2.20.1

Alex Bennée March 4, 2021, 10:19 a.m. UTC | #2

"Winkler, Tomas" <[e-mail]> writes:

>> 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 keys used for programming and writing blocks to the device are
>> key_serial_t handles as provided by the keyctl() interface.
>> 
>> [AJB: here there are two key differences between this and the original
>> proposal. The first is the dropping of the sequence of preformated frames in
>> favour of explicit actions. The second is the introduction of key_serial_t and
>> the keyring API for referencing the key to use]
>
> Putting it gently I'm not sure this is good idea, from the security point of view.
> The key has to be possession of the one that signs the frames as they are, it doesn't mean it is linux kernel keyring, it can be other party on different system.
> With this approach you will make the other usecases not applicable. It
> is less then trivial to move key securely from one system to another.

OK I can understand the desire for such a use-case but it does constrain
the interface on the kernel with access to the hardware to purely
providing a pipe to the raw hardware while also having to expose the
details of the HW to userspace. Also doesn't this break down after a
PROGRAM_KEY event as the key will have had to traverse into the
"untrusted" kernel?

I wonder if virtio-rpmb may be of help here? You could wrap up up the
front-end in the security domain that has the keys although I don't know
how easy it would be for a backend to work with real hardware?

> We all wished it can be abstracted more but the frames has to come already signed, and the key material is inside the frame. 
> Thanks
> Tomas
>
>
>> 
>> Signed-off-by: Alex Bennée <[e-mail]>
>> Cc: Ulf Hansson <[e-mail]>
>> Cc: Linus  Walleij <[e-mail]>
>> Cc: Arnd Bergmann <[e-mail]>
>> Cc: Ilias Apalodimas <[e-mail]>
>> Cc: Tomas Winkler <[e-mail]>
>> Cc: Alexander Usyskin <[e-mail]>
>> Cc: Avri Altman <[e-mail]>
>> ---
>>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>>  MAINTAINERS                                   |   1 +
>>  drivers/char/rpmb/Kconfig                     |   7 +
>>  drivers/char/rpmb/Makefile                    |   1 +
>>  drivers/char/rpmb/cdev.c                      | 246 ++++++++++++++++++
>>  drivers/char/rpmb/core.c                      |  10 +-
>>  drivers/char/rpmb/rpmb-cdev.h                 |  17 ++
>>  include/linux/rpmb.h                          |  10 +
>>  include/uapi/linux/rpmb.h                     |  68 +++++
>>  9 files changed, 357 insertions(+), 4 deletions(-)  create mode 100644
>> drivers/char/rpmb/cdev.c  create mode 100644 drivers/char/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 a4c75a28c839..0ff2d4d81bb0 100644
>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> @@ -344,6 +344,7 @@ Code  Seq#    Include File
>> Comments
>>  0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:linux-
>> [e-mail]>
>>  0xB6  all    linux/fpga-dfl.h
>>  0xB7  all    uapi/linux/remoteproc_cdev.h                            <mailto:linux-
>> [e-mail]>
>> +0xB8  80-8F  uapi/linux/rpmb.h                                       <mailto:linux-
>> [e-mail]>
>>  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 076f3983526c..c60b41b6e6bd 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15374,6 +15374,7 @@ M:	?
>>  L:	[e-mail]
>>  S:	Supported
>>  F:	drivers/char/rpmb/*
>> +F:	include/uapi/linux/rpmb.h
>>  F:	include/linux/rpmb.h
>> 
>>  RTL2830 MEDIA DRIVER
>> diff --git a/drivers/char/rpmb/Kconfig b/drivers/char/rpmb/Kconfig index
>> 431c2823cf70..9068664a399a 100644
>> --- a/drivers/char/rpmb/Kconfig
>> +++ b/drivers/char/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 && KEYS
>> +	help
>> +	  Say yes here if you want to access RPMB from user space
>> +	  via character device interface /dev/rpmb%d
>> diff --git a/drivers/char/rpmb/Makefile b/drivers/char/rpmb/Makefile index
>> 24d4752a9a53..f54b3f30514b 100644
>> --- a/drivers/char/rpmb/Makefile
>> +++ b/drivers/char/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/char/rpmb/cdev.c b/drivers/char/rpmb/cdev.c new file
>> mode 100644 index 000000000000..55f66720fd03
>> --- /dev/null
>> +++ b/drivers/char/rpmb/cdev.c
>> @@ -0,0 +1,246 @@
>> +// 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.auth_method = rdev->ops->auth_method;
>> +	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, key_serial_t
>> +__user *k) {
>> +	key_serial_t keyid;
>> +
>> +	if (get_user(keyid, k))
>> +		return -EFAULT;
>> +	else
>> +		return rpmb_program_key(rdev, keyid); }
>> +
>> +static long rpmb_ioctl_counter_cmd(struct rpmb_dev *rdev, int __user
>> +*ptr) {
>> +	int count = rpmb_get_write_count(rdev);
>> +
>> +	if (count > 0)
>> +		return put_user(count, ptr);
>> +	else
>> +		return count;
>> +}
>> +
>> +static long rpmb_ioctl_wblocks_cmd(struct rpmb_dev *rdev,
>> +				   struct rpmb_ioc_blocks_cmd __user *ptr) {
>> +	struct rpmb_ioc_blocks_cmd wblocks;
>> +	int sz;
>> +	long ret;
>> +	u8 *data;
>> +
>> +	if (copy_from_user(&wblocks, ptr, sizeof(struct
>> rpmb_ioc_blocks_cmd)))
>> +		return -EFAULT;
>> +
>> +	/* Don't write more blocks device supports */
>> +	if (wblocks.count > rdev->ops->wr_cnt_max)
>> +		return -EINVAL;
>> +
>> +	sz = wblocks.count * 256;
>> +	data = kmalloc(sz, GFP_KERNEL);
>> +
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	if (copy_from_user(data, wblocks.data, sz))
>> +		ret = -EFAULT;
>> +	else
>> +		ret = rpmb_write_blocks(rdev, wblocks.key, wblocks.addr,
>> +wblocks.count, data);
>> +
>> +	kfree(data);
>> +	return ret;
>> +}
>> +
>> +static long rpmb_ioctl_rblocks_cmd(struct rpmb_dev *rdev,
>> +				   struct rpmb_ioc_blocks_cmd __user *ptr) {
>> +	struct rpmb_ioc_blocks_cmd rblocks;
>> +	int sz;
>> +	long ret;
>> +	u8 *data;
>> +
>> +	if (copy_from_user(&rblocks, ptr, sizeof(struct
>> rpmb_ioc_blocks_cmd)))
>> +		return -EFAULT;
>> +
>> +	if (rblocks.count > rdev->ops->rd_cnt_max)
>> +		return -EINVAL;
>> +
>> +	sz = rblocks.count * 256;
>> +	data = kmalloc(sz, GFP_KERNEL);
>> +
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	ret = rpmb_read_blocks(rdev, rblocks.addr, rblocks.count, data);
>> +
>> +	if (ret == 0)
>> +		ret = copy_to_user(rblocks.data, data, sz);
>> +
>> +	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/char/rpmb/core.c b/drivers/char/rpmb/core.c index
>> a2e21c14986a..e26d605e48e1 100644
>> --- a/drivers/char/rpmb/core.c
>> +++ b/drivers/char/rpmb/core.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/slab.h>
>> 
>>  #include <linux/rpmb.h>
>> +#include "rpmb-cdev.h"
>> 
>>  static DEFINE_IDA(rpmb_ida);
>> 
>> @@ -277,6 +278,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);
>> 
>> @@ -371,9 +373,6 @@ struct rpmb_dev *rpmb_dev_register(struct device
>> *dev, u8 target,
>>  	if (!ops->read_blocks)
>>  		return ERR_PTR(-EINVAL);
>> 
>> -	if (ops->type == RPMB_TYPE_ANY || ops->type >
>> RPMB_TYPE_MAX)
>> -		return ERR_PTR(-EINVAL);
>> -
>>  	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
>>  	if (!rdev)
>>  		return ERR_PTR(-ENOMEM);
>> @@ -396,6 +395,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;
>> @@ -412,11 +413,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/char/rpmb/rpmb-cdev.h b/drivers/char/rpmb/rpmb-
>> cdev.h new file mode 100644 index 000000000000..e59ff0c05e9d
>> --- /dev/null
>> +++ b/drivers/char/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
>> 718ba7c91ecd..fe44f60efe31 100644
>> --- a/include/linux/rpmb.h
>> +++ b/include/linux/rpmb.h
>> @@ -8,9 +8,13 @@
>> 
>>  #include <linux/types.h>
>>  #include <linux/device.h>
>> +#include <linux/cdev.h>
>> +#include <uapi/linux/rpmb.h>
>>  #include <linux/kref.h>
>>  #include <linux/key.h>
>> 
>> +#define RPMB_API_VERSION 0x80000001
>> +
>>  /**
>>   * struct rpmb_ops - RPMB ops to be implemented by underlying block
>> device
>>   *
>> @@ -51,6 +55,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 block layer
>>   */
>>  struct rpmb_dev {
>> @@ -58,6 +64,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..3957b785cdd5
>> --- /dev/null
>> +++ b/include/uapi/linux/rpmb.h
>> @@ -0,0 +1,68 @@
>> +/* 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 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;
>> +} __attribute__((packed));
>> +
>> +/**
>> + * struct rpmb_ioc_blocks_cmd - read/write blocks to/from RPMB
>> + *
>> + * @keyid: key_serial_t of key to use
>> + * @addr: index into device (units of 256B blocks)
>> + * @count: number of 256B blocks
>> + * @data: pointer to data to write/read  */ struct rpmb_ioc_blocks_cmd
>> +{
>> +	__s32 key; /* key_serial_t */
>> +	__u32 addr;
>> +	__u32 count;
>> +	__u8 __user *data;
>> +} __attribute__((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    _IOW(0xB8, 82, key_serial_t)
>> +#define RPMB_IOC_COUNTER_CMD _IOR(0xB8, 84, int) #define
>> +RPMB_IOC_WBLOCKS_CMD _IOW(0xB8, 85, struct rpmb_ioc_blocks_cmd)
>> #define
>> +RPMB_IOC_RBLOCKS_CMD _IOR(0xB8, 86, struct rpmb_ioc_blocks_cmd)
>> +
>> +#endif /* _UAPI_LINUX_RPMB_H_ */
>> --
>> 2.20.1
>

Winkler, Tomas March 4, 2021, 10:34 a.m. UTC | #3

> "Winkler, Tomas" <[e-mail]> writes:
> 
> >> 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 keys used for programming and writing blocks to the device are
> >> key_serial_t handles as provided by the keyctl() interface.
> >>
> >> [AJB: here there are two key differences between this and the
> >> original proposal. The first is the dropping of the sequence of
> >> preformated frames in favour of explicit actions. The second is the
> >> introduction of key_serial_t and the keyring API for referencing the
> >> key to use]
> >
> > Putting it gently I'm not sure this is good idea, from the security point of
> view.
> > The key has to be possession of the one that signs the frames as they are,
> it doesn't mean it is linux kernel keyring, it can be other party on different
> system.
> > With this approach you will make the other usecases not applicable. It
> > is less then trivial to move key securely from one system to another.
> 
> OK I can understand the desire for such a use-case but it does constrain the
> interface on the kernel with access to the hardware to purely providing a
> pipe to the raw hardware while also having to expose the details of the HW
> to userspace. 
This is the use case in Android. The key is in the "trusty" which different os running in a 	 virtual environment. The file storage abstraction is implemented there.
I'm not sure the point of constraining the kernel, can you please elaborate on that.

Also doesn't this break down after a PROGRAM_KEY event as
> the key will have had to traverse into the "untrusted" kernel?

This is one in a life event of the card happening on the manufacturing floor, maybe even not performed on Linux.

> I wonder if virtio-rpmb may be of help here? You could wrap up up the front-
> end in the security domain that has the keys although I don't know how easy
> it would be for a backend to work with real hardware?

I'm open to see any proposal, not sure I can wrap may head about it right now. 

Anyway I was about to send the new round of my code,  but let's come to common ground first. 

Thanks
Tomas

> 
> > We all wished it can be abstracted more but the frames has to come
> already signed, and the key material is inside the frame.
> > Thanks
> > Tomas
> >
> >
> >>
> >> Signed-off-by: Alex Bennée <[e-mail]>
> >> Cc: Ulf Hansson <[e-mail]>
> >> Cc: Linus  Walleij <[e-mail]>
> >> Cc: Arnd Bergmann <[e-mail]>
> >> Cc: Ilias Apalodimas <[e-mail]>
> >> Cc: Tomas Winkler <[e-mail]>
> >> Cc: Alexander Usyskin <[e-mail]>
> >> Cc: Avri Altman <[e-mail]>
> >> ---
> >>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
> >>  MAINTAINERS                                   |   1 +
> >>  drivers/char/rpmb/Kconfig                     |   7 +
> >>  drivers/char/rpmb/Makefile                    |   1 +
> >>  drivers/char/rpmb/cdev.c                      | 246 ++++++++++++++++++
> >>  drivers/char/rpmb/core.c                      |  10 +-
> >>  drivers/char/rpmb/rpmb-cdev.h                 |  17 ++
> >>  include/linux/rpmb.h                          |  10 +
> >>  include/uapi/linux/rpmb.h                     |  68 +++++
> >>  9 files changed, 357 insertions(+), 4 deletions(-)  create mode
> >> 100644 drivers/char/rpmb/cdev.c  create mode 100644
> >> drivers/char/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 a4c75a28c839..0ff2d4d81bb0 100644
> >> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> >> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> >> @@ -344,6 +344,7 @@ Code  Seq#    Include File
> >> Comments
> >>  0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:linux-
> >> [e-mail]>
> >>  0xB6  all    linux/fpga-dfl.h
> >>  0xB7  all    uapi/linux/remoteproc_cdev.h                            <mailto:linux-
> >> [e-mail]>
> >> +0xB8  80-8F  uapi/linux/rpmb.h                                       <mailto:linux-
> >> [e-mail]>
> >>  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
> >> 076f3983526c..c60b41b6e6bd 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -15374,6 +15374,7 @@ M:	?
> >>  L:	[e-mail]
> >>  S:	Supported
> >>  F:	drivers/char/rpmb/*
> >> +F:	include/uapi/linux/rpmb.h
> >>  F:	include/linux/rpmb.h
> >>
> >>  RTL2830 MEDIA DRIVER
> >> diff --git a/drivers/char/rpmb/Kconfig b/drivers/char/rpmb/Kconfig
> >> index 431c2823cf70..9068664a399a 100644
> >> --- a/drivers/char/rpmb/Kconfig
> >> +++ b/drivers/char/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 && KEYS
> >> +	help
> >> +	  Say yes here if you want to access RPMB from user space
> >> +	  via character device interface /dev/rpmb%d
> >> diff --git a/drivers/char/rpmb/Makefile b/drivers/char/rpmb/Makefile
> >> index 24d4752a9a53..f54b3f30514b 100644
> >> --- a/drivers/char/rpmb/Makefile
> >> +++ b/drivers/char/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/char/rpmb/cdev.c b/drivers/char/rpmb/cdev.c new
> >> file mode 100644 index 000000000000..55f66720fd03
> >> --- /dev/null
> >> +++ b/drivers/char/rpmb/cdev.c
> >> @@ -0,0 +1,246 @@
> >> +// 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.auth_method = rdev->ops->auth_method;
> >> +	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, key_serial_t
> >> +__user *k) {
> >> +	key_serial_t keyid;
> >> +
> >> +	if (get_user(keyid, k))
> >> +		return -EFAULT;
> >> +	else
> >> +		return rpmb_program_key(rdev, keyid); }
> >> +
> >> +static long rpmb_ioctl_counter_cmd(struct rpmb_dev *rdev, int __user
> >> +*ptr) {
> >> +	int count = rpmb_get_write_count(rdev);
> >> +
> >> +	if (count > 0)
> >> +		return put_user(count, ptr);
> >> +	else
> >> +		return count;
> >> +}
> >> +
> >> +static long rpmb_ioctl_wblocks_cmd(struct rpmb_dev *rdev,
> >> +				   struct rpmb_ioc_blocks_cmd __user *ptr) {
> >> +	struct rpmb_ioc_blocks_cmd wblocks;
> >> +	int sz;
> >> +	long ret;
> >> +	u8 *data;
> >> +
> >> +	if (copy_from_user(&wblocks, ptr, sizeof(struct
> >> rpmb_ioc_blocks_cmd)))
> >> +		return -EFAULT;
> >> +
> >> +	/* Don't write more blocks device supports */
> >> +	if (wblocks.count > rdev->ops->wr_cnt_max)
> >> +		return -EINVAL;
> >> +
> >> +	sz = wblocks.count * 256;
> >> +	data = kmalloc(sz, GFP_KERNEL);
> >> +
> >> +	if (!data)
> >> +		return -ENOMEM;
> >> +
> >> +	if (copy_from_user(data, wblocks.data, sz))
> >> +		ret = -EFAULT;
> >> +	else
> >> +		ret = rpmb_write_blocks(rdev, wblocks.key, wblocks.addr,
> >> +wblocks.count, data);
> >> +
> >> +	kfree(data);
> >> +	return ret;
> >> +}
> >> +
> >> +static long rpmb_ioctl_rblocks_cmd(struct rpmb_dev *rdev,
> >> +				   struct rpmb_ioc_blocks_cmd __user *ptr) {
> >> +	struct rpmb_ioc_blocks_cmd rblocks;
> >> +	int sz;
> >> +	long ret;
> >> +	u8 *data;
> >> +
> >> +	if (copy_from_user(&rblocks, ptr, sizeof(struct
> >> rpmb_ioc_blocks_cmd)))
> >> +		return -EFAULT;
> >> +
> >> +	if (rblocks.count > rdev->ops->rd_cnt_max)
> >> +		return -EINVAL;
> >> +
> >> +	sz = rblocks.count * 256;
> >> +	data = kmalloc(sz, GFP_KERNEL);
> >> +
> >> +	if (!data)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = rpmb_read_blocks(rdev, rblocks.addr, rblocks.count, data);
> >> +
> >> +	if (ret == 0)
> >> +		ret = copy_to_user(rblocks.data, data, sz);
> >> +
> >> +	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/char/rpmb/core.c b/drivers/char/rpmb/core.c
> >> index
> >> a2e21c14986a..e26d605e48e1 100644
> >> --- a/drivers/char/rpmb/core.c
> >> +++ b/drivers/char/rpmb/core.c
> >> @@ -12,6 +12,7 @@
> >>  #include <linux/slab.h>
> >>
> >>  #include <linux/rpmb.h>
> >> +#include "rpmb-cdev.h"
> >>
> >>  static DEFINE_IDA(rpmb_ida);
> >>
> >> @@ -277,6 +278,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);
> >>
> >> @@ -371,9 +373,6 @@ struct rpmb_dev *rpmb_dev_register(struct
> device
> >> *dev, u8 target,
> >>  	if (!ops->read_blocks)
> >>  		return ERR_PTR(-EINVAL);
> >>
> >> -	if (ops->type == RPMB_TYPE_ANY || ops->type >
> >> RPMB_TYPE_MAX)
> >> -		return ERR_PTR(-EINVAL);
> >> -
> >>  	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
> >>  	if (!rdev)
> >>  		return ERR_PTR(-ENOMEM);
> >> @@ -396,6 +395,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;
> >> @@ -412,11 +413,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/char/rpmb/rpmb-cdev.h b/drivers/char/rpmb/rpmb-
> >> cdev.h new file mode 100644 index 000000000000..e59ff0c05e9d
> >> --- /dev/null
> >> +++ b/drivers/char/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
> >> 718ba7c91ecd..fe44f60efe31 100644
> >> --- a/include/linux/rpmb.h
> >> +++ b/include/linux/rpmb.h
> >> @@ -8,9 +8,13 @@
> >>
> >>  #include <linux/types.h>
> >>  #include <linux/device.h>
> >> +#include <linux/cdev.h>
> >> +#include <uapi/linux/rpmb.h>
> >>  #include <linux/kref.h>
> >>  #include <linux/key.h>
> >>
> >> +#define RPMB_API_VERSION 0x80000001
> >> +
> >>  /**
> >>   * struct rpmb_ops - RPMB ops to be implemented by underlying block
> >> device
> >>   *
> >> @@ -51,6 +55,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 block layer
> >>   */
> >>  struct rpmb_dev {
> >> @@ -58,6 +64,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..3957b785cdd5
> >> --- /dev/null
> >> +++ b/include/uapi/linux/rpmb.h
> >> @@ -0,0 +1,68 @@
> >> +/* 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 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;
> >> +} __attribute__((packed));
> >> +
> >> +/**
> >> + * struct rpmb_ioc_blocks_cmd - read/write blocks to/from RPMB
> >> + *
> >> + * @keyid: key_serial_t of key to use
> >> + * @addr: index into device (units of 256B blocks)
> >> + * @count: number of 256B blocks
> >> + * @data: pointer to data to write/read  */ struct
> >> +rpmb_ioc_blocks_cmd {
> >> +	__s32 key; /* key_serial_t */
> >> +	__u32 addr;
> >> +	__u32 count;
> >> +	__u8 __user *data;
> >> +} __attribute__((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    _IOW(0xB8, 82, key_serial_t)
> >> +#define RPMB_IOC_COUNTER_CMD _IOR(0xB8, 84, int) #define
> >> +RPMB_IOC_WBLOCKS_CMD _IOW(0xB8, 85, struct
> rpmb_ioc_blocks_cmd)
> >> #define
> >> +RPMB_IOC_RBLOCKS_CMD _IOR(0xB8, 86, struct
> rpmb_ioc_blocks_cmd)
> >> +
> >> +#endif /* _UAPI_LINUX_RPMB_H_ */
> >> --
> >> 2.20.1
> >
> 
> --
> Alex Bennée

Alex Bennée March 4, 2021, 5:52 p.m. UTC | #4

Winkler, Tomas <[e-mail]> writes:

>> "Winkler, Tomas" <[e-mail]> writes:
>> 
>> >> 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 keys used for programming and writing blocks to the device are
>> >> key_serial_t handles as provided by the keyctl() interface.
>> >>
>> >> [AJB: here there are two key differences between this and the
>> >> original proposal. The first is the dropping of the sequence of
>> >> preformated frames in favour of explicit actions. The second is the
>> >> introduction of key_serial_t and the keyring API for referencing the
>> >> key to use]
>> >
>> > Putting it gently I'm not sure this is good idea, from the security point of
>> view.
>> > The key has to be possession of the one that signs the frames as they are,
>> it doesn't mean it is linux kernel keyring, it can be other party on different
>> system.
>> > With this approach you will make the other usecases not applicable. It
>> > is less then trivial to move key securely from one system to another.
>> 
>> OK I can understand the desire for such a use-case but it does constrain the
>> interface on the kernel with access to the hardware to purely providing a
>> pipe to the raw hardware while also having to expose the details of the HW
>> to userspace. 
> This is the use case in Android. The key is in the "trusty" which
> different os running in a virtual environment. The file storage
> abstraction is implemented there. I'm not sure the point of
> constraining the kernel, can you please elaborate on that.

Well the kernel is all about abstracting differences not baking in
assumptions. However can I ask a bit more about this security model?

Is the secure enclave just a separate userspace process or is it in a
separate virtual machine? Is it accessible at all by the kernel running
the driver?

The fact that key id is passed down into the kernel doesn't have to
imply the kernel does the final cryptographic operation. In the ARM
world you could make a call to the secure world to do the operation for
you. I note the keyctl() interface already has support for going to
userspace to make queries of the keyring. Maybe what is really needed is
an abstraction for the kernel to delegate the MAC calculation to some other
trusted process that also understands the keyid.

>
> Also doesn't this break down after a PROGRAM_KEY event as
>> the key will have had to traverse into the "untrusted" kernel?
>
> This is one in a life event of the card happening on the manufacturing
> floor, maybe even not performed on Linux.

In an off list conversation it was suggested that maybe the PROGRAM_KEY
ioctl should be disabled for locked down kernels to dissuade production
use of the facility (it is handy for testing though!).

>> I wonder if virtio-rpmb may be of help here? You could wrap up up the front-
>> end in the security domain that has the keys although I don't know how easy
>> it would be for a backend to work with real hardware?
>
> I'm open to see any proposal, not sure I can wrap may head about it right now. 
>
> Anyway I was about to send the new round of my code,  but let's come to common ground first. 
>

OK - I'll see what the others say.

Winkler, Tomas March 4, 2021, 7:54 p.m. UTC | #5

> 
> Winkler, Tomas <[e-mail]> writes:
> 
> >> "Winkler, Tomas" <[e-mail]> writes:
> >>
> >> >> 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 keys used for programming and writing blocks to the device are
> >> >> key_serial_t handles as provided by the keyctl() interface.
> >> >>
> >> >> [AJB: here there are two key differences between this and the
> >> >> original proposal. The first is the dropping of the sequence of
> >> >> preformated frames in favour of explicit actions. The second is
> >> >> the introduction of key_serial_t and the keyring API for
> >> >> referencing the key to use]
> >> >
> >> > Putting it gently I'm not sure this is good idea, from the security
> >> > point of
> >> view.
> >> > The key has to be possession of the one that signs the frames as
> >> > they are,
> >> it doesn't mean it is linux kernel keyring, it can be other party on
> >> different system.
> >> > With this approach you will make the other usecases not applicable.
> >> > It is less then trivial to move key securely from one system to another.
> >>
> >> OK I can understand the desire for such a use-case but it does
> >> constrain the interface on the kernel with access to the hardware to
> >> purely providing a pipe to the raw hardware while also having to
> >> expose the details of the HW to userspace.
> > This is the use case in Android. The key is in the "trusty" which
> > different os running in a virtual environment. The file storage
> > abstraction is implemented there. I'm not sure the point of
> > constraining the kernel, can you please elaborate on that.
> 
> Well the kernel is all about abstracting differences not baking in assumptions.
> However can I ask a bit more about this security model?
> Is the secure enclave just a separate userspace process or is it in a separate
> virtual machine? Is it accessible at all by the kernel running the driver?

It's not an assumption this is working for few years already (https://source.android.com/security/trusty#application_services) 
The model is that you have a trusted environment (TEE)  in which can be in any of the form you described above.
And there is established agreement via the RPMB key that TEE is only entity that can produce content to be stored on RPBM,
The RPMB hardware also ensure that nobody can catch it in the middle and replay that storage event. 

My point is that interface you are suggesting is not covering all possible usages of RPMB, actually usages that are already in place.

> The fact that key id is passed down into the kernel doesn't have to imply the
> kernel does the final cryptographic operation. In the ARM world you could
> make a call to the secure world to do the operation for you. I note the
> keyctl() interface already has support for going to userspace to make queries
> of the keyring.  Maybe what is really needed is an abstraction for the kernel
> to delegate the MAC calculation to some other trusted process that also
> understands the keyid.

Sure but that you want need to make sure that the entity that creates the content has the right to use this specific key, so you will need to create another channel of trust. 
And this trust has to be established somewhere at the manufacturing time. 

> >
> > Also doesn't this break down after a PROGRAM_KEY event as
> >> the key will have had to traverse into the "untrusted" kernel?
> >
> > This is one in a life event of the card happening on the manufacturing
> > floor, maybe even not performed on Linux.
> 
> In an off list conversation it was suggested that maybe the PROGRAM_KEY
> ioctl should be disabled for locked down kernels to dissuade production use
> of the facility (it is handy for testing t

This is really protected by the hardware,  also once you are programming key your platform would be rather sealed already at least it's TEE environment, as this is the other part that knows the key.

> >> I wonder if virtio-rpmb may be of help here? You could wrap up up the
> >> front- end in the security domain that has the keys although I don't
> >> know how easy it would be for a backend to work with real hardware?
> >
> > I'm open to see any proposal, not sure I can wrap may head about it right
> now.
> >
> > Anyway I was about to send the new round of my code,  but let's come to
> common ground first.
> >
> 
> OK - I'll see what the others say.
> 
> --
> Alex Bennée

Arnd Bergmann March 4, 2021, 9:08 p.m. UTC | #6

On Wed, Mar 3, 2021 at 2:54 PM Alex Bennée <[e-mail]> wrote:
>
> +       /* the rpmb is single open! */
> +       if (test_and_set_bit(RPMB_DEV_OPEN, &rdev->status))
> +               return -EBUSY;

open counters on device nodes are fundamentally broken, because
they do not stop you from using dup() or sharing the file descriptor
across a fork. Just remove this.

> +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;
> +}

Similarly, API versions are fundamentally flawed, as the kernel requires
us to keep compatibility with existing user space. Remove this as well.

> +static long rpmb_ioctl_cap_cmd(struct rpmb_dev *rdev,
> +                              struct rpmb_ioc_cap_cmd __user *ptr)
> +{
> +       struct rpmb_ioc_cap_cmd cap;

Better do a memset() here to ensure this does not leak kernel
stack data to user space.


> +static const struct file_operations rpmb_fops = {
> +       .open           = rpmb_open,
> +       .release        = rpmb_release,
> +       .unlocked_ioctl = rpmb_ioctl,
> +       .owner          = THIS_MODULE,
> +       .llseek         = noop_llseek,
> +};

Add

       .compat_ioctl = compat_ptr_ioctl

to make it work for 32-bit user space on 64-bit kernels.

> @@ -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; }

I don't think it's necessary to make the user interface optional,
I'd just always provide these.

>
> +#define RPMB_API_VERSION 0x80000001

Remove this

> + */
> +struct rpmb_ioc_ver_cmd {
> +       __u32 api_version;
> +} __packed;

And this

> +
> +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;
> +} __attribute__((packed));
> +

Remove the packed attribute, it does not change the structure layout but
just makes it less efficient to access on architectures that turn unaligned
loads and stores into byte accesses.

> +/**
> + * struct rpmb_ioc_blocks_cmd - read/write blocks to/from RPMB
> + *
> + * @keyid: key_serial_t of key to use
> + * @addr: index into device (units of 256B blocks)
> + * @count: number of 256B blocks
> + * @data: pointer to data to write/read
> + */
> +struct rpmb_ioc_blocks_cmd {
> +       __s32 key; /* key_serial_t */
> +       __u32 addr;
> +       __u32 count;
> +       __u8 __user *data;
> +} __attribute__((packed));

ioctl structures should generally not have pointers in them. If this can be done
one block at a time, you can have the 256 bytes as part of the structure.

This probably needs a redesign anyway based on Tomas' feedback though.

If you end up needing a pointer, use a __u64 member  with
u64_to_user_ptr() as described in Documentation/driver-api/ioctl.rst

> +#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    _IOW(0xB8, 82, key_serial_t)
> +#define RPMB_IOC_COUNTER_CMD _IOR(0xB8, 84, int)
> +#define RPMB_IOC_WBLOCKS_CMD _IOW(0xB8, 85, struct rpmb_ioc_blocks_cmd)
> +#define RPMB_IOC_RBLOCKS_CMD _IOR(0xB8, 86, struct rpmb_ioc_blocks_cmd)

The last one should be _IOWR(), not _IOR(), since you write the metadata and
read the data.

      Arnd

Arnd Bergmann March 4, 2021, 9:43 p.m. UTC | #7

On Thu, Mar 4, 2021 at 8:54 PM Winkler, Tomas <[e-mail]> wrote:
> > Winkler, Tomas <[e-mail]> writes:
> > >> "Winkler, Tomas" <[e-mail]> writes:
> > >>
> > >> >> 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 keys used for programming and writing blocks to the device are
> > >> >> key_serial_t handles as provided by the keyctl() interface.
> > >> >>
> > >> >> [AJB: here there are two key differences between this and the
> > >> >> original proposal. The first is the dropping of the sequence of
> > >> >> preformated frames in favour of explicit actions. The second is
> > >> >> the introduction of key_serial_t and the keyring API for
> > >> >> referencing the key to use]
> > >> >
> > >> > Putting it gently I'm not sure this is good idea, from the security
> > >> > point of
> > >> view.
> > >> > The key has to be possession of the one that signs the frames as
> > >> > they are,
> > >> it doesn't mean it is linux kernel keyring, it can be other party on
> > >> different system.
> > >> > With this approach you will make the other usecases not applicable.
> > >> > It is less then trivial to move key securely from one system to another.
> > >>
> > >> OK I can understand the desire for such a use-case but it does
> > >> constrain the interface on the kernel with access to the hardware to
> > >> purely providing a pipe to the raw hardware while also having to
> > >> expose the details of the HW to userspace.
> > > This is the use case in Android. The key is in the "trusty" which
> > > different os running in a virtual environment. The file storage
> > > abstraction is implemented there. I'm not sure the point of
> > > constraining the kernel, can you please elaborate on that.
> >
> > Well the kernel is all about abstracting differences not baking in assumptions.
> > However can I ask a bit more about this security model?
> > Is the secure enclave just a separate userspace process or is it in a separate
> > virtual machine? Is it accessible at all by the kernel running the driver?
>
> It's not an assumption this is working for few years already (https://source.android.com/security/trusty#application_services)
> The model is that you have a trusted environment (TEE)  in which can be in any of the form you described above.
> And there is established agreement via the RPMB key that TEE is only entity that can produce content to be stored on RPBM,
> The RPMB hardware also ensure that nobody can catch it in the middle and replay that storage event.
>
> My point is that interface you are suggesting is not covering all possible usages of RPMB, actually usages that are already in place.

It turned out that the application that we (Linaro) need does have the
same requirements and needs to store the key in a TEE, transferring
the message with the MAC into the kernel, rather than keeping the
key stored in user space or kernel.

However, after I had a look at the nvme-rpmb user space implementation,
I found that this is different, and always expects the key to be stored
in a local file:
https://github.com/linux-nvme/nvme-cli/blob/master/nvme-rpmb.c#L878

This both works with the same kernel interface though, as the kernel
would still get the data along with the HMAC, rather than having the key
stored in the kernel, but it does mean that the frame gets passed to
the kernel in a device specific layout, with at least nvme using an incompatible
layout from everything else.

        Arnd

Winkler, Tomas March 5, 2021, 6:31 a.m. UTC | #8

> On Thu, Mar 4, 2021 at 8:54 PM Winkler, Tomas <[e-mail]>
> wrote:
> > > Winkler, Tomas <[e-mail]> writes:
> > > >> "Winkler, Tomas" <[e-mail]> writes:
> > > >>
> > > >> >> 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 keys used for programming and writing blocks to the device
> > > >> >> are key_serial_t handles as provided by the keyctl() interface.
> > > >> >>
> > > >> >> [AJB: here there are two key differences between this and the
> > > >> >> original proposal. The first is the dropping of the sequence
> > > >> >> of preformated frames in favour of explicit actions. The
> > > >> >> second is the introduction of key_serial_t and the keyring API
> > > >> >> for referencing the key to use]
> > > >> >
> > > >> > Putting it gently I'm not sure this is good idea, from the
> > > >> > security point of
> > > >> view.
> > > >> > The key has to be possession of the one that signs the frames
> > > >> > as they are,
> > > >> it doesn't mean it is linux kernel keyring, it can be other party
> > > >> on different system.
> > > >> > With this approach you will make the other usecases not applicable.
> > > >> > It is less then trivial to move key securely from one system to
> another.
> > > >>
> > > >> OK I can understand the desire for such a use-case but it does
> > > >> constrain the interface on the kernel with access to the hardware
> > > >> to purely providing a pipe to the raw hardware while also having
> > > >> to expose the details of the HW to userspace.
> > > > This is the use case in Android. The key is in the "trusty" which
> > > > different os running in a virtual environment. The file storage
> > > > abstraction is implemented there. I'm not sure the point of
> > > > constraining the kernel, can you please elaborate on that.
> > >
> > > Well the kernel is all about abstracting differences not baking in
> assumptions.
> > > However can I ask a bit more about this security model?
> > > Is the secure enclave just a separate userspace process or is it in
> > > a separate virtual machine? Is it accessible at all by the kernel running the
> driver?
> >
> > It's not an assumption this is working for few years already
> > (https://source.android.com/security/trusty#application_services)
> > The model is that you have a trusted environment (TEE)  in which can be in
> any of the form you described above.
> > And there is established agreement via the RPMB key that TEE is only
> > entity that can produce content to be stored on RPBM, The RPMB
> hardware also ensure that nobody can catch it in the middle and replay that
> storage event.
> >
> > My point is that interface you are suggesting is not covering all possible
> usages of RPMB, actually usages that are already in place.
> 
> It turned out that the application that we (Linaro) need does have the same
> requirements and needs to store the key in a TEE, transferring the message
> with the MAC into the kernel, rather than keeping the key stored in user
> space or kernel.
> 
> However, after I had a look at the nvme-rpmb user space implementation, I
> found that this is different, and always expects the key to be stored in a local
> file:
> https://github.com/linux-nvme/nvme-cli/blob/master/nvme-rpmb.c#L878


This doesn't make it very safe

> This both works with the same kernel interface though, as the kernel would
> still get the data along with the HMAC, rather than having the key stored in
> the kernel, but it does mean that the frame gets passed to the kernel in a
> device specific layout, with at least nvme using an incompatible layout from
> everything else.

NVMe is not by JEDEC so this layout is different and there are some changes but the overall storage operations are the same story.
 I do have a solution also for NVME inclusion into the framework. I haven't published that part yet.  
It won't support virtio part,  as this requires some legal work to include that into  virtio spec.

Thanks
Tomas


 
>         Arnd

Patch

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index a4c75a28c839..0ff2d4d81bb0 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -344,6 +344,7 @@  Code  Seq#    Include File                                           Comments
 0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:[e-mail]>
 0xB6  all    linux/fpga-dfl.h
 0xB7  all    uapi/linux/remoteproc_cdev.h                            <mailto:[e-mail]>
+0xB8  80-8F  uapi/linux/rpmb.h                                       <mailto:[e-mail]>
 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 076f3983526c..c60b41b6e6bd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15374,6 +15374,7 @@  M:	?
 L:	[e-mail]
 S:	Supported
 F:	drivers/char/rpmb/*
+F:	include/uapi/linux/rpmb.h
 F:	include/linux/rpmb.h
 
 RTL2830 MEDIA DRIVER
diff --git a/drivers/char/rpmb/Kconfig b/drivers/char/rpmb/Kconfig
index 431c2823cf70..9068664a399a 100644
--- a/drivers/char/rpmb/Kconfig
+++ b/drivers/char/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 && KEYS
+	help
+	  Say yes here if you want to access RPMB from user space
+	  via character device interface /dev/rpmb%d
diff --git a/drivers/char/rpmb/Makefile b/drivers/char/rpmb/Makefile
index 24d4752a9a53..f54b3f30514b 100644
--- a/drivers/char/rpmb/Makefile
+++ b/drivers/char/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/char/rpmb/cdev.c b/drivers/char/rpmb/cdev.c
new file mode 100644
index 000000000000..55f66720fd03
--- /dev/null
+++ b/drivers/char/rpmb/cdev.c
@@ -0,0 +1,246 @@ 
+// 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.auth_method = rdev->ops->auth_method;
+	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, key_serial_t __user *k)
+{
+	key_serial_t keyid;
+
+	if (get_user(keyid, k))
+		return -EFAULT;
+	else
+		return rpmb_program_key(rdev, keyid);
+}
+
+static long rpmb_ioctl_counter_cmd(struct rpmb_dev *rdev, int __user *ptr)
+{
+	int count = rpmb_get_write_count(rdev);
+
+	if (count > 0)
+		return put_user(count, ptr);
+	else
+		return count;
+}
+
+static long rpmb_ioctl_wblocks_cmd(struct rpmb_dev *rdev,
+				   struct rpmb_ioc_blocks_cmd __user *ptr)
+{
+	struct rpmb_ioc_blocks_cmd wblocks;
+	int sz;
+	long ret;
+	u8 *data;
+
+	if (copy_from_user(&wblocks, ptr, sizeof(struct rpmb_ioc_blocks_cmd)))
+		return -EFAULT;
+
+	/* Don't write more blocks device supports */
+	if (wblocks.count > rdev->ops->wr_cnt_max)
+		return -EINVAL;
+
+	sz = wblocks.count * 256;
+	data = kmalloc(sz, GFP_KERNEL);
+
+	if (!data)
+		return -ENOMEM;
+
+	if (copy_from_user(data, wblocks.data, sz))
+		ret = -EFAULT;
+	else
+		ret = rpmb_write_blocks(rdev, wblocks.key, wblocks.addr, wblocks.count, data);
+
+	kfree(data);
+	return ret;
+}
+
+static long rpmb_ioctl_rblocks_cmd(struct rpmb_dev *rdev,
+				   struct rpmb_ioc_blocks_cmd __user *ptr)
+{
+	struct rpmb_ioc_blocks_cmd rblocks;
+	int sz;
+	long ret;
+	u8 *data;
+
+	if (copy_from_user(&rblocks, ptr, sizeof(struct rpmb_ioc_blocks_cmd)))
+		return -EFAULT;
+
+	if (rblocks.count > rdev->ops->rd_cnt_max)
+		return -EINVAL;
+
+	sz = rblocks.count * 256;
+	data = kmalloc(sz, GFP_KERNEL);
+
+	if (!data)
+		return -ENOMEM;
+
+	ret = rpmb_read_blocks(rdev, rblocks.addr, rblocks.count, data);
+
+	if (ret == 0)
+		ret = copy_to_user(rblocks.data, data, sz);
+
+	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/char/rpmb/core.c b/drivers/char/rpmb/core.c
index a2e21c14986a..e26d605e48e1 100644
--- a/drivers/char/rpmb/core.c
+++ b/drivers/char/rpmb/core.c
@@ -12,6 +12,7 @@ 
 #include <linux/slab.h>
 
 #include <linux/rpmb.h>
+#include "rpmb-cdev.h"
 
 static DEFINE_IDA(rpmb_ida);
 
@@ -277,6 +278,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);
 
@@ -371,9 +373,6 @@  struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target,
 	if (!ops->read_blocks)
 		return ERR_PTR(-EINVAL);
 
-	if (ops->type == RPMB_TYPE_ANY || ops->type > RPMB_TYPE_MAX)
-		return ERR_PTR(-EINVAL);
-
 	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
 	if (!rdev)
 		return ERR_PTR(-ENOMEM);
@@ -396,6 +395,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;
@@ -412,11 +413,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/char/rpmb/rpmb-cdev.h b/drivers/char/rpmb/rpmb-cdev.h
new file mode 100644
index 000000000000..e59ff0c05e9d
--- /dev/null
+++ b/drivers/char/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 718ba7c91ecd..fe44f60efe31 100644
--- a/include/linux/rpmb.h
+++ b/include/linux/rpmb.h
@@ -8,9 +8,13 @@ 
 
 #include <linux/types.h>
 #include <linux/device.h>
+#include <linux/cdev.h>
+#include <uapi/linux/rpmb.h>
 #include <linux/kref.h>
 #include <linux/key.h>
 
+#define RPMB_API_VERSION 0x80000001
+
 /**
  * struct rpmb_ops - RPMB ops to be implemented by underlying block device
  *
@@ -51,6 +55,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 block layer
  */
 struct rpmb_dev {
@@ -58,6 +64,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..3957b785cdd5
--- /dev/null
+++ b/include/uapi/linux/rpmb.h
@@ -0,0 +1,68 @@ 
+/* 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 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;
+} __attribute__((packed));
+
+/**
+ * struct rpmb_ioc_blocks_cmd - read/write blocks to/from RPMB
+ *
+ * @keyid: key_serial_t of key to use
+ * @addr: index into device (units of 256B blocks)
+ * @count: number of 256B blocks
+ * @data: pointer to data to write/read
+ */
+struct rpmb_ioc_blocks_cmd {
+	__s32 key; /* key_serial_t */
+	__u32 addr;
+	__u32 count;
+	__u8 __user *data;
+} __attribute__((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    _IOW(0xB8, 82, key_serial_t)
+#define RPMB_IOC_COUNTER_CMD _IOR(0xB8, 84, int)
+#define RPMB_IOC_WBLOCKS_CMD _IOW(0xB8, 85, struct rpmb_ioc_blocks_cmd)
+#define RPMB_IOC_RBLOCKS_CMD _IOR(0xB8, 86, struct rpmb_ioc_blocks_cmd)
+
+#endif /* _UAPI_LINUX_RPMB_H_ */