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

[Shyam Saini]

Hi Ulf,

> On Sat, 22 Jul 2023 at 03:41, Shyam Saini
> <shyamsaini@xxxxxxxxxxxxxxxxxxx> wrote:
> > From: Alex Bennée <alex.bennee@xxxxxxxxxx>
> >
> > [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@xxxxxxxxxx/
> > [2] https://lore.kernel.org/linux-mmc/1478548394-8184-2-git-send-email-tomas.winkler@xxxxxxxxx/
> > [3] https://optee.readthedocs.io/en/latest/architecture/secure_storage.html
> >
> > Signed-off-by: Alex Bennée <alex.bennee@xxxxxxxxxx>
> > Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> > Signed-off-by: Shyam Saini <shyamsaini@xxxxxxxxxxxxxxxxxxx>
>
> [...]
>
> > +/**
> > + * 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