Re: [PATCH 1/8] drivers: rollback: Add rollback devices to driver model

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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, seanedmond@linux.microsoft.com wrote:
> From: Stephen Carlson <stcarlso@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 <stcarlso@linux.microsoft.com>
> Signed-off-by: Sean Edmond <seanedmond@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 <stcarlso@linux.microsoft.com>
> +M:	Sean Edmond <seanedmond@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.anderson@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 <stcarlso@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