Hey David,
Following up here on v6.
On Tue, Jan 04, 2022 at 10:06:45AM +0000, David Brazdil wrote:
> Open Profile for DICE is an open protocol for measured boot compatible
> with the Trusted Computing Group's Device Identifier Composition
> Engine (DICE) specification. The generated Compound Device Identifier
> (CDI) certificates represent the hardware/software combination measured
> by DICE, and can be used for remote attestation and sealing.
>
> Add a driver that exposes reserved memory regions populated by firmware
> with DICE CDIs and exposes them to userspace via a character device.
>
> Userspace obtains the memory region's size from read() and calls mmap()
> to create a mapping of the memory region in its address space. The
> mapping is not allowed to be write+shared, giving userspace a guarantee
> that the data were not overwritten by another process.
>
> Userspace can also call write(), which triggers a wipe of the DICE data
> by the driver. Because both the kernel and userspace mappings use
> write-combine semantics, all clients observe the memory as zeroed after
> the syscall has returned.
>
> Acked-by: Rob Herring <robh@xxxxxxxxxx>
> Cc: Andrew Scull <ascull@xxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Signed-off-by: David Brazdil <dbrazdil@xxxxxxxxxx>
> ---
> drivers/misc/Kconfig | 12 +++
> drivers/misc/Makefile | 1 +
> drivers/misc/open-dice.c | 188 +++++++++++++++++++++++++++++++++++++++
> drivers/of/platform.c | 1 +
> 4 files changed, 202 insertions(+)
> create mode 100644 drivers/misc/open-dice.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0f5a49fc7c9e..a2b26426efba 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -470,6 +470,18 @@ config HISI_HIKEY_USB
> switching between the dual-role USB-C port and the USB-A host ports
> using only one USB controller.
>
> +config OPEN_DICE
> + tristate "Open Profile for DICE driver"
> + depends on OF_RESERVED_MEM
> + help
> + This driver exposes a DICE reserved memory region to userspace via
> + a character device. The memory region contains Compound Device
> + Identifiers (CDIs) generated by firmware as an output of DICE
> + measured boot flow. Userspace can use CDIs for remote attestation
> + and sealing.
> +
> + If unsure, say N.
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index a086197af544..70e800e9127f 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_UACCE) += uacce/
> obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
> obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
> obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.o
> +obj-$(CONFIG_OPEN_DICE) += open-dice.o
> diff --git a/drivers/misc/open-dice.c b/drivers/misc/open-dice.c
> new file mode 100644
> index 000000000000..f1819f951173
> --- /dev/null
> +++ b/drivers/misc/open-dice.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 - Google LLC
> + * Author: David Brazdil <dbrazdil@xxxxxxxxxx>
> + *
> + * Driver for Open Profile for DICE.
> + *
> + * This driver takes ownership of a reserved memory region containing data
> + * generated by the Open Profile for DICE measured boot protocol. The memory
> + * contents are not interpreted by the kernel but can be mapped into a userspace
> + * process via a misc device. Userspace can also request a wipe of the memory.
> + *
> + * Userspace can access the data with (w/o error handling):
> + *
> + * fd = open("/dev/open-dice0", O_RDWR);
> + * read(fd, &size, sizeof(unsigned long));
> + * data = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
> + * write(fd, NULL, 0); // wipe
> + * close(fd);
> + */
> +
> +#include <linux/io.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME "open-dice"
> +
> +struct open_dice_drvdata {
> + spinlock_t lock;
> + char name[16];
> + struct reserved_mem *rmem;
> + struct miscdevice misc;
> +};
> +
> +static inline struct open_dice_drvdata *to_open_dice_drvdata(struct file *filp)
> +{
> + return container_of(filp->private_data, struct open_dice_drvdata, misc);
> +}
> +
> +static int open_dice_wipe(struct open_dice_drvdata *drvdata)
> +{
> + void *kaddr;
> +
> + spin_lock(&drvdata->lock);
> + kaddr = devm_memremap(drvdata->misc.this_device, drvdata->rmem->base,
> + drvdata->rmem->size, MEMREMAP_WC);
What's the plan here if devm_memremap sleeps while you're holding the spinlock?
> + if (IS_ERR(kaddr)) {
> + spin_unlock(&drvdata->lock);
> + return PTR_ERR(kaddr);
> + }
> +
> + memset(kaddr, 0, drvdata->rmem->size);
> + devm_memunmap(drvdata->misc.this_device, kaddr);
> + spin_unlock(&drvdata->lock);
> + return 0;
> +}
> +
> +/*
> + * Copies the size of the reserved memory region to the user-provided buffer.
> + */
> +static ssize_t open_dice_read(struct file *filp, char __user *ptr, size_t len,
> + loff_t *off)
> +{
> + unsigned long val = to_open_dice_drvdata(filp)->rmem->size;
> +
> + return simple_read_from_buffer(ptr, len, off, &val, sizeof(val));
> +}
> +
> +/*
> + * Triggers a wipe of the reserved memory region. The user-provided pointer
> + * is never dereferenced.
> + */
> +static ssize_t open_dice_write(struct file *filp, const char __user *ptr,
> + size_t len, loff_t *off)
> +{
> + if (open_dice_wipe(to_open_dice_drvdata(filp)))
> + return -EIO;
> +
> + /* Consume the input buffer. */
> + return len;
> +}
> +
> +/*
> + * Creates a mapping of the reserved memory region in user address space.
> + */
> +static int open_dice_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> + struct open_dice_drvdata *drvdata = to_open_dice_drvdata(filp);
> +
> + /* Do not allow userspace to modify the underlying data. */
> + if ((vma->vm_flags & VM_WRITE) && (vma->vm_flags & VM_SHARED))
> + return -EPERM;
> +
> + /* Create write-combine mapping so all clients observe a wipe. */
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> + vma->vm_flags |= VM_DONTCOPY | VM_DONTDUMP;
I think we need to clear VM_MAYWRITE here too, otherwise what prevents a user
(that opened the file for write as well) from later adding VM_WRITE to the vma
by calling mprotect?
> + return vm_iomap_memory(vma, drvdata->rmem->base, drvdata->rmem->size);
> +}
> +
> +static const struct file_operations open_dice_fops = {
> + .owner = THIS_MODULE,
> + .read = open_dice_read,
> + .write = open_dice_write,
> + .mmap = open_dice_mmap,
> +};
> +
> +static int __init open_dice_probe(struct platform_device *pdev)
> +{
> + static unsigned int dev_idx;
> + struct device *dev = &pdev->dev;
> + struct reserved_mem *rmem;
> + struct open_dice_drvdata *drvdata;
> + int ret;
> +
> + rmem = of_reserved_mem_lookup(dev->of_node);
> + if (!rmem) {
> + dev_err(dev, "failed to lookup reserved memory\n");
> + return -EINVAL;
> + }
> +
> + if (!rmem->size || (rmem->size > ULONG_MAX)) {
> + dev_err(dev, "invalid memory region size\n");
> + return -EINVAL;
> + }
> +
> + if (!PAGE_ALIGNED(rmem->base) || !PAGE_ALIGNED(rmem->size)) {
> + dev_err(dev, "memory region must be page-aligned\n");
> + return -EINVAL;
> + }
> +
> + drvdata = devm_kmalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + *drvdata = (struct open_dice_drvdata){
> + .lock = __SPIN_LOCK_UNLOCKED(drvdata->lock),
> + .rmem = rmem,
> + .misc = (struct miscdevice){
> + .parent = dev,
> + .name = drvdata->name,
> + .minor = MISC_DYNAMIC_MINOR,
> + .fops = &open_dice_fops,
> + .mode = 0600,
> + },
> + };
> +
> + /* Index overflow check not needed, misc_register() will fail. */
> + snprintf(drvdata->name, sizeof(drvdata->name), DRIVER_NAME"%u", dev_idx++);
> +
> + ret = misc_register(&drvdata->misc);
> + if (ret) {
> + dev_err(dev, "failed to register misc device '%s': %d\n",
> + drvdata->name, ret);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, drvdata);
> + return 0;
> +}
> +
> +static int open_dice_remove(struct platform_device *pdev)
> +{
As we discussed before, this should never be called, right? If it does, users
can trigger UAF. Should we call BUG or WARN here?
> + struct open_dice_drvdata *drvdata = platform_get_drvdata(pdev);
> +
> + misc_deregister(&drvdata->misc);
> + return 0;
> +}
> +
> +static const struct of_device_id open_dice_of_match[] = {
> + { .compatible = "google,open-dice" },
> + {},
> +};
> +
> +static struct platform_driver open_dice_driver = {
> + .remove = open_dice_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = open_dice_of_match,
> + },
> +};
> +
> +module_platform_driver_probe(open_dice_driver, open_dice_probe);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("David Brazdil <dbrazdil@xxxxxxxxxx>");
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b3faf89744aa..d659ed0be342 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -514,6 +514,7 @@ static const struct of_device_id reserved_mem_matches[] = {
> { .compatible = "qcom,smem" },
> { .compatible = "ramoops" },
> { .compatible = "nvmem-rmem" },
> + { .compatible = "google,open-dice" },
> {}
> };
>
> --
> 2.34.1.448.ga2b2bfdf31-goog
>
