Hi Sean,
Apologies for the very late reply.
Simon, can you have a look since this is mostly the DM part?
On Tue, Sep 12, 2023 at 02:47:24AM -0700, seanedm...@linux.microsoft.com wrote:
> From: Stephen Carlson <stcar...@linux.microsoft.com>
>
> Rollback devices currently implement operations to store an OS
> anti-rollback monotonic counter. Existing devices such as the Trusted
> Platform Module (TPM) already support this operation, but this uclass
> provides abstraction for current and future devices that may support
> different features.
>
> - New Driver Model uclass UCLASS_ROLLBACK.
> - New config CONFIG_DM_ROLLBACK to enable security device support.
> - New driver rollback-sandbox matching "rollback,sandbox", enabled with
> new config CONFIG_ROLLBACK_SANDBOX.
>
> Signed-off-by: Stephen Carlson <stcar...@linux.microsoft.com>
> Signed-off-by: Sean Edmond <seanedm...@microsoft.com>
> ---
> MAINTAINERS | 9 ++++
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/rollback/Kconfig | 25 +++++++++++
> drivers/rollback/Makefile | 6 +++
> drivers/rollback/rollback-sandbox.c | 65 +++++++++++++++++++++++++++++
> drivers/rollback/rollback-uclass.c | 30 +++++++++++++
> include/dm/uclass-id.h | 1 +
> include/rollback.h | 42 +++++++++++++++++++
> 9 files changed, 181 insertions(+)
> create mode 100644 drivers/rollback/Kconfig
> create mode 100644 drivers/rollback/Makefile
> create mode 100644 drivers/rollback/rollback-sandbox.c
> create mode 100644 drivers/rollback/rollback-uclass.c
> create mode 100644 include/rollback.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bf851cffd6..de14724c27 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1438,6 +1438,15 @@ F: cmd/seama.c
> F: doc/usage/cmd/seama.rst
> F: test/cmd/seama.c
>
> +ROLLBACK
> +M: Stephen Carlson <stcar...@linux.microsoft.com>
> +M: Sean Edmond <seanedm...@microsoft.com>
> +S: Maintained
> +F: drivers/rollback/Kconfig
> +F: drivers/rollback/Makefile
> +F: drivers/rollback/rollback-sandbox.c
> +F: drivers/rollback/rollback-uclass.c
> +
> SEMIHOSTING
> R: Sean Anderson <sean.ander...@seco.com>
> S: Orphaned
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index a25f6ae02f..faa7cbb72b 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -116,6 +116,8 @@ source "drivers/rtc/Kconfig"
>
> source "drivers/scsi/Kconfig"
>
> +source "drivers/rollback/Kconfig"
> +
> source "drivers/serial/Kconfig"
>
> source "drivers/smem/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index efc2a4afb2..f6cfb48cb6 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_PCH) += pch/
> obj-$(CONFIG_DM_REBOOT_MODE) += reboot-mode/
> obj-y += rtc/
> obj-y += scsi/
> +obj-y += rollback/
> obj-y += sound/
> obj-y += spmi/
> obj-y += watchdog/
> diff --git a/drivers/rollback/Kconfig b/drivers/rollback/Kconfig
> new file mode 100644
> index 0000000000..31f5a3f56d
> --- /dev/null
> +++ b/drivers/rollback/Kconfig
> @@ -0,0 +1,25 @@
> +config DM_ROLLBACK
> + bool "Support rollback devices with driver model"
> + depends on DM
> + help
> + This option enables support for the rollback uclass which supports
> + devices intended to provide the anti-rollback feature during
> + boot. These devices might encapsulate existing features of TPM
> + or TEE devices, but can also be dedicated security processors
> + implemented in specific hardware.
> +
> +config ROLLBACK_SANDBOX
> + bool "Enable sandbox rollback driver"
> + depends on DM_ROLLBACK
> + help
> + This driver supports a simulated rollback device that uses volatile
> + memory to store secure data and begins uninitialized. This
> + implementation allows OS images with security requirements to be
> + loaded in the sandbox environment.
> +
> +config ROLLBACK_TPM
> + bool "Enable TPM rollback driver"
> + depends on TPM && TPM_V2 && DM_ROLLBACK
> + help
> + This driver supports a rollback device based on existing TPM
> + functionality.
> diff --git a/drivers/rollback/Makefile b/drivers/rollback/Makefile
> new file mode 100644
> index 0000000000..4e7fa46041
> --- /dev/null
> +++ b/drivers/rollback/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# (C) Copyright 2021 Microsoft, Inc.
> +
> +obj-$(CONFIG_DM_ROLLBACK) += rollback-uclass.o
> +obj-$(CONFIG_ROLLBACK_SANDBOX) += rollback-sandbox.o
> diff --git a/drivers/rollback/rollback-sandbox.c
> b/drivers/rollback/rollback-sandbox.c
> new file mode 100644
> index 0000000000..acbe6d2303
> --- /dev/null
> +++ b/drivers/rollback/rollback-sandbox.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 Microsoft, Inc
> + * Written by Stephen Carlson <stcar...@microsoft.com>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <rollback.h>
> +
> +static struct rollback_state {
> + u64 rollback_idx;
> +};
> +
> +static int sb_rollback_idx_get(struct udevice *dev, u64 *rollback_idx)
> +{
> + struct rollback_state *priv = dev_get_priv(dev);
> +
> + if (!rollback_idx)
> + return -EINVAL;
> +
> + *rollback_idx = priv->rollback_idx;
> + return 0;
> +}
> +
> +static int sb_rollback_idx_set(struct udevice *dev, u64 rollback_idx)
> +{
> + struct rollback_state *priv = dev_get_priv(dev);
> + u64 old_rollback_idx;
> +
> + old_rollback_idx = priv->rollback_idx;
Skip the assignment, if (rollback_idx < priv->rollback_idx) is pretty
straight forward to read
> + if (rollback_idx < old_rollback_idx)
> + return -EPERM;
> +
> + priv->rollback_idx = rollback_idx;
> + return 0;
> +}
> +
> +static const struct rollback_ops rollback_sandbox_ops = {
> + .rollback_idx_get = sb_rollback_idx_get,
> + .rollback_idx_set = sb_rollback_idx_set,
> +};
nit, but I prefer
.rollback_idx_get = sb_rollback_idx_get, etc
makes grepping a lot easier
> +
> +static int rollback_sandbox_probe(struct udevice *dev)
> +{
> + struct rollback_state *priv = dev_get_priv(dev);
> +
> + priv->rollback_idx = 0ULL;
Why do you need the integer constant here?
> + return 0;
> +}
> +
> +static const struct udevice_id rollback_sandbox_ids[] = {
> + { .compatible = "sandbox,rollback" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(rollback_sandbox) = {
> + .name = "rollback_sandbox",
> + .id = UCLASS_ROLLBACK,
> + .priv_auto = sizeof(struct rollback_state),
> + .of_match = rollback_sandbox_ids,
> + .probe = rollback_sandbox_probe,
> + .ops = &rollback_sandbox_ops,
> +};
[...]
Thanks
/Ilias
