Messages in this thread | | | Date | Wed, 16 Aug 2023 16:31:59 -0700 (PDT) | From | Shyam Saini <> | Subject | Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver |
| |
Hi Ulf,
> On Sat, 22 Jul 2023 at 03:41, Shyam Saini > <shyamsaini@linux.microsoft.com> wrote: >> >> From: Alex Bennée <alex.bennee@linaro.org> >> >> [This is patch 1 from [1] Alex's submission and this RPMB layer was >> originally proposed by [2]Thomas Winkler ] >> >> A number of storage technologies support a specialised hardware >> partition designed to be resistant to replay attacks. The underlying >> HW protocols differ but the operations are common. The RPMB partition >> cannot be accessed via standard block layer, but by a set of specific >> commands: WRITE, READ, GET_WRITE_COUNTER, and PROGRAM_KEY. Such a >> partition provides authenticated and replay protected access, hence >> suitable as a secure storage. >> >> The initial aim of this patch is to provide a simple RPMB Driver which >> can be accessed by Linux's optee driver to facilitate fast-path for >> RPMB access to optee OS(secure OS) during the boot time. [1] Currently, >> Optee OS relies on user-tee supplicant to access eMMC RPMB partition. >> >> A TEE device driver can claim the RPMB interface, for example, via >> class_interface_register(). The RPMB driver provides a series of >> operations for interacting with the device. > > I don't quite follow this. More exactly, how will the TEE driver know > what RPMB device it should use?
I don't have complete code to for this yet, but i think OP-TEE driver should register with RPMB subsystem and then we can have eMMC/UFS/NVMe specific implementation for RPMB operations.
Linux optee driver can handle RPMB frames and pass it to RPMB subsystem
[1] U-Boot has mmc specific implementation
I think OPTEE-OS has CFG_RPMB_FS_DEV_ID option CFG_RPMB_FS_DEV_ID=1 for /dev/mmcblk1rpmb, but in case if a system has multiple RPMB devices such as UFS/eMMC/NVMe, one them should be declared as secure storage and optee should access that one only.
Sumit, do you have suggestions for this ?
>> >> * program_key - a one time operation for setting up a new device >> * get_capacity - introspect the device capacity >> * get_write_counter - check the write counter >> * write_blocks - write a series of blocks to the RPMB device >> * read_blocks - read a series of blocks from the RPMB device >> >> The detailed operation of implementing the access is left to the TEE >> device driver itself. >> >> The framing details and HW specific bits (JDEC vs NVME frames) are >> left to the lower level TEE driver to worry about. >> >> Without kernel fast path to RPMB access doesn't work when IMA try to >> extend ftpm's PCR registers. >> >> This fast-path would require additional work in linux optee driver and >> as well as in MMC driver. >> >> [1] https://lore.kernel.org/lkml/20220405093759.1126835-2-alex.bennee@linaro.org/ >> [2] https://lore.kernel.org/linux-mmc/1478548394-8184-2-git-send-email-tomas.winkler@intel.com/ >> [3] https://optee.readthedocs.io/en/latest/architecture/secure_storage.html >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> >> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com> >> > > [...] > >> +/** >> + * rpmb_dev_find_device() - return first matching rpmb device >> + * @data: data for the match function >> + * @match: the matching function >> + * >> + * Return: matching rpmb device or NULL on failure >> + */ >> +static >> +struct rpmb_dev *rpmb_dev_find_device(const void *data, >> + int (*match)(struct device *dev, >> + const void *data)) >> +{ >> + struct device *dev; >> + >> + dev = class_find_device(&rpmb_class, NULL, data, match); >> + >> + return dev ? to_rpmb_dev(dev) : NULL; >> +} >> + >> +struct device_with_target { >> + const struct device *dev; >> + u8 target; >> +}; >> + >> +static int match_by_parent(struct device *dev, const void *data) >> +{ >> + const struct device_with_target *d = data; >> + struct rpmb_dev *rdev = to_rpmb_dev(dev); >> + >> + return (d->dev && dev->parent == d->dev && rdev->target == d->target); >> +} >> + >> +/** >> + * rpmb_dev_find_by_device() - retrieve rpmb device from the parent device >> + * @parent: parent device of the rpmb device >> + * @target: RPMB target/region within the physical device >> + * >> + * Return: NULL if there is no rpmb device associated with the parent device >> + */ >> +struct rpmb_dev *rpmb_dev_find_by_device(struct device *parent, u8 target) >> +{ >> + struct device_with_target t; >> + >> + if (!parent) >> + return NULL; >> + >> + t.dev = parent; >> + t.target = target; >> + >> + return rpmb_dev_find_device(&t, match_by_parent); >> +} >> +EXPORT_SYMBOL_GPL(rpmb_dev_find_by_device); > > Is this what the TEE driver would be calling to find the rpmb device/partition?
yes, that's the idea.
>> + >> +/** >> + * rpmb_dev_unregister() - unregister RPMB partition from the RPMB subsystem >> + * @rdev: the rpmb device to unregister >> + * Return: >> + * * 0 on success >> + * * -EINVAL on wrong parameters >> + */ >> +int rpmb_dev_unregister(struct rpmb_dev *rdev) >> +{ >> + if (!rdev) >> + return -EINVAL; >> + >> + mutex_lock(&rdev->lock); >> + rpmb_cdev_del(rdev); > > I can't find the function above. I guess it should be included as a > part of the patch too?
Sorry for the confusion, this is leftover from original version Will be removed in next iteration.
>> + device_del(&rdev->dev); >> + mutex_unlock(&rdev->lock); >> + >> + rpmb_dev_put(rdev); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(rpmb_dev_unregister); > > [...] > >> +/** >> + * rpmb_dev_register - register RPMB partition with the RPMB subsystem >> + * @dev: storage device of the rpmb device >> + * @target: RPMB target/region within the physical device >> + * @ops: device specific operations >> + * >> + * Return: a pointer to rpmb device >> + */ >> +struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target, >> + const struct rpmb_ops *ops) >> +{ >> + struct rpmb_dev *rdev; >> + int id; >> + int ret; >> + >> + if (!dev || !ops) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->program_key) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->get_capacity) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->get_write_counter) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->write_blocks) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->read_blocks) >> + return ERR_PTR(-EINVAL); >> + >> + rdev = kzalloc(sizeof(*rdev), GFP_KERNEL); >> + if (!rdev) >> + return ERR_PTR(-ENOMEM); >> + >> + id = ida_simple_get(&rpmb_ida, 0, 0, GFP_KERNEL); >> + if (id < 0) { >> + ret = id; >> + goto exit; >> + } >> + >> + mutex_init(&rdev->lock); >> + rdev->ops = ops; >> + rdev->id = id; >> + rdev->target = target; >> + >> + dev_set_name(&rdev->dev, "rpmb%d", id); >> + rdev->dev.class = &rpmb_class; >> + rdev->dev.parent = dev; >> + >> + rpmb_cdev_prepare(rdev); > > Ditto.
same as my last comment >> + >> + ret = device_register(&rdev->dev); >> + if (ret) >> + goto exit; >> + >> + rpmb_cdev_add(rdev); > > Ditto.
same as above.
>> + >> + dev_dbg(&rdev->dev, "registered device\n"); >> + >> + return rdev; >> + >> +exit: >> + if (id >= 0) >> + ida_simple_remove(&rpmb_ida, id); >> + kfree(rdev); >> + return ERR_PTR(ret); >> +} >> +EXPORT_SYMBOL_GPL(rpmb_dev_register); >> + > > [...] >
[1] https://source.denx.de/u-boot/u-boot/-/commit/4853ad3e13e21462a86e09caee4ea27ae68e764b
Best Regards, Shyam
| |