| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b875892c-1777-d84a-987e-1b0d5ac29df@linux.microsoft.com>
Date: Wed, 16 Aug 2023 16:31:59 -0700 (PDT)
From: Shyam Saini <shyamsaini@...ux.microsoft.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
cc: linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org,
op-tee@...ts.trustedfirmware.org, linux-scsi@...r.kernel.org,
Alex Bennée <alex.bennee@...aro.org>,
Tomas Winkler <tomas.winkler@...el.com>,
Linus Walleij <linus.walleij@...aro.org>,
Arnd Bergmann <arnd.bergmann@...aro.org>,
Ilias Apalodimas <ilias.apalodimas@...aro.org>,
Sumit Garg <sumit.garg@...aro.org>,
Tyler Hicks <code@...icks.com>,
"Srivatsa S . Bhat" <srivatsa@...il.mit.edu>,
Paul Moore <paul@...l-moore.com>,
Allen Pais <apais@...ux.microsoft.com>
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@...ux.microsoft.com> wrote:
>>
>> From: Alex Bennée <alex.bennee@...aro.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@...aro.org>
>> Signed-off-by: Tomas Winkler <tomas.winkler@...el.com>
>> Signed-off-by: Shyam Saini <shyamsaini@...ux.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
Powered by blists - more mailing lists