* [PATCH 0/5] Add anti-rollback validation feature
@ 2023-08-12 0:28 seanedmond
2023-08-12 0:28 ` [PATCH 1/5] drivers: security: Add security devices to driver model seanedmond
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: seanedmond @ 2023-08-12 0:28 UTC (permalink / raw)
To: u-boot; +Cc: sjg, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
From: Sean Edmond <seanedmond@microsoft.com>
Adds Add anti-rollback version protection. Images with an anti-rollback counter
value "arbvn" declared in the FDT will be compared against the current device
anti-rollback counter value, and older images will not pass signature
validation. If the image is newer, the device anti-rollback counter value will
be updated.
The "arbvn" value is stored/retrieved using the newly added security driver.
A "TPM backed" and "sandbox backed" security driver have been provided as examples.
Adds new configs:
- CONFIG_DM_SECURITY : enable security device support
- CONFIG_SECURITY_SANDBOX : enables "sandbox_security" driver
- CONFIG_SECURITY_TPM : Enables "tpm_security" driver
- CONFIG_ARBP : enable enforcement of OS anti-rollback counter during image loading
- CONFIG_FIT_ARBVP_GRACE : adds a one unit grace period to OS anti-rollback protection
Sean Edmond (1):
dm: test: Add a test for security driver
Stephen Carlson (4):
drivers: security: Add security devices to driver model
drivers: security: Add TPM2 implementation of security devices
common: Add OS anti-rollback validation using security devices
common: Add OS anti-rollback grace period
MAINTAINERS | 9 ++
arch/sandbox/dts/test.dts | 8 ++
boot/Kconfig | 19 +++
boot/image-fit-sig.c | 94 +++++++++++++++
boot/image-fit.c | 23 ++++
configs/sandbox_defconfig | 3 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/security/Kconfig | 25 ++++
drivers/security/Makefile | 7 ++
drivers/security/sandbox_security.c | 65 +++++++++++
drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++
drivers/security/security-uclass.c | 30 +++++
include/dm-security.h | 44 +++++++
include/dm/uclass-id.h | 1 +
include/image.h | 4 +
include/tpm-v2.h | 1 +
test/dm/Makefile | 1 +
test/dm/security.c | 78 +++++++++++++
19 files changed, 588 insertions(+)
create mode 100644 drivers/security/Kconfig
create mode 100644 drivers/security/Makefile
create mode 100644 drivers/security/sandbox_security.c
create mode 100644 drivers/security/security-tpm.c
create mode 100644 drivers/security/security-uclass.c
create mode 100644 include/dm-security.h
create mode 100644 test/dm/security.c
--
2.40.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/5] drivers: security: Add security devices to driver model
2023-08-12 0:28 [PATCH 0/5] Add anti-rollback validation feature seanedmond
@ 2023-08-12 0:28 ` seanedmond
2023-08-16 13:14 ` Ilias Apalodimas
2023-08-17 13:41 ` Simon Glass
2023-08-12 0:28 ` [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices seanedmond
` (4 subsequent siblings)
5 siblings, 2 replies; 18+ messages in thread
From: seanedmond @ 2023-08-12 0:28 UTC (permalink / raw)
To: u-boot; +Cc: sjg, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
From: Stephen Carlson <stcarlso@linux.microsoft.com>
Security 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_SECURITY.
- New config CONFIG_DM_SECURITY to enable security device support.
- New driver sandbox_security matching "security,sandbox", enabled with
new config CONFIG_SECURITY_SANDBOX.
Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
---
MAINTAINERS | 8 ++++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/security/Kconfig | 25 +++++++++++
drivers/security/Makefile | 6 +++
drivers/security/sandbox_security.c | 65 +++++++++++++++++++++++++++++
drivers/security/security-uclass.c | 30 +++++++++++++
include/dm-security.h | 44 +++++++++++++++++++
include/dm/uclass-id.h | 1 +
9 files changed, 182 insertions(+)
create mode 100644 drivers/security/Kconfig
create mode 100644 drivers/security/Makefile
create mode 100644 drivers/security/sandbox_security.c
create mode 100644 drivers/security/security-uclass.c
create mode 100644 include/dm-security.h
diff --git a/MAINTAINERS b/MAINTAINERS
index bf851cffd6..73b6943e03 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1438,6 +1438,14 @@ F: cmd/seama.c
F: doc/usage/cmd/seama.rst
F: test/cmd/seama.c
+SECURITY
+M: Stephen Carlson <stcarlso@linux.microsoft.com>
+S: Maintained
+F: drivers/security/Kconfig
+F: drivers/security/Makefile
+F: drivers/security/sandbox_security.c
+F: drivers/security/security-uclass.c
+
SEMIHOSTING
R: Sean Anderson <sean.anderson@seco.com>
S: Orphaned
diff --git a/drivers/Kconfig b/drivers/Kconfig
index a25f6ae02f..95ea614210 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -116,6 +116,8 @@ source "drivers/rtc/Kconfig"
source "drivers/scsi/Kconfig"
+source "drivers/security/Kconfig"
+
source "drivers/serial/Kconfig"
source "drivers/smem/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index efc2a4afb2..b670aae5fd 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 += security/
obj-y += sound/
obj-y += spmi/
obj-y += watchdog/
diff --git a/drivers/security/Kconfig b/drivers/security/Kconfig
new file mode 100644
index 0000000000..f7af5c4e78
--- /dev/null
+++ b/drivers/security/Kconfig
@@ -0,0 +1,25 @@
+config DM_SECURITY
+ bool "Support security devices with driver model"
+ depends on DM
+ help
+ This option enables support for the security uclass which supports
+ devices intended to provide additional security features 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 SECURITY_SANDBOX
+ bool "Enable sandbox security driver"
+ depends on DM_SECURITY
+ help
+ This driver supports a simulated security 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 SECURITY_TPM
+ bool "Enable TPM security driver"
+ depends on TPM && TPM_V2 && DM_SECURITY
+ help
+ This driver supports a security device based on existing TPM
+ functionality.
diff --git a/drivers/security/Makefile b/drivers/security/Makefile
new file mode 100644
index 0000000000..ed10c3f234
--- /dev/null
+++ b/drivers/security/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# (C) Copyright 2021 Microsoft, Inc.
+
+obj-$(CONFIG_DM_SECURITY) += security-uclass.o
+obj-$(CONFIG_SECURITY_SANDBOX) += sandbox_security.o
diff --git a/drivers/security/sandbox_security.c b/drivers/security/sandbox_security.c
new file mode 100644
index 0000000000..bcb817a842
--- /dev/null
+++ b/drivers/security/sandbox_security.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 <dm-security.h>
+
+static struct security_state {
+ u64 arbvn;
+};
+
+static int sb_security_arbvn_get(struct udevice *dev, u64 *arbvn)
+{
+ struct security_state *priv = dev_get_priv(dev);
+
+ if (!arbvn)
+ return -EINVAL;
+
+ *arbvn = priv->arbvn;
+ return 0;
+}
+
+static int sb_security_arbvn_set(struct udevice *dev, u64 arbvn)
+{
+ struct security_state *priv = dev_get_priv(dev);
+ u64 old_arbvn;
+
+ old_arbvn = priv->arbvn;
+ if (arbvn < old_arbvn)
+ return -EPERM;
+
+ priv->arbvn = arbvn;
+ return 0;
+}
+
+static const struct dm_security_ops security_sandbox_ops = {
+ .arbvn_get = sb_security_arbvn_get,
+ .arbvn_set = sb_security_arbvn_set,
+};
+
+static int security_sandbox_probe(struct udevice *dev)
+{
+ struct security_state *priv = dev_get_priv(dev);
+
+ priv->arbvn = 0ULL;
+ return 0;
+}
+
+static const struct udevice_id security_sandbox_ids[] = {
+ { .compatible = "sandbox,security" },
+ { }
+};
+
+U_BOOT_DRIVER(security_sandbox) = {
+ .name = "security_sandbox",
+ .id = UCLASS_SECURITY,
+ .priv_auto = sizeof(struct security_state),
+ .of_match = security_sandbox_ids,
+ .probe = security_sandbox_probe,
+ .ops = &security_sandbox_ops,
+};
diff --git a/drivers/security/security-uclass.c b/drivers/security/security-uclass.c
new file mode 100644
index 0000000000..26790f3130
--- /dev/null
+++ b/drivers/security/security-uclass.c
@@ -0,0 +1,30 @@
+// 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 <dm-security.h>
+
+int dm_security_arbvn_get(struct udevice *dev, uint64_t *arbvn)
+{
+ if (!dev || !arbvn)
+ return -EINVAL;
+
+ return security_get_ops(dev)->arbvn_get(dev, arbvn);
+}
+
+int dm_security_arbvn_set(struct udevice *dev, uint64_t arbvn)
+{
+ if (!dev)
+ return -EINVAL;
+
+ return security_get_ops(dev)->arbvn_set(dev, arbvn);
+}
+
+UCLASS_DRIVER(security) = {
+ .id = UCLASS_SECURITY,
+ .name = "security",
+};
diff --git a/include/dm-security.h b/include/dm-security.h
new file mode 100644
index 0000000000..f71fe5c255
--- /dev/null
+++ b/include/dm-security.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2021 Microsoft, Inc.
+ */
+
+#ifndef _DM_SECURITY_H_
+#define _DM_SECURITY_H_
+
+#include <stdint.h>
+
+/* Access the security operations for a device */
+#define security_get_ops(dev) ((struct dm_security_ops *)(dev)->driver->ops)
+
+/**
+ * dm_security_arbvn_get() Gets the OS anti-roll back version number (ARBVN)
+ *
+ * @dev: Device to check
+ * @arbvn: Location where the ARBVN will be stored on success
+ * @return 0 if OK, -ve on error
+ */
+int dm_security_arbvn_get(struct udevice *dev, uint64_t *arbvn);
+
+/**
+ * dm_security_arbvn_set() Sets the OS anti-roll back version number (ARBVN).
+ * Only succeeds if the new version number is greater than or equal to the
+ * current ARBVN.
+ *
+ * @dev: Device to modify
+ * @arbvn: The new ARBVN value of the image that is loaded
+ * @return 0 if OK, -ve on error
+ */
+int dm_security_arbvn_set(struct udevice *dev, uint64_t arbvn);
+
+/**
+ * struct dm_security_ops - Driver model security operations
+ *
+ * Refer to the functions above for the description of each operation.
+ */
+struct dm_security_ops {
+ int (*arbvn_get)(struct udevice *dev, uint64_t *arbvn);
+ int (*arbvn_set)(struct udevice *dev, uint64_t arbvn);
+};
+
+#endif
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 0432c95c9e..af282a1baa 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -124,6 +124,7 @@ enum uclass_id {
UCLASS_RTC, /* Real time clock device */
UCLASS_SCMI_AGENT, /* Interface with an SCMI server */
UCLASS_SCSI, /* SCSI device */
+ UCLASS_SECURITY, /* Security device */
UCLASS_SERIAL, /* Serial UART */
UCLASS_SIMPLE_BUS, /* Bus with child devices */
UCLASS_SMEM, /* Shared memory interface */
--
2.40.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
2023-08-12 0:28 [PATCH 0/5] Add anti-rollback validation feature seanedmond
2023-08-12 0:28 ` [PATCH 1/5] drivers: security: Add security devices to driver model seanedmond
@ 2023-08-12 0:28 ` seanedmond
2023-08-14 8:39 ` Ilias Apalodimas
2023-08-17 13:41 ` Simon Glass
2023-08-12 0:28 ` [PATCH 3/5] common: Add OS anti-rollback validation using " seanedmond
` (3 subsequent siblings)
5 siblings, 2 replies; 18+ messages in thread
From: seanedmond @ 2023-08-12 0:28 UTC (permalink / raw)
To: u-boot; +Cc: sjg, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
From: Stephen Carlson <stcarlso@linux.microsoft.com>
This implementation of the security uclass driver allows existing TPM2
devices declared in the device tree to be referenced for storing the OS
anti-rollback counter, using the TPM2 non-volatile storage API.
Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
---
MAINTAINERS | 1 +
drivers/security/Makefile | 1 +
drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++++++
include/tpm-v2.h | 1 +
4 files changed, 176 insertions(+)
create mode 100644 drivers/security/security-tpm.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 73b6943e03..257660a847 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1444,6 +1444,7 @@ S: Maintained
F: drivers/security/Kconfig
F: drivers/security/Makefile
F: drivers/security/sandbox_security.c
+F: drivers/security/security-tpm.c
F: drivers/security/security-uclass.c
SEMIHOSTING
diff --git a/drivers/security/Makefile b/drivers/security/Makefile
index ed10c3f234..e81966bb4a 100644
--- a/drivers/security/Makefile
+++ b/drivers/security/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_DM_SECURITY) += security-uclass.o
obj-$(CONFIG_SECURITY_SANDBOX) += sandbox_security.o
+obj-$(CONFIG_SECURITY_TPM) += security-tpm.o
diff --git a/drivers/security/security-tpm.c b/drivers/security/security-tpm.c
new file mode 100644
index 0000000000..9070dd49ac
--- /dev/null
+++ b/drivers/security/security-tpm.c
@@ -0,0 +1,173 @@
+// 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 <dm-security.h>
+#include <tpm-v2.h>
+
+struct security_state {
+ u32 index_arbvn;
+ struct udevice *tpm_dev;
+};
+
+static int tpm_security_init(struct udevice *tpm_dev)
+{
+ int res;
+
+ /* Initialize TPM but allow reuse of existing session */
+ res = tpm_open(tpm_dev);
+ if (res == -EBUSY) {
+ log(UCLASS_SECURITY, LOGL_DEBUG,
+ "Existing TPM session found, reusing\n");
+ } else {
+ if (res) {
+ log(UCLASS_SECURITY, LOGL_ERR,
+ "TPM initialization failed (ret=%d)\n", res);
+ return res;
+ }
+
+ res = tpm2_startup(tpm_dev, TPM2_SU_CLEAR);
+ if (res) {
+ log(UCLASS_SECURITY, LOGL_ERR,
+ "TPM startup failed (ret=%d)\n", res);
+ return res;
+ }
+ }
+
+ return 0;
+}
+
+static int tpm_security_arbvn_get(struct udevice *dev, u64 *arbvn)
+{
+ struct security_state *priv = dev_get_priv(dev);
+ int ret;
+
+ if (!arbvn)
+ return -EINVAL;
+
+ ret = tpm2_nv_read_value(priv->tpm_dev, priv->index_arbvn, arbvn,
+ sizeof(u64));
+ if (ret == TPM2_RC_NV_UNINITIALIZED) {
+ /* Expected if no OS image has been loaded before */
+ log(UCLASS_SECURITY, LOGL_INFO,
+ "No previous OS image, defaulting ARBVN to 0\n");
+ *arbvn = 0ULL;
+ } else if (ret) {
+ log(UCLASS_SECURITY, LOGL_ERR,
+ "Unable to read ARBVN from TPM (ret=%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int tpm_security_arbvn_set(struct udevice *dev, u64 arbvn)
+{
+ struct security_state *priv = dev_get_priv(dev);
+ struct udevice *tpm_dev = priv->tpm_dev;
+ u64 old_arbvn;
+ int ret;
+
+ ret = tpm_security_arbvn_get(dev, &old_arbvn);
+ if (ret)
+ return ret;
+
+ if (arbvn < old_arbvn)
+ return -EPERM;
+
+ if (arbvn > old_arbvn) {
+ ret = tpm2_nv_write_value(tpm_dev, priv->index_arbvn, &arbvn,
+ sizeof(u64));
+ if (ret) {
+ log(UCLASS_SECURITY, LOGL_ERR,
+ "Unable to write ARBVN to TPM (ret=%d)\n", ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static const struct dm_security_ops tpm_security_ops = {
+ .arbvn_get = tpm_security_arbvn_get,
+ .arbvn_set = tpm_security_arbvn_set,
+};
+
+static int tpm_security_probe(struct udevice *dev)
+{
+ struct security_state *priv = dev_get_priv(dev);
+ struct udevice *tpm_dev = priv->tpm_dev;
+ u32 index = priv->index_arbvn;
+ int ret;
+
+ if (!tpm_dev) {
+ log(UCLASS_SECURITY, LOGL_ERR,
+ "TPM device not defined in DTS\n");
+ return -EINVAL;
+ }
+
+ ret = tpm_security_init(tpm_dev);
+ if (ret)
+ return ret;
+
+ ret = tpm2_nv_define_space(tpm_dev, index, sizeof(u64), TPMA_NV_PPREAD |
+ TPMA_NV_PPWRITE | TPMA_NV_PLATFORMCREATE,
+ NULL, 0);
+ /* NV_DEFINED is an expected error if ARBVN already initialized */
+ if (ret == TPM2_RC_NV_DEFINED)
+ log(UCLASS_SECURITY, LOGL_DEBUG,
+ "ARBVN index %u already defined\n", index);
+ else if (ret) {
+ log(UCLASS_SECURITY, LOGL_ERR,
+ "Unable to create ARBVN NV index (ret=%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int tpm_security_remove(struct udevice *dev)
+{
+ struct security_state *priv = dev_get_priv(dev);
+
+ return tpm_close(priv->tpm_dev);
+}
+
+static int tpm_security_ofdata_to_platdata(struct udevice *dev)
+{
+ const u32 phandle = (u32)dev_read_u32_default(dev, "tpm", 0);
+ struct security_state *priv = dev_get_priv(dev);
+ struct udevice *tpm_dev;
+ int ret;
+
+ ret = uclass_get_device_by_phandle_id(UCLASS_TPM, phandle, &tpm_dev);
+ if (ret) {
+ log(UCLASS_SECURITY, LOGL_ERR, "TPM node in DTS is invalid\n");
+ return ret;
+ }
+
+ priv->index_arbvn = (u32)dev_read_u32_default(dev, "arbvn-nv-index", 0);
+ priv->tpm_dev = tpm_dev;
+ return 0;
+}
+
+static const struct udevice_id tpm_security_ids[] = {
+ { .compatible = "tpm,security" },
+ { }
+};
+
+U_BOOT_DRIVER(security_tpm) = {
+ .name = "security_tpm",
+ .id = UCLASS_SECURITY,
+ .priv_auto = sizeof(struct security_state),
+ .of_match = tpm_security_ids,
+ .of_to_plat = tpm_security_ofdata_to_platdata,
+ .probe = tpm_security_probe,
+ .remove = tpm_security_remove,
+ .ops = &tpm_security_ops,
+};
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index 2b6980e441..49bf0f0ba4 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -321,6 +321,7 @@ enum tpm2_return_codes {
TPM2_RC_COMMAND_CODE = TPM2_RC_VER1 + 0x0043,
TPM2_RC_AUTHSIZE = TPM2_RC_VER1 + 0x0044,
TPM2_RC_AUTH_CONTEXT = TPM2_RC_VER1 + 0x0045,
+ TPM2_RC_NV_UNINITIALIZED = TPM2_RC_VER1 + 0x04a,
TPM2_RC_NV_DEFINED = TPM2_RC_VER1 + 0x004c,
TPM2_RC_NEEDS_TEST = TPM2_RC_VER1 + 0x0053,
TPM2_RC_WARN = 0x0900,
--
2.40.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/5] common: Add OS anti-rollback validation using security devices
2023-08-12 0:28 [PATCH 0/5] Add anti-rollback validation feature seanedmond
2023-08-12 0:28 ` [PATCH 1/5] drivers: security: Add security devices to driver model seanedmond
2023-08-12 0:28 ` [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices seanedmond
@ 2023-08-12 0:28 ` seanedmond
2023-08-17 13:41 ` Simon Glass
2023-08-12 0:28 ` [PATCH 4/5] common: Add OS anti-rollback grace period seanedmond
` (2 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: seanedmond @ 2023-08-12 0:28 UTC (permalink / raw)
To: u-boot; +Cc: sjg, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
From: Stephen Carlson <stcarlso@linux.microsoft.com>
New config CONFIG_ARBP to enable enforcement of OS anti-rollback counter
during image loading.
Images with an anti-rollback counter value "arbvn" declared in the FDT will
be compared against the current device anti-rollback counter value, and
older images will not pass signature validation. If the image is newer, the
device anti-rollback counter value will be updated.
Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
---
boot/Kconfig | 9 +++++
boot/image-fit-sig.c | 89 ++++++++++++++++++++++++++++++++++++++++++++
boot/image-fit.c | 23 ++++++++++++
include/image.h | 4 ++
4 files changed, 125 insertions(+)
diff --git a/boot/Kconfig b/boot/Kconfig
index e8fb03b801..e08c274b7c 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -103,6 +103,15 @@ config FIT_CIPHER
Enable the feature of data ciphering/unciphering in the tool mkimage
and in the u-boot support of the FIT image.
+config FIT_ARBP
+ bool "Enable Anti rollback version check for FIT images"
+ depends on FIT_SIGNATURE
+ default n
+ help
+ Enables FIT image anti-rollback protection. This feature is required
+ when a platform needs to retire previous versions of FIT images due to
+ security flaws and prevent devices from being reverted to them.
+
config FIT_VERBOSE
bool "Show verbose messages when FIT images fail"
depends on FIT
diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c
index 12369896fe..bf3b81a3a3 100644
--- a/boot/image-fit-sig.c
+++ b/boot/image-fit-sig.c
@@ -11,6 +11,8 @@
#include <log.h>
#include <malloc.h>
#include <asm/global_data.h>
+#include <dm.h>
+#include <dm-security.h>
DECLARE_GLOBAL_DATA_PTR;
#endif /* !USE_HOSTCC*/
#include <fdt_region.h>
@@ -63,6 +65,39 @@ struct image_region *fit_region_make_list(const void *fit,
return region;
}
+#if !defined(USE_HOSTCC)
+static int fit_image_verify_arbvn(const void *fit, int image_noffset)
+{
+ u64 image_arbvn;
+ u64 plat_arbvn = 0ULL;
+ struct udevice *dev;
+ int ret;
+
+ ret = fit_image_get_arbvn(fit, image_noffset, &image_arbvn);
+ if (ret)
+ return 0;
+
+ ret = uclass_first_device_err(UCLASS_SECURITY, &dev);
+ if (ret)
+ return -ENODEV;
+
+ ret = dm_security_arbvn_get(dev, &plat_arbvn);
+ if (ret)
+ return -EIO;
+
+ if (image_arbvn < plat_arbvn) {
+ return -EPERM;
+ } else if (image_arbvn > plat_arbvn) {
+ ret = dm_security_arbvn_set(dev, image_arbvn);
+ printf(" Updating OS anti-rollback to %llu from %llu\n",
+ image_arbvn, plat_arbvn);
+ return ret;
+ }
+
+ return 0;
+}
+#endif
+
static int fit_image_setup_verify(struct image_sign_info *info,
const void *fit, int noffset,
const void *key_blob, int required_keynode,
@@ -175,6 +210,16 @@ static int fit_image_verify_sig(const void *fit, int image_noffset,
goto error;
}
+#if !defined(USE_HOSTCC)
+ if (FIT_IMAGE_ENABLE_ARBP && verified) {
+ ret = fit_image_verify_arbvn(fit, image_noffset);
+ if (ret) {
+ err_msg = "Anti-rollback verification failed";
+ goto error;
+ }
+ }
+#endif
+
return verified ? 0 : -EPERM;
error:
@@ -385,6 +430,40 @@ static int fit_config_check_sig(const void *fit, int noffset, int conf_noffset,
return 0;
}
+#if !defined(USE_HOSTCC)
+static int fit_config_verify_arbvn(const void *fit, int conf_noffset,
+ int sig_offset)
+{
+ static const char default_list[] = FIT_KERNEL_PROP "\0"
+ FIT_FDT_PROP;
+ int ret, len;
+ const char *prop, *iname, *end;
+ int image_noffset;
+
+ /* If there is "sign-images" property, use that */
+ prop = fdt_getprop(fit, sig_offset, "sign-images", &len);
+ if (!prop) {
+ prop = default_list;
+ len = sizeof(default_list);
+ }
+
+ /* Locate the images */
+ end = prop + len;
+ for (iname = prop; iname < end; iname += strlen(iname) + 1) {
+ image_noffset = fit_conf_get_prop_node(fit, conf_noffset,
+ iname, IH_PHASE_NONE);
+ if (image_noffset < 0)
+ return -ENOENT;
+
+ ret = fit_image_verify_arbvn(fit, image_noffset);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+#endif
+
/**
* fit_config_verify_key() - Verify that a configuration is signed with a key
*
@@ -444,6 +523,16 @@ static int fit_config_verify_key(const void *fit, int conf_noffset,
goto error;
}
+#if !defined(USE_HOSTCC)
+ if (FIT_IMAGE_ENABLE_ARBP && verified) {
+ ret = fit_config_verify_arbvn(fit, conf_noffset, noffset);
+ if (ret) {
+ err_msg = "Anti-rollback verification failed";
+ goto error;
+ }
+ }
+#endif
+
if (verified)
return 0;
diff --git a/boot/image-fit.c b/boot/image-fit.c
index 3cc556b727..d4e324752a 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -1084,6 +1084,29 @@ int fit_image_get_data_and_size(const void *fit, int noffset,
return ret;
}
+/**
+ * fit_image_get_arbvn - get anti-rollback counter
+ * @fit: pointer to the FIT image header
+ * @noffset: component image node offset
+ * @arbvn: holds the arbvn property value
+ *
+ * returns:
+ * 0, on success
+ * -ENOENT if the property could not be found
+ */
+int fit_image_get_arbvn(const void *fit, int noffset, uint64_t *arbvn)
+{
+ const fdt64_t *val;
+
+ val = fdt_getprop(fit, noffset, FIT_ARBVN_PROP, NULL);
+ if (!val)
+ return -ENOENT;
+
+ *arbvn = fdt64_to_cpu(*val);
+
+ return 0;
+}
+
/**
* fit_image_hash_get_algo - get hash algorithm name
* @fit: pointer to the FIT format image header
diff --git a/include/image.h b/include/image.h
index 01a6787d21..497772fb4b 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1024,6 +1024,7 @@ int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
#define FIT_COMP_PROP "compression"
#define FIT_ENTRY_PROP "entry"
#define FIT_LOAD_PROP "load"
+#define FIT_ARBVN_PROP "arbvn"
/* configuration node */
#define FIT_KERNEL_PROP "kernel"
@@ -1105,6 +1106,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
size_t *data_size);
int fit_image_get_data_and_size(const void *fit, int noffset,
const void **data, size_t *size);
+int fit_image_get_arbvn(const void *fit, int noffset, uint64_t *arbvn);
/**
* fit_get_data_node() - Get verified image data for an image
@@ -1389,6 +1391,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
* device
*/
#if defined(USE_HOSTCC)
+# define FIT_IMAGE_ENABLE_ARBP 0
# if defined(CONFIG_FIT_SIGNATURE)
# define IMAGE_ENABLE_SIGN 1
# define FIT_IMAGE_ENABLE_VERIFY 1
@@ -1400,6 +1403,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
#else
# define IMAGE_ENABLE_SIGN 0
# define FIT_IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(FIT_SIGNATURE)
+# define FIT_IMAGE_ENABLE_ARBP CONFIG_IS_ENABLED(FIT_ARBP)
#endif
#ifdef USE_HOSTCC
--
2.40.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/5] common: Add OS anti-rollback grace period
2023-08-12 0:28 [PATCH 0/5] Add anti-rollback validation feature seanedmond
` (2 preceding siblings ...)
2023-08-12 0:28 ` [PATCH 3/5] common: Add OS anti-rollback validation using " seanedmond
@ 2023-08-12 0:28 ` seanedmond
2023-08-17 13:41 ` Simon Glass
2023-08-12 0:28 ` [PATCH 5/5] dm: test: Add a test for security driver seanedmond
2023-08-17 13:41 ` [PATCH 0/5] Add anti-rollback validation feature Simon Glass
5 siblings, 1 reply; 18+ messages in thread
From: seanedmond @ 2023-08-12 0:28 UTC (permalink / raw)
To: u-boot; +Cc: sjg, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
From: Stephen Carlson <stcarlso@microsoft.com>
New config CONFIG_FIT_ARBVP_GRACE to add a one unit grace period to OS
anti-rollback protection, allowing images with anti-rollback counters
exactly one less than the platform value to still be loaded. No update to
the platform anti-rollback counter will be performed in this case.
Signed-off-by: Stephen Carlson <stcarlso@microsoft.com>
---
boot/Kconfig | 10 ++++++++++
boot/image-fit-sig.c | 7 ++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/boot/Kconfig b/boot/Kconfig
index e08c274b7c..cd16bb8e53 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -112,6 +112,16 @@ config FIT_ARBP
when a platform needs to retire previous versions of FIT images due to
security flaws and prevent devices from being reverted to them.
+config FIT_ARBP_GRACE
+ bool "Enable FIT Anti rollback grace period"
+ depends on FIT_ARBP
+ default n
+ help
+ Enables a one unit grace period for FIT image anti-rollback protection,
+ where anti-rollback protection will still accept a FIT image with an
+ anti-rollback version one less than the current number, but will not
+ update the platform anti-rollback counter in that case.
+
config FIT_VERBOSE
bool "Show verbose messages when FIT images fail"
depends on FIT
diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c
index bf3b81a3a3..dc88a4b2cb 100644
--- a/boot/image-fit-sig.c
+++ b/boot/image-fit-sig.c
@@ -70,6 +70,7 @@ static int fit_image_verify_arbvn(const void *fit, int image_noffset)
{
u64 image_arbvn;
u64 plat_arbvn = 0ULL;
+ u64 target_arbvn;
struct udevice *dev;
int ret;
@@ -85,7 +86,11 @@ static int fit_image_verify_arbvn(const void *fit, int image_noffset)
if (ret)
return -EIO;
- if (image_arbvn < plat_arbvn) {
+ target_arbvn = plat_arbvn;
+ /* Calculate target ARBVN, including grace period if enabled */
+ if (CONFIG_IS_ENABLED(FIT_ARBP_GRACE) && plat_arbvn > 0ULL)
+ target_arbvn = plat_arbvn - 1ULL;
+ if (image_arbvn < target_arbvn) {
return -EPERM;
} else if (image_arbvn > plat_arbvn) {
ret = dm_security_arbvn_set(dev, image_arbvn);
--
2.40.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/5] dm: test: Add a test for security driver
2023-08-12 0:28 [PATCH 0/5] Add anti-rollback validation feature seanedmond
` (3 preceding siblings ...)
2023-08-12 0:28 ` [PATCH 4/5] common: Add OS anti-rollback grace period seanedmond
@ 2023-08-12 0:28 ` seanedmond
2023-08-17 13:41 ` Simon Glass
2023-08-17 13:41 ` [PATCH 0/5] Add anti-rollback validation feature Simon Glass
5 siblings, 1 reply; 18+ messages in thread
From: seanedmond @ 2023-08-12 0:28 UTC (permalink / raw)
To: u-boot; +Cc: sjg, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
From: Sean Edmond <seanedmond@microsoft.com>
Adds a test for a sandbox and TPM backed security driver.
Allows for testing of anti-rollback version number get/set API using the
security driver.
Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
---
arch/sandbox/dts/test.dts | 8 ++++
configs/sandbox_defconfig | 3 ++
test/dm/Makefile | 1 +
test/dm/security.c | 78 +++++++++++++++++++++++++++++++++++++++
4 files changed, 90 insertions(+)
create mode 100644 test/dm/security.c
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index f351d5cb84..c87298cd46 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -1263,6 +1263,14 @@
backlight = <&backlight 0 100>;
};
+ security@0 {
+ compatible = "sandbox,security";
+ };
+
+ security@1 {
+ compatible = "tpm,security";
+ };
+
scsi {
compatible = "sandbox,scsi";
sandbox,filepath = "scsi.img";
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 1cd1c2ed7c..546873b049 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -346,3 +346,6 @@ CONFIG_UNIT_TEST=y
CONFIG_UT_TIME=y
CONFIG_UT_DM=y
CONFIG_ARM_FFA_TRANSPORT=y
+CONFIG_DM_SECURITY=y
+CONFIG_SECURITY_SANDBOX=y
+CONFIG_SECURITY_TPM=y
\ No newline at end of file
diff --git a/test/dm/Makefile b/test/dm/Makefile
index 7ed00733c1..d0583c0332 100644
--- a/test/dm/Makefile
+++ b/test/dm/Makefile
@@ -104,6 +104,7 @@ obj-$(CONFIG_DM_RNG) += rng.o
obj-$(CONFIG_DM_RTC) += rtc.o
obj-$(CONFIG_SCMI_FIRMWARE) += scmi.o
obj-$(CONFIG_SCSI) += scsi.o
+obj-$(CONFIG_DM_SECURITY) += security.o
obj-$(CONFIG_DM_SERIAL) += serial.o
obj-$(CONFIG_DM_SPI_FLASH) += sf.o
obj-$(CONFIG_SIMPLE_BUS) += simple-bus.o
diff --git a/test/dm/security.c b/test/dm/security.c
new file mode 100644
index 0000000000..a388a80096
--- /dev/null
+++ b/test/dm/security.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2023 Microsoft Corporation
+ * Written by Sean Edmond <seanedmond@microsoft.com>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <dm-security.h>
+#include <log.h>
+#include <dm/test.h>
+#include <test/test.h>
+#include <test/ut.h>
+
+/*
+ * get_security() - Get a security driver of a given driver name
+ *
+ * @devp: Returns the security device
+ * @driver_name: Driver name to find
+ * Returns: 0 if OK, -ENODEV if not found
+ */
+static int get_security(struct udevice **devp, char *driver_name)
+{
+ struct udevice *dev;
+
+ uclass_foreach_dev_probe(UCLASS_SECURITY, dev) {
+ if (strcmp(dev->driver->name, driver_name) == 0) {
+ *devp = dev;
+ return 0;
+ }
+ }
+
+ return -ENODEV;
+}
+
+/* Basic test of security driver Anti rollback version number read/write */
+static int test_security_arbvn(struct unit_test_state *uts, char *driver_name)
+{
+ struct udevice *dev;
+ uint64_t arbvn;
+
+ /* get the security driver */
+ ut_assertok(get_security(&dev, driver_name));
+
+ /* ensure initial value is 0 */
+ dm_security_arbvn_get(dev, &arbvn);
+ ut_asserteq(0, arbvn);
+
+ /* write 1 and ensure it's read back */
+ dm_security_arbvn_set(dev, 1);
+ dm_security_arbvn_get(dev, &arbvn);
+ ut_asserteq(1, arbvn);
+
+ /* write all ones and ensure it's read back */
+ dm_security_arbvn_set(dev, 0xffffffffffffffffULL);
+ dm_security_arbvn_get(dev, &arbvn);
+ ut_asserteq(0xffffffffffffffffULL, arbvn);
+
+ return 0;
+}
+
+static int dm_test_security_sandbox(struct unit_test_state *uts)
+{
+ ut_assertok(test_security_arbvn(uts, "security_sandbox"));
+
+ return 0;
+}
+
+DM_TEST(dm_test_security_sandbox, UT_TESTF_SCAN_FDT);
+
+static int dm_test_security_tpm(struct unit_test_state *uts)
+{
+ ut_assertok(test_security_arbvn(uts, "security_tpm"));
+
+ return 0;
+}
+
+DM_TEST(dm_test_security_tpm, UT_TESTF_SCAN_FDT);
--
2.40.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
2023-08-12 0:28 ` [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices seanedmond
@ 2023-08-14 8:39 ` Ilias Apalodimas
2023-08-14 21:23 ` Sean Edmond
2023-08-17 13:41 ` Simon Glass
1 sibling, 1 reply; 18+ messages in thread
From: Ilias Apalodimas @ 2023-08-14 8:39 UTC (permalink / raw)
To: seanedmond; +Cc: u-boot, sjg, stcarlso, abdellatif.elkhlifi
Hi Sean
On Sat, 12 Aug 2023 at 03:28, <seanedmond@linux.microsoft.com> wrote:
>
> From: Stephen Carlson <stcarlso@linux.microsoft.com>
>
> This implementation of the security uclass driver allows existing TPM2
> devices declared in the device tree to be referenced for storing the OS
> anti-rollback counter, using the TPM2 non-volatile storage API.
>
> Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
> ---
> MAINTAINERS | 1 +
> drivers/security/Makefile | 1 +
> drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++++++
> include/tpm-v2.h | 1 +
> 4 files changed, 176 insertions(+)
> create mode 100644 drivers/security/security-tpm.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
[...]
> +
> +struct security_state {
> + u32 index_arbvn;
> + struct udevice *tpm_dev;
> +};
> +
> +static int tpm_security_init(struct udevice *tpm_dev)
> +{
> + int res;
> +
> + /* Initialize TPM but allow reuse of existing session */
> + res = tpm_open(tpm_dev);
> + if (res == -EBUSY) {
> + log(UCLASS_SECURITY, LOGL_DEBUG,
> + "Existing TPM session found, reusing\n");
> + } else {
> + if (res) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "TPM initialization failed (ret=%d)\n", res);
> + return res;
> + }
> +
> + res = tpm2_startup(tpm_dev, TPM2_SU_CLEAR);
> + if (res) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "TPM startup failed (ret=%d)\n", res);
> + return res;
> + }
> + }
> +
> + return 0;
> +}
There's nothing security related in that wrapper. It looks like a
typical tpm startup sequence. Any reason you can't use
tpm_auto_start()?
> +
> +static int tpm_security_arbvn_get(struct udevice *dev, u64 *arbvn)
> +{
> + struct security_state *priv = dev_get_priv(dev);
> + int ret;
> +
> + if (!arbvn)
> + return -EINVAL;
> +
> + ret = tpm2_nv_read_value(priv->tpm_dev, priv->index_arbvn, arbvn,
> + sizeof(u64));
> + if (ret == TPM2_RC_NV_UNINITIALIZED) {
> + /* Expected if no OS image has been loaded before */
> + log(UCLASS_SECURITY, LOGL_INFO,
> + "No previous OS image, defaulting ARBVN to 0\n");
> + *arbvn = 0ULL;
Why aren't we returning an error here? Looks like the code following
this is trying to reason with the validity of arbnv
> + } else if (ret) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "Unable to read ARBVN from TPM (ret=%d)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tpm_security_arbvn_set(struct udevice *dev, u64 arbvn)
> +{
> + struct security_state *priv = dev_get_priv(dev);
> + struct udevice *tpm_dev = priv->tpm_dev;
> + u64 old_arbvn;
> + int ret;
> +
> + ret = tpm_security_arbvn_get(dev, &old_arbvn);
> + if (ret)
> + return ret;
> +
> + if (arbvn < old_arbvn)
> + return -EPERM;
> +
What happens if they are equal ?
> + if (arbvn > old_arbvn) {
You just check for this and exited
> + ret = tpm2_nv_write_value(tpm_dev, priv->index_arbvn, &arbvn,
> + sizeof(u64));
> + if (ret) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "Unable to write ARBVN to TPM (ret=%d)\n", ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct dm_security_ops tpm_security_ops = {
> + .arbvn_get = tpm_security_arbvn_get,
> + .arbvn_set = tpm_security_arbvn_set,
> +};
> +
> +static int tpm_security_probe(struct udevice *dev)
> +{
> + struct security_state *priv = dev_get_priv(dev);
> + struct udevice *tpm_dev = priv->tpm_dev;
> + u32 index = priv->index_arbvn;
> + int ret;
> +
> + if (!tpm_dev) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "TPM device not defined in DTS\n");
> + return -EINVAL;
> + }
> +
> + ret = tpm_security_init(tpm_dev);
> + if (ret)
> + return ret;
> +
> + ret = tpm2_nv_define_space(tpm_dev, index, sizeof(u64), TPMA_NV_PPREAD |
> + TPMA_NV_PPWRITE | TPMA_NV_PLATFORMCREATE,
> + NULL, 0);
How secure is that ? Is it protected by a locality? We dont seem to be
using an auth value when creating the index
> + /* NV_DEFINED is an expected error if ARBVN already initialized */
> + if (ret == TPM2_RC_NV_DEFINED)
> + log(UCLASS_SECURITY, LOGL_DEBUG,
> + "ARBVN index %u already defined\n", index);
I'd prefer returning 0 here. The rewrite the code below as
if (ret)
log().....
return ret;
> + else if (ret) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "Unable to create ARBVN NV index (ret=%d)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tpm_security_remove(struct udevice *dev)
> +{
> + struct security_state *priv = dev_get_priv(dev);
> +
> + return tpm_close(priv->tpm_dev);
> +}
> +
> +static int tpm_security_ofdata_to_platdata(struct udevice *dev)
> +{
> + const u32 phandle = (u32)dev_read_u32_default(dev, "tpm", 0);
> + struct security_state *priv = dev_get_priv(dev);
> + struct udevice *tpm_dev;
> + int ret;
> +
> + ret = uclass_get_device_by_phandle_id(UCLASS_TPM, phandle, &tpm_dev);
> + if (ret) {
> + log(UCLASS_SECURITY, LOGL_ERR, "TPM node in DTS is invalid\n");
> + return ret;
> + }
> +
> + priv->index_arbvn = (u32)dev_read_u32_default(dev, "arbvn-nv-index", 0);
> + priv->tpm_dev = tpm_dev;
> + return 0;
> +}
> +
> +static const struct udevice_id tpm_security_ids[] = {
> + { .compatible = "tpm,security" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(security_tpm) = {
> + .name = "security_tpm",
> + .id = UCLASS_SECURITY,
> + .priv_auto = sizeof(struct security_state),
> + .of_match = tpm_security_ids,
> + .of_to_plat = tpm_security_ofdata_to_platdata,
> + .probe = tpm_security_probe,
> + .remove = tpm_security_remove,
> + .ops = &tpm_security_ops,
> +};
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 2b6980e441..49bf0f0ba4 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -321,6 +321,7 @@ enum tpm2_return_codes {
> TPM2_RC_COMMAND_CODE = TPM2_RC_VER1 + 0x0043,
> TPM2_RC_AUTHSIZE = TPM2_RC_VER1 + 0x0044,
> TPM2_RC_AUTH_CONTEXT = TPM2_RC_VER1 + 0x0045,
> + TPM2_RC_NV_UNINITIALIZED = TPM2_RC_VER1 + 0x04a,
> TPM2_RC_NV_DEFINED = TPM2_RC_VER1 + 0x004c,
> TPM2_RC_NEEDS_TEST = TPM2_RC_VER1 + 0x0053,
> TPM2_RC_WARN = 0x0900,
> --
> 2.40.0
>
Thanks
/Ilias
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
2023-08-14 8:39 ` Ilias Apalodimas
@ 2023-08-14 21:23 ` Sean Edmond
2023-08-16 13:55 ` Ilias Apalodimas
0 siblings, 1 reply; 18+ messages in thread
From: Sean Edmond @ 2023-08-14 21:23 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, sjg, stcarlso, abdellatif.elkhlifi
On 2023-08-14 1:39 a.m., Ilias Apalodimas wrote:
> Hi Sean
>
> On Sat, 12 Aug 2023 at 03:28, <seanedmond@linux.microsoft.com> wrote:
>> From: Stephen Carlson <stcarlso@linux.microsoft.com>
>>
>> This implementation of the security uclass driver allows existing TPM2
>> devices declared in the device tree to be referenced for storing the OS
>> anti-rollback counter, using the TPM2 non-volatile storage API.
>>
>> Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/security/Makefile | 1 +
>> drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++++++
>> include/tpm-v2.h | 1 +
>> 4 files changed, 176 insertions(+)
>> create mode 100644 drivers/security/security-tpm.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
> [...]
>
>> +
>> +struct security_state {
>> + u32 index_arbvn;
>> + struct udevice *tpm_dev;
>> +};
>> +
>> +static int tpm_security_init(struct udevice *tpm_dev)
>> +{
>> + int res;
>> +
>> + /* Initialize TPM but allow reuse of existing session */
>> + res = tpm_open(tpm_dev);
>> + if (res == -EBUSY) {
>> + log(UCLASS_SECURITY, LOGL_DEBUG,
>> + "Existing TPM session found, reusing\n");
>> + } else {
>> + if (res) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "TPM initialization failed (ret=%d)\n", res);
>> + return res;
>> + }
>> +
>> + res = tpm2_startup(tpm_dev, TPM2_SU_CLEAR);
>> + if (res) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "TPM startup failed (ret=%d)\n", res);
>> + return res;
>> + }
>> + }
>> +
>> + return 0;
>> +}
> There's nothing security related in that wrapper. It looks like a
> typical tpm startup sequence. Any reason you can't use
> tpm_auto_start()?
Good suggestion, I'll make this change.
>
>> +
>> +static int tpm_security_arbvn_get(struct udevice *dev, u64 *arbvn)
>> +{
>> + struct security_state *priv = dev_get_priv(dev);
>> + int ret;
>> +
>> + if (!arbvn)
>> + return -EINVAL;
>> +
>> + ret = tpm2_nv_read_value(priv->tpm_dev, priv->index_arbvn, arbvn,
>> + sizeof(u64));
>> + if (ret == TPM2_RC_NV_UNINITIALIZED) {
>> + /* Expected if no OS image has been loaded before */
>> + log(UCLASS_SECURITY, LOGL_INFO,
>> + "No previous OS image, defaulting ARBVN to 0\n");
>> + *arbvn = 0ULL;
> Why aren't we returning an error here? Looks like the code following
> this is trying to reason with the validity of arbnv
On the first boot (before ARBVN has been set in NV memory), it's
expected that the NV index hasn't been initialized/written yet. In this
case, TPM2_RC_NV_UNINITIALIZED is expected. A value of 0 is returned to
ensure that the anti-rollback check always passes (which it should since
there's nothing to check on the first boot).
>
>> + } else if (ret) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "Unable to read ARBVN from TPM (ret=%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tpm_security_arbvn_set(struct udevice *dev, u64 arbvn)
>> +{
>> + struct security_state *priv = dev_get_priv(dev);
>> + struct udevice *tpm_dev = priv->tpm_dev;
>> + u64 old_arbvn;
>> + int ret;
>> +
>> + ret = tpm_security_arbvn_get(dev, &old_arbvn);
>> + if (ret)
>> + return ret;
>> +
>> + if (arbvn < old_arbvn)
>> + return -EPERM;
>> +
> What happens if they are equal ?
>
If they are equal, then we are booting the same OS that was previously
booted (we are not moving the OS version forward or back).
Note the actual "anti-rollback" check is in fit_image_verify_arbvn().
If it make things more clear, we could remove the value checks here
completely and just write the value.
>> + if (arbvn > old_arbvn) {
> You just check for this and exited
>
>> + ret = tpm2_nv_write_value(tpm_dev, priv->index_arbvn, &arbvn,
>> + sizeof(u64));
>> + if (ret) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "Unable to write ARBVN to TPM (ret=%d)\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dm_security_ops tpm_security_ops = {
>> + .arbvn_get = tpm_security_arbvn_get,
>> + .arbvn_set = tpm_security_arbvn_set,
>> +};
>> +
>> +static int tpm_security_probe(struct udevice *dev)
>> +{
>> + struct security_state *priv = dev_get_priv(dev);
>> + struct udevice *tpm_dev = priv->tpm_dev;
>> + u32 index = priv->index_arbvn;
>> + int ret;
>> +
>> + if (!tpm_dev) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "TPM device not defined in DTS\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = tpm_security_init(tpm_dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = tpm2_nv_define_space(tpm_dev, index, sizeof(u64), TPMA_NV_PPREAD |
>> + TPMA_NV_PPWRITE | TPMA_NV_PLATFORMCREATE,
>> + NULL, 0);
> How secure is that ? Is it protected by a locality? We dont seem to be
> using an auth value when creating the index
On our platform, we're using a different security device driver to
provide our secure storage (we aren't using this TPM backed driver).
I'm not an expert on authorization for NV indexes, but I'd welcome
feedback on how we could make this driver more secure (and publicly
available) for others.
>
>> + /* NV_DEFINED is an expected error if ARBVN already initialized */
>> + if (ret == TPM2_RC_NV_DEFINED)
>> + log(UCLASS_SECURITY, LOGL_DEBUG,
>> + "ARBVN index %u already defined\n", index);
> I'd prefer returning 0 here. The rewrite the code below as
> if (ret)
> log().....
>
> return ret;
>
>> + else if (ret) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "Unable to create ARBVN NV index (ret=%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tpm_security_remove(struct udevice *dev)
>> +{
>> + struct security_state *priv = dev_get_priv(dev);
>> +
>> + return tpm_close(priv->tpm_dev);
>> +}
>> +
>> +static int tpm_security_ofdata_to_platdata(struct udevice *dev)
>> +{
>> + const u32 phandle = (u32)dev_read_u32_default(dev, "tpm", 0);
>> + struct security_state *priv = dev_get_priv(dev);
>> + struct udevice *tpm_dev;
>> + int ret;
>> +
>> + ret = uclass_get_device_by_phandle_id(UCLASS_TPM, phandle, &tpm_dev);
>> + if (ret) {
>> + log(UCLASS_SECURITY, LOGL_ERR, "TPM node in DTS is invalid\n");
>> + return ret;
>> + }
>> +
>> + priv->index_arbvn = (u32)dev_read_u32_default(dev, "arbvn-nv-index", 0);
>> + priv->tpm_dev = tpm_dev;
>> + return 0;
>> +}
>> +
>> +static const struct udevice_id tpm_security_ids[] = {
>> + { .compatible = "tpm,security" },
>> + { }
>> +};
>> +
>> +U_BOOT_DRIVER(security_tpm) = {
>> + .name = "security_tpm",
>> + .id = UCLASS_SECURITY,
>> + .priv_auto = sizeof(struct security_state),
>> + .of_match = tpm_security_ids,
>> + .of_to_plat = tpm_security_ofdata_to_platdata,
>> + .probe = tpm_security_probe,
>> + .remove = tpm_security_remove,
>> + .ops = &tpm_security_ops,
>> +};
>> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
>> index 2b6980e441..49bf0f0ba4 100644
>> --- a/include/tpm-v2.h
>> +++ b/include/tpm-v2.h
>> @@ -321,6 +321,7 @@ enum tpm2_return_codes {
>> TPM2_RC_COMMAND_CODE = TPM2_RC_VER1 + 0x0043,
>> TPM2_RC_AUTHSIZE = TPM2_RC_VER1 + 0x0044,
>> TPM2_RC_AUTH_CONTEXT = TPM2_RC_VER1 + 0x0045,
>> + TPM2_RC_NV_UNINITIALIZED = TPM2_RC_VER1 + 0x04a,
>> TPM2_RC_NV_DEFINED = TPM2_RC_VER1 + 0x004c,
>> TPM2_RC_NEEDS_TEST = TPM2_RC_VER1 + 0x0053,
>> TPM2_RC_WARN = 0x0900,
>> --
>> 2.40.0
>>
> Thanks
> /Ilias
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] drivers: security: Add security devices to driver model
2023-08-12 0:28 ` [PATCH 1/5] drivers: security: Add security devices to driver model seanedmond
@ 2023-08-16 13:14 ` Ilias Apalodimas
2023-08-17 13:41 ` Simon Glass
1 sibling, 0 replies; 18+ messages in thread
From: Ilias Apalodimas @ 2023-08-16 13:14 UTC (permalink / raw)
To: seanedmond; +Cc: u-boot, sjg, stcarlso, abdellatif.elkhlifi
Hi Sean
On Sat, 12 Aug 2023 at 03:28, <seanedmond@linux.microsoft.com> wrote:
>
> From: Stephen Carlson <stcarlso@linux.microsoft.com>
>
> Security 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_SECURITY.
> - New config CONFIG_DM_SECURITY to enable security device support.
> - New driver sandbox_security matching "security,sandbox", enabled with
> new config CONFIG_SECURITY_SANDBOX.
>
[...]
>
> source "drivers/scsi/Kconfig"
>
> +source "drivers/security/Kconfig"
> +
> source "drivers/serial/Kconfig"
>
> source "drivers/smem/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index efc2a4afb2..b670aae5fd 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 += security/
> obj-y += sound/
> obj-y += spmi/
> obj-y += watchdog/
> diff --git a/drivers/security/Kconfig b/drivers/security/Kconfig
> new file mode 100644
> index 0000000000..f7af5c4e78
> --- /dev/null
> +++ b/drivers/security/Kconfig
> @@ -0,0 +1,25 @@
> +config DM_SECURITY
> + bool "Support security devices with driver model"
> + depends on DM
> + help
> + This option enables support for the security uclass which supports
> + devices intended to provide additional security features 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 SECURITY_SANDBOX
> + bool "Enable sandbox security driver"
> + depends on DM_SECURITY
> + help
> + This driver supports a simulated security 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 SECURITY_TPM
> + bool "Enable TPM security driver"
> + depends on TPM && TPM_V2 && DM_SECURITY
> + help
> + This driver supports a security device based on existing TPM
> + functionality.
I think this is generally a good idea. But we need to define a bit
better what we consider 'security' and what is supported by this
uclass. One example would be a TPM RNG device. We already support
that and we even use it as an RNG in certain cases. Is this something
that we should move here? Because atm the new class seems to only
support a rollback counter (which is fine, we might just have to pick
a different name)
[...]
Thanks
/Ilias
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
2023-08-14 21:23 ` Sean Edmond
@ 2023-08-16 13:55 ` Ilias Apalodimas
0 siblings, 0 replies; 18+ messages in thread
From: Ilias Apalodimas @ 2023-08-16 13:55 UTC (permalink / raw)
To: Sean Edmond; +Cc: u-boot, sjg, stcarlso, abdellatif.elkhlifi
On Mon, Aug 14, 2023 at 02:23:22PM -0700, Sean Edmond wrote:
>
> On 2023-08-14 1:39 a.m., Ilias Apalodimas wrote:
> > Hi Sean
> >
> > On Sat, 12 Aug 2023 at 03:28, <seanedmond@linux.microsoft.com> wrote:
> > > From: Stephen Carlson <stcarlso@linux.microsoft.com>
> > >
> > > This implementation of the security uclass driver allows existing TPM2
> > > devices declared in the device tree to be referenced for storing the OS
> > > anti-rollback counter, using the TPM2 non-volatile storage API.
> > >
> > > Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
> > > ---
> > > MAINTAINERS | 1 +
> > > drivers/security/Makefile | 1 +
> > > drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++++++
> > > include/tpm-v2.h | 1 +
> > > 4 files changed, 176 insertions(+)
> > > create mode 100644 drivers/security/security-tpm.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > [...]
> >
> > > +
> > > +struct security_state {
> > > + u32 index_arbvn;
> > > + struct udevice *tpm_dev;
> > > +};
> > > +
> > > +static int tpm_security_init(struct udevice *tpm_dev)
> > > +{
> > > + int res;
> > > +
> > > + /* Initialize TPM but allow reuse of existing session */
> > > + res = tpm_open(tpm_dev);
> > > + if (res == -EBUSY) {
> > > + log(UCLASS_SECURITY, LOGL_DEBUG,
> > > + "Existing TPM session found, reusing\n");
> > > + } else {
> > > + if (res) {
> > > + log(UCLASS_SECURITY, LOGL_ERR,
> > > + "TPM initialization failed (ret=%d)\n", res);
> > > + return res;
> > > + }
> > > +
> > > + res = tpm2_startup(tpm_dev, TPM2_SU_CLEAR);
> > > + if (res) {
> > > + log(UCLASS_SECURITY, LOGL_ERR,
> > > + "TPM startup failed (ret=%d)\n", res);
> > > + return res;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > There's nothing security related in that wrapper. It looks like a
> > typical tpm startup sequence. Any reason you can't use
> > tpm_auto_start()?
>
> Good suggestion, I'll make this change.
>
> >
> > > +
> > > +static int tpm_security_arbvn_get(struct udevice *dev, u64 *arbvn)
> > > +{
> > > + struct security_state *priv = dev_get_priv(dev);
> > > + int ret;
> > > +
> > > + if (!arbvn)
> > > + return -EINVAL;
> > > +
> > > + ret = tpm2_nv_read_value(priv->tpm_dev, priv->index_arbvn, arbvn,
> > > + sizeof(u64));
> > > + if (ret == TPM2_RC_NV_UNINITIALIZED) {
> > > + /* Expected if no OS image has been loaded before */
> > > + log(UCLASS_SECURITY, LOGL_INFO,
> > > + "No previous OS image, defaulting ARBVN to 0\n");
> > > + *arbvn = 0ULL;
> > Why aren't we returning an error here? Looks like the code following
> > this is trying to reason with the validity of arbnv
> On the first boot (before ARBVN has been set in NV memory), it's expected
> that the NV index hasn't been initialized/written yet. In this case,
> TPM2_RC_NV_UNINITIALIZED is expected. A value of 0 is returned to ensure
> that the anti-rollback check always passes (which it should since there's
> nothing to check on the first boot).
Ok then I think it's better to add an 'init' function which will talk to
the TPM and try to read the value. If you get an error or
TPM2_RC_NV_UNINITIALIZED(), we can then create the NV storage and
initialize it. Note here that blindly returning 0 isn't correct either.
When you define a TPM NV counter index it will hold any stored value (and
reuse it) even if you delete it and re-add it.
I think doing it like this will make _get() a bit simpler, since you just
have to talk to the TPM and return whatever you read.
> >
> > > + } else if (ret) {
> > > + log(UCLASS_SECURITY, LOGL_ERR,
> > > + "Unable to read ARBVN from TPM (ret=%d)\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int tpm_security_arbvn_set(struct udevice *dev, u64 arbvn)
> > > +{
> > > + struct security_state *priv = dev_get_priv(dev);
> > > + struct udevice *tpm_dev = priv->tpm_dev;
> > > + u64 old_arbvn;
> > > + int ret;
> > > +
> > > + ret = tpm_security_arbvn_get(dev, &old_arbvn);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (arbvn < old_arbvn)
> > > + return -EPERM;
> > > +
> > What happens if they are equal ?
> >
> If they are equal, then we are booting the same OS that was previously
> booted (we are not moving the OS version forward or back).
>
> Note the actual "anti-rollback" check is in fit_image_verify_arbvn(). If it
> make things more clear, we could remove the value checks here completely and
> just write the value.
>
Ok, I think adding another statment would be a bit easier to read then.
Right after reading the stored TPM value, just return 0 if arbvn == old_arbvn
> > > + if (arbvn > old_arbvn) {
> > You just check for this and exited
> >
> > > + ret = tpm2_nv_write_value(tpm_dev, priv->index_arbvn, &arbvn,
> > > + sizeof(u64));
> > > + if (ret) {
> > > + log(UCLASS_SECURITY, LOGL_ERR,
> > > + "Unable to write ARBVN to TPM (ret=%d)\n", ret);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct dm_security_ops tpm_security_ops = {
> > > + .arbvn_get = tpm_security_arbvn_get,
> > > + .arbvn_set = tpm_security_arbvn_set,
> > > +};
> > > +
> > > +static int tpm_security_probe(struct udevice *dev)
> > > +{
> > > + struct security_state *priv = dev_get_priv(dev);
> > > + struct udevice *tpm_dev = priv->tpm_dev;
> > > + u32 index = priv->index_arbvn;
> > > + int ret;
> > > +
> > > + if (!tpm_dev) {
> > > + log(UCLASS_SECURITY, LOGL_ERR,
> > > + "TPM device not defined in DTS\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = tpm_security_init(tpm_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = tpm2_nv_define_space(tpm_dev, index, sizeof(u64), TPMA_NV_PPREAD |
> > > + TPMA_NV_PPWRITE | TPMA_NV_PLATFORMCREATE,
> > > + NULL, 0);
> > How secure is that ? Is it protected by a locality? We dont seem to be
> > using an auth value when creating the index
> On our platform, we're using a different security device driver to provide
> our secure storage (we aren't using this TPM backed driver). I'm not an
> expert on authorization for NV indexes, but I'd welcome feedback on how we
> could make this driver more secure (and publicly available) for others.
IIRC we can use a 'NV Counter Index' for the counter. That's only allowed
to increment and even if you delete it and reinitialize it, it will retain
it's (lost) value)
> >
> > > + /* NV_DEFINED is an expected error if ARBVN already initialized */
> > > + if (ret == TPM2_RC_NV_DEFINED)
> > > + log(UCLASS_SECURITY, LOGL_DEBUG,
> > > + "ARBVN index %u already defined\n", index);
> > I'd prefer returning 0 here. The rewrite the code below as
> > if (ret)
> > log().....
> >
> > return ret;
> >
> > > + else if (ret) {
> > > + log(UCLASS_SECURITY, LOGL_ERR,
> > > + "Unable to create ARBVN NV index (ret=%d)\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int tpm_security_remove(struct udevice *dev)
> > > +{
> > > + struct security_state *priv = dev_get_priv(dev);
> > > +
> > > + return tpm_close(priv->tpm_dev);
> > > +}
> > > +
> > > +static int tpm_security_ofdata_to_platdata(struct udevice *dev)
> > > +{
> > > + const u32 phandle = (u32)dev_read_u32_default(dev, "tpm", 0);
> > > + struct security_state *priv = dev_get_priv(dev);
> > > + struct udevice *tpm_dev;
> > > + int ret;
> > > +
> > > + ret = uclass_get_device_by_phandle_id(UCLASS_TPM, phandle, &tpm_dev);
> > > + if (ret) {
> > > + log(UCLASS_SECURITY, LOGL_ERR, "TPM node in DTS is invalid\n");
> > > + return ret;
> > > + }
> > > +
> > > + priv->index_arbvn = (u32)dev_read_u32_default(dev, "arbvn-nv-index", 0);
> > > + priv->tpm_dev = tpm_dev;
> > > + return 0;
> > > +}
> > > +
> > > +static const struct udevice_id tpm_security_ids[] = {
> > > + { .compatible = "tpm,security" },
> > > + { }
> > > +};
> > > +
> > > +U_BOOT_DRIVER(security_tpm) = {
> > > + .name = "security_tpm",
> > > + .id = UCLASS_SECURITY,
> > > + .priv_auto = sizeof(struct security_state),
> > > + .of_match = tpm_security_ids,
> > > + .of_to_plat = tpm_security_ofdata_to_platdata,
> > > + .probe = tpm_security_probe,
> > > + .remove = tpm_security_remove,
> > > + .ops = &tpm_security_ops,
> > > +};
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index 2b6980e441..49bf0f0ba4 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -321,6 +321,7 @@ enum tpm2_return_codes {
> > > TPM2_RC_COMMAND_CODE = TPM2_RC_VER1 + 0x0043,
> > > TPM2_RC_AUTHSIZE = TPM2_RC_VER1 + 0x0044,
> > > TPM2_RC_AUTH_CONTEXT = TPM2_RC_VER1 + 0x0045,
> > > + TPM2_RC_NV_UNINITIALIZED = TPM2_RC_VER1 + 0x04a,
> > > TPM2_RC_NV_DEFINED = TPM2_RC_VER1 + 0x004c,
> > > TPM2_RC_NEEDS_TEST = TPM2_RC_VER1 + 0x0053,
> > > TPM2_RC_WARN = 0x0900,
> > > --
> > > 2.40.0
> > >
> > Thanks
> > /Ilias
> >
> >
Thanks
/Ilias
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/5] Add anti-rollback validation feature
2023-08-12 0:28 [PATCH 0/5] Add anti-rollback validation feature seanedmond
` (4 preceding siblings ...)
2023-08-12 0:28 ` [PATCH 5/5] dm: test: Add a test for security driver seanedmond
@ 2023-08-17 13:41 ` Simon Glass
5 siblings, 0 replies; 18+ messages in thread
From: Simon Glass @ 2023-08-17 13:41 UTC (permalink / raw)
To: seanedmond; +Cc: u-boot, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
Hi Sean,
On Fri, 11 Aug 2023 at 18:28, <seanedmond@linux.microsoft.com> wrote:
>
> From: Sean Edmond <seanedmond@microsoft.com>
>
> Adds Add anti-rollback version protection. Images with an anti-rollback counter
> value "arbvn" declared in the FDT will be compared against the current device
> anti-rollback counter value, and older images will not pass signature
> validation. If the image is newer, the device anti-rollback counter value will
> be updated.
>
> The "arbvn" value is stored/retrieved using the newly added security driver.
> A "TPM backed" and "sandbox backed" security driver have been provided as examples.
>
> Adds new configs:
> - CONFIG_DM_SECURITY : enable security device support
> - CONFIG_SECURITY_SANDBOX : enables "sandbox_security" driver
> - CONFIG_SECURITY_TPM : Enables "tpm_security" driver
> - CONFIG_ARBP : enable enforcement of OS anti-rollback counter during image loading
> - CONFIG_FIT_ARBVP_GRACE : adds a one unit grace period to OS anti-rollback protection
>
> Sean Edmond (1):
> dm: test: Add a test for security driver
>
> Stephen Carlson (4):
> drivers: security: Add security devices to driver model
> drivers: security: Add TPM2 implementation of security devices
> common: Add OS anti-rollback validation using security devices
> common: Add OS anti-rollback grace period
>
> MAINTAINERS | 9 ++
> arch/sandbox/dts/test.dts | 8 ++
> boot/Kconfig | 19 +++
> boot/image-fit-sig.c | 94 +++++++++++++++
> boot/image-fit.c | 23 ++++
> configs/sandbox_defconfig | 3 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/security/Kconfig | 25 ++++
> drivers/security/Makefile | 7 ++
> drivers/security/sandbox_security.c | 65 +++++++++++
> drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++
> drivers/security/security-uclass.c | 30 +++++
> include/dm-security.h | 44 +++++++
> include/dm/uclass-id.h | 1 +
> include/image.h | 4 +
> include/tpm-v2.h | 1 +
> test/dm/Makefile | 1 +
> test/dm/security.c | 78 +++++++++++++
> 19 files changed, 588 insertions(+)
> create mode 100644 drivers/security/Kconfig
> create mode 100644 drivers/security/Makefile
> create mode 100644 drivers/security/sandbox_security.c
> create mode 100644 drivers/security/security-tpm.c
> create mode 100644 drivers/security/security-uclass.c
> create mode 100644 include/dm-security.h
> create mode 100644 test/dm/security.c
Can you please add something to doc/ about this?
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] drivers: security: Add security devices to driver model
2023-08-12 0:28 ` [PATCH 1/5] drivers: security: Add security devices to driver model seanedmond
2023-08-16 13:14 ` Ilias Apalodimas
@ 2023-08-17 13:41 ` Simon Glass
1 sibling, 0 replies; 18+ messages in thread
From: Simon Glass @ 2023-08-17 13:41 UTC (permalink / raw)
To: seanedmond; +Cc: u-boot, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
Hi Sean,
On Fri, 11 Aug 2023 at 18:28, <seanedmond@linux.microsoft.com> wrote:
>
> From: Stephen Carlson <stcarlso@linux.microsoft.com>
>
> Security 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_SECURITY.
> - New config CONFIG_DM_SECURITY to enable security device support.
> - New driver sandbox_security matching "security,sandbox", enabled with
> new config CONFIG_SECURITY_SANDBOX.
How about calling this UCLASS_ROLLBACK and implementing that function?
Then you can add this device as a child of a TPM and the TPM can
implement the API.
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
2023-08-12 0:28 ` [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices seanedmond
2023-08-14 8:39 ` Ilias Apalodimas
@ 2023-08-17 13:41 ` Simon Glass
2023-08-17 23:29 ` Sean Edmond
1 sibling, 1 reply; 18+ messages in thread
From: Simon Glass @ 2023-08-17 13:41 UTC (permalink / raw)
To: seanedmond; +Cc: u-boot, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
Hi Sean,
On Fri, 11 Aug 2023 at 18:28, <seanedmond@linux.microsoft.com> wrote:
>
> From: Stephen Carlson <stcarlso@linux.microsoft.com>
>
> This implementation of the security uclass driver allows existing TPM2
> devices declared in the device tree to be referenced for storing the OS
> anti-rollback counter, using the TPM2 non-volatile storage API.
>
> Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
> ---
> MAINTAINERS | 1 +
> drivers/security/Makefile | 1 +
> drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++++++
> include/tpm-v2.h | 1 +
> 4 files changed, 176 insertions(+)
> create mode 100644 drivers/security/security-tpm.c
This is a bit wonky w.r.t driver model. The TPM itself should
implement this API, perhaps ina separate file shared with all v2 TPMs.
You should not be opening the device, etc. in here.
I hope that makes sense...you effectively need to turn the TPM into a
multi-function device within driver model. Of course TPMs are
multi-function devices anyway, but here you are making their functions
available more explicitly with a nicer API.
Then you can call the TPM API to do what you want, but at least you
know that the TPM has been probed before you start.
Regards,
Simon
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 73b6943e03..257660a847 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1444,6 +1444,7 @@ S: Maintained
> F: drivers/security/Kconfig
> F: drivers/security/Makefile
> F: drivers/security/sandbox_security.c
> +F: drivers/security/security-tpm.c
> F: drivers/security/security-uclass.c
>
> SEMIHOSTING
> diff --git a/drivers/security/Makefile b/drivers/security/Makefile
> index ed10c3f234..e81966bb4a 100644
> --- a/drivers/security/Makefile
> +++ b/drivers/security/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_DM_SECURITY) += security-uclass.o
> obj-$(CONFIG_SECURITY_SANDBOX) += sandbox_security.o
> +obj-$(CONFIG_SECURITY_TPM) += security-tpm.o
> diff --git a/drivers/security/security-tpm.c b/drivers/security/security-tpm.c
> new file mode 100644
> index 0000000000..9070dd49ac
> --- /dev/null
> +++ b/drivers/security/security-tpm.c
> @@ -0,0 +1,173 @@
> +// 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 <dm-security.h>
> +#include <tpm-v2.h>
> +
> +struct security_state {
> + u32 index_arbvn;
> + struct udevice *tpm_dev;
> +};
> +
> +static int tpm_security_init(struct udevice *tpm_dev)
> +{
> + int res;
> +
> + /* Initialize TPM but allow reuse of existing session */
> + res = tpm_open(tpm_dev);
> + if (res == -EBUSY) {
> + log(UCLASS_SECURITY, LOGL_DEBUG,
> + "Existing TPM session found, reusing\n");
> + } else {
> + if (res) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "TPM initialization failed (ret=%d)\n", res);
> + return res;
> + }
> +
> + res = tpm2_startup(tpm_dev, TPM2_SU_CLEAR);
> + if (res) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "TPM startup failed (ret=%d)\n", res);
> + return res;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int tpm_security_arbvn_get(struct udevice *dev, u64 *arbvn)
> +{
> + struct security_state *priv = dev_get_priv(dev);
> + int ret;
> +
> + if (!arbvn)
> + return -EINVAL;
> +
> + ret = tpm2_nv_read_value(priv->tpm_dev, priv->index_arbvn, arbvn,
> + sizeof(u64));
> + if (ret == TPM2_RC_NV_UNINITIALIZED) {
> + /* Expected if no OS image has been loaded before */
> + log(UCLASS_SECURITY, LOGL_INFO,
> + "No previous OS image, defaulting ARBVN to 0\n");
> + *arbvn = 0ULL;
> + } else if (ret) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "Unable to read ARBVN from TPM (ret=%d)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tpm_security_arbvn_set(struct udevice *dev, u64 arbvn)
> +{
> + struct security_state *priv = dev_get_priv(dev);
> + struct udevice *tpm_dev = priv->tpm_dev;
> + u64 old_arbvn;
> + int ret;
> +
> + ret = tpm_security_arbvn_get(dev, &old_arbvn);
> + if (ret)
> + return ret;
> +
> + if (arbvn < old_arbvn)
> + return -EPERM;
> +
> + if (arbvn > old_arbvn) {
> + ret = tpm2_nv_write_value(tpm_dev, priv->index_arbvn, &arbvn,
> + sizeof(u64));
> + if (ret) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "Unable to write ARBVN to TPM (ret=%d)\n", ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct dm_security_ops tpm_security_ops = {
> + .arbvn_get = tpm_security_arbvn_get,
> + .arbvn_set = tpm_security_arbvn_set,
> +};
> +
> +static int tpm_security_probe(struct udevice *dev)
> +{
> + struct security_state *priv = dev_get_priv(dev);
> + struct udevice *tpm_dev = priv->tpm_dev;
> + u32 index = priv->index_arbvn;
> + int ret;
> +
> + if (!tpm_dev) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "TPM device not defined in DTS\n");
> + return -EINVAL;
> + }
> +
> + ret = tpm_security_init(tpm_dev);
> + if (ret)
> + return ret;
> +
> + ret = tpm2_nv_define_space(tpm_dev, index, sizeof(u64), TPMA_NV_PPREAD |
> + TPMA_NV_PPWRITE | TPMA_NV_PLATFORMCREATE,
> + NULL, 0);
> + /* NV_DEFINED is an expected error if ARBVN already initialized */
> + if (ret == TPM2_RC_NV_DEFINED)
> + log(UCLASS_SECURITY, LOGL_DEBUG,
> + "ARBVN index %u already defined\n", index);
> + else if (ret) {
> + log(UCLASS_SECURITY, LOGL_ERR,
> + "Unable to create ARBVN NV index (ret=%d)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tpm_security_remove(struct udevice *dev)
> +{
> + struct security_state *priv = dev_get_priv(dev);
> +
> + return tpm_close(priv->tpm_dev);
> +}
> +
> +static int tpm_security_ofdata_to_platdata(struct udevice *dev)
> +{
> + const u32 phandle = (u32)dev_read_u32_default(dev, "tpm", 0);
> + struct security_state *priv = dev_get_priv(dev);
> + struct udevice *tpm_dev;
> + int ret;
> +
> + ret = uclass_get_device_by_phandle_id(UCLASS_TPM, phandle, &tpm_dev);
> + if (ret) {
> + log(UCLASS_SECURITY, LOGL_ERR, "TPM node in DTS is invalid\n");
> + return ret;
> + }
> +
> + priv->index_arbvn = (u32)dev_read_u32_default(dev, "arbvn-nv-index", 0);
> + priv->tpm_dev = tpm_dev;
> + return 0;
> +}
> +
> +static const struct udevice_id tpm_security_ids[] = {
> + { .compatible = "tpm,security" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(security_tpm) = {
> + .name = "security_tpm",
> + .id = UCLASS_SECURITY,
> + .priv_auto = sizeof(struct security_state),
> + .of_match = tpm_security_ids,
> + .of_to_plat = tpm_security_ofdata_to_platdata,
> + .probe = tpm_security_probe,
> + .remove = tpm_security_remove,
> + .ops = &tpm_security_ops,
> +};
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 2b6980e441..49bf0f0ba4 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -321,6 +321,7 @@ enum tpm2_return_codes {
> TPM2_RC_COMMAND_CODE = TPM2_RC_VER1 + 0x0043,
> TPM2_RC_AUTHSIZE = TPM2_RC_VER1 + 0x0044,
> TPM2_RC_AUTH_CONTEXT = TPM2_RC_VER1 + 0x0045,
> + TPM2_RC_NV_UNINITIALIZED = TPM2_RC_VER1 + 0x04a,
> TPM2_RC_NV_DEFINED = TPM2_RC_VER1 + 0x004c,
> TPM2_RC_NEEDS_TEST = TPM2_RC_VER1 + 0x0053,
> TPM2_RC_WARN = 0x0900,
> --
> 2.40.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] common: Add OS anti-rollback validation using security devices
2023-08-12 0:28 ` [PATCH 3/5] common: Add OS anti-rollback validation using " seanedmond
@ 2023-08-17 13:41 ` Simon Glass
0 siblings, 0 replies; 18+ messages in thread
From: Simon Glass @ 2023-08-17 13:41 UTC (permalink / raw)
To: seanedmond; +Cc: u-boot, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
Hi Sean,
On Fri, 11 Aug 2023 at 18:28, <seanedmond@linux.microsoft.com> wrote:
>
> From: Stephen Carlson <stcarlso@linux.microsoft.com>
>
> New config CONFIG_ARBP to enable enforcement of OS anti-rollback counter
> during image loading.
>
> Images with an anti-rollback counter value "arbvn" declared in the FDT will
> be compared against the current device anti-rollback counter value, and
> older images will not pass signature validation. If the image is newer, the
> device anti-rollback counter value will be updated.
>
> Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
> ---
> boot/Kconfig | 9 +++++
> boot/image-fit-sig.c | 89 ++++++++++++++++++++++++++++++++++++++++++++
> boot/image-fit.c | 23 ++++++++++++
> include/image.h | 4 ++
> 4 files changed, 125 insertions(+)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index e8fb03b801..e08c274b7c 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -103,6 +103,15 @@ config FIT_CIPHER
> Enable the feature of data ciphering/unciphering in the tool mkimage
> and in the u-boot support of the FIT image.
>
> +config FIT_ARBP
FIT_ROLLBACK would be better
arbp is really horrible :-)
> + bool "Enable Anti rollback version check for FIT images"
> + depends on FIT_SIGNATURE
> + default n
> + help
> + Enables FIT image anti-rollback protection. This feature is required
> + when a platform needs to retire previous versions of FIT images due to
> + security flaws and prevent devices from being reverted to them.
> +
> config FIT_VERBOSE
> bool "Show verbose messages when FIT images fail"
> depends on FIT
> diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c
> index 12369896fe..bf3b81a3a3 100644
> --- a/boot/image-fit-sig.c
> +++ b/boot/image-fit-sig.c
> @@ -11,6 +11,8 @@
> #include <log.h>
> #include <malloc.h>
> #include <asm/global_data.h>
> +#include <dm.h>
> +#include <dm-security.h>
You don't need dm- in your headerfiles. I think this should be
rolllback.h and that should be the name of your uclass.
> DECLARE_GLOBAL_DATA_PTR;
> #endif /* !USE_HOSTCC*/
> #include <fdt_region.h>
> @@ -63,6 +65,39 @@ struct image_region *fit_region_make_list(const void *fit,
> return region;
> }
>
> +#if !defined(USE_HOSTCC)
Can we drop that?
> +static int fit_image_verify_arbvn(const void *fit, int image_noffset)
> +{
> + u64 image_arbvn;
> + u64 plat_arbvn = 0ULL;
> + struct udevice *dev;
> + int ret;
> +
> + ret = fit_image_get_arbvn(fit, image_noffset, &image_arbvn);
> + if (ret)
> + return 0;
?? Isn't this an error?
> +
> + ret = uclass_first_device_err(UCLASS_SECURITY, &dev);
> + if (ret)
> + return -ENODEV;
return ret
> +
> + ret = dm_security_arbvn_get(dev, &plat_arbvn);
> + if (ret)
> + return -EIO;
> +
> + if (image_arbvn < plat_arbvn) {
> + return -EPERM;
> + } else if (image_arbvn > plat_arbvn) {
> + ret = dm_security_arbvn_set(dev, image_arbvn);
> + printf(" Updating OS anti-rollback to %llu from %llu\n",
> + image_arbvn, plat_arbvn);
So the update happens in U-Boot? Don't we want to update it when we
know it boots?
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> static int fit_image_setup_verify(struct image_sign_info *info,
> const void *fit, int noffset,
> const void *key_blob, int required_keynode,
> @@ -175,6 +210,16 @@ static int fit_image_verify_sig(const void *fit, int image_noffset,
> goto error;
> }
>
> +#if !defined(USE_HOSTCC)
Can you use
if (!tools_build())
?
This seems to be adding to FIT so the FIT docs should be updated.
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] common: Add OS anti-rollback grace period
2023-08-12 0:28 ` [PATCH 4/5] common: Add OS anti-rollback grace period seanedmond
@ 2023-08-17 13:41 ` Simon Glass
0 siblings, 0 replies; 18+ messages in thread
From: Simon Glass @ 2023-08-17 13:41 UTC (permalink / raw)
To: seanedmond; +Cc: u-boot, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
Hi Sean,
On Fri, 11 Aug 2023 at 18:28, <seanedmond@linux.microsoft.com> wrote:
>
> From: Stephen Carlson <stcarlso@microsoft.com>
>
> New config CONFIG_FIT_ARBVP_GRACE to add a one unit grace period to OS
> anti-rollback protection, allowing images with anti-rollback counters
> exactly one less than the platform value to still be loaded. No update to
> the platform anti-rollback counter will be performed in this case.
This seems like a grace version rather than a grace period? I'm not
sure if that is a better name, but I might imagine a grace period of
one month, for example.
>
> Signed-off-by: Stephen Carlson <stcarlso@microsoft.com>
> ---
> boot/Kconfig | 10 ++++++++++
> boot/image-fit-sig.c | 7 ++++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index e08c274b7c..cd16bb8e53 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -112,6 +112,16 @@ config FIT_ARBP
> when a platform needs to retire previous versions of FIT images due to
> security flaws and prevent devices from being reverted to them.
>
> +config FIT_ARBP_GRACE
> + bool "Enable FIT Anti rollback grace period"
> + depends on FIT_ARBP
> + default n
> + help
> + Enables a one unit grace period for FIT image anti-rollback protection,
> + where anti-rollback protection will still accept a FIT image with an
> + anti-rollback version one less than the current number, but will not
> + update the platform anti-rollback counter in that case.
> +
> config FIT_VERBOSE
> bool "Show verbose messages when FIT images fail"
> depends on FIT
> diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c
> index bf3b81a3a3..dc88a4b2cb 100644
> --- a/boot/image-fit-sig.c
> +++ b/boot/image-fit-sig.c
> @@ -70,6 +70,7 @@ static int fit_image_verify_arbvn(const void *fit, int image_noffset)
> {
> u64 image_arbvn;
> u64 plat_arbvn = 0ULL;
> + u64 target_arbvn;
> struct udevice *dev;
> int ret;
>
> @@ -85,7 +86,11 @@ static int fit_image_verify_arbvn(const void *fit, int image_noffset)
> if (ret)
> return -EIO;
>
> - if (image_arbvn < plat_arbvn) {
> + target_arbvn = plat_arbvn;
> + /* Calculate target ARBVN, including grace period if enabled */
> + if (CONFIG_IS_ENABLED(FIT_ARBP_GRACE) && plat_arbvn > 0ULL)
> 0
> + target_arbvn = plat_arbvn - 1ULL;
> + if (image_arbvn < target_arbvn) {
> return -EPERM;
> } else if (image_arbvn > plat_arbvn) {
> ret = dm_security_arbvn_set(dev, image_arbvn);
> --
> 2.40.0
>
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] dm: test: Add a test for security driver
2023-08-12 0:28 ` [PATCH 5/5] dm: test: Add a test for security driver seanedmond
@ 2023-08-17 13:41 ` Simon Glass
0 siblings, 0 replies; 18+ messages in thread
From: Simon Glass @ 2023-08-17 13:41 UTC (permalink / raw)
To: seanedmond; +Cc: u-boot, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
Hi Sean,
On Fri, 11 Aug 2023 at 18:28, <seanedmond@linux.microsoft.com> wrote:
>
> From: Sean Edmond <seanedmond@microsoft.com>
>
> Adds a test for a sandbox and TPM backed security driver.
>
> Allows for testing of anti-rollback version number get/set API using the
> security driver.
>
> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> ---
> arch/sandbox/dts/test.dts | 8 ++++
> configs/sandbox_defconfig | 3 ++
> test/dm/Makefile | 1 +
> test/dm/security.c | 78 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 90 insertions(+)
> create mode 100644 test/dm/security.c
Again please drop the dm_ prefixes.
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
2023-08-17 13:41 ` Simon Glass
@ 2023-08-17 23:29 ` Sean Edmond
2023-08-18 3:10 ` Simon Glass
0 siblings, 1 reply; 18+ messages in thread
From: Sean Edmond @ 2023-08-17 23:29 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, stcarlso, ilias.apalodimas, abdellatif.elkhlifi
Hi Simon,
On 2023-08-17 6:41 a.m., Simon Glass wrote:
> Hi Sean,
>
> On Fri, 11 Aug 2023 at 18:28, <seanedmond@linux.microsoft.com> wrote:
>> From: Stephen Carlson <stcarlso@linux.microsoft.com>
>>
>> This implementation of the security uclass driver allows existing TPM2
>> devices declared in the device tree to be referenced for storing the OS
>> anti-rollback counter, using the TPM2 non-volatile storage API.
>>
>> Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/security/Makefile | 1 +
>> drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++++++
>> include/tpm-v2.h | 1 +
>> 4 files changed, 176 insertions(+)
>> create mode 100644 drivers/security/security-tpm.c
> This is a bit wonky w.r.t driver model. The TPM itself should
> implement this API, perhaps ina separate file shared with all v2 TPMs.
> You should not be opening the device, etc. in here.
>
> I hope that makes sense...you effectively need to turn the TPM into a
> multi-function device within driver model. Of course TPMs are
> multi-function devices anyway, but here you are making their functions
> available more explicitly with a nicer API.
>
> Then you can call the TPM API to do what you want, but at least you
> know that the TPM has been probed before you start.
>
> Regards,
> Simon
>
Let's step back for a moment to understand our intention with this feature.
We want secure storage for the anti-rollback counter. The intention is
that this storage is provided by whatever "secure driver" (let's start
calling it the "rollback driver"). On our platform, we're using a
different "rollback driver" which accesses a non-TPM based secure
storage (which we can't upstream because it's proprietary). For the
purpose of making this feature publicly available, we've added the
TPM-backed "rollback driver" (this patch). I'll make this intention
more clear in the documentation, which should lead to less confusion?
At the end of the day, all we require is dm_security_arbvn_get() and
dm_security_arbvn_set(), to get/set from the secure storage (we'll
rename these to something less ugly, because yes arbvn is gross). We
don't want to lock this feature to the TPM, because it doesn't enable
us/others to choose a different secure storage mechanism.
W.r.t opening/initializing the TPM. Someone has to open it? In our
case, it's being opened earlier with our measured boot, so "-EBUSY" in
returned on tpm_open() (we return and everyone is happy). My
understanding is that this is the currently available way for multiple
drivers to access the TPM. Ilias has recommended we use
tpm_auto_start(), which is an enhancement I'm planning to make. This
should clean thing up? If this doesn't meet your expectations, can you
describe in more detail how we should turn the TPM into a
"multi-function device"?
>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 73b6943e03..257660a847 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1444,6 +1444,7 @@ S: Maintained
>> F: drivers/security/Kconfig
>> F: drivers/security/Makefile
>> F: drivers/security/sandbox_security.c
>> +F: drivers/security/security-tpm.c
>> F: drivers/security/security-uclass.c
>>
>> SEMIHOSTING
>> diff --git a/drivers/security/Makefile b/drivers/security/Makefile
>> index ed10c3f234..e81966bb4a 100644
>> --- a/drivers/security/Makefile
>> +++ b/drivers/security/Makefile
>> @@ -4,3 +4,4 @@
>>
>> obj-$(CONFIG_DM_SECURITY) += security-uclass.o
>> obj-$(CONFIG_SECURITY_SANDBOX) += sandbox_security.o
>> +obj-$(CONFIG_SECURITY_TPM) += security-tpm.o
>> diff --git a/drivers/security/security-tpm.c b/drivers/security/security-tpm.c
>> new file mode 100644
>> index 0000000000..9070dd49ac
>> --- /dev/null
>> +++ b/drivers/security/security-tpm.c
>> @@ -0,0 +1,173 @@
>> +// 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 <dm-security.h>
>> +#include <tpm-v2.h>
>> +
>> +struct security_state {
>> + u32 index_arbvn;
>> + struct udevice *tpm_dev;
>> +};
>> +
>> +static int tpm_security_init(struct udevice *tpm_dev)
>> +{
>> + int res;
>> +
>> + /* Initialize TPM but allow reuse of existing session */
>> + res = tpm_open(tpm_dev);
>> + if (res == -EBUSY) {
>> + log(UCLASS_SECURITY, LOGL_DEBUG,
>> + "Existing TPM session found, reusing\n");
>> + } else {
>> + if (res) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "TPM initialization failed (ret=%d)\n", res);
>> + return res;
>> + }
>> +
>> + res = tpm2_startup(tpm_dev, TPM2_SU_CLEAR);
>> + if (res) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "TPM startup failed (ret=%d)\n", res);
>> + return res;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tpm_security_arbvn_get(struct udevice *dev, u64 *arbvn)
>> +{
>> + struct security_state *priv = dev_get_priv(dev);
>> + int ret;
>> +
>> + if (!arbvn)
>> + return -EINVAL;
>> +
>> + ret = tpm2_nv_read_value(priv->tpm_dev, priv->index_arbvn, arbvn,
>> + sizeof(u64));
>> + if (ret == TPM2_RC_NV_UNINITIALIZED) {
>> + /* Expected if no OS image has been loaded before */
>> + log(UCLASS_SECURITY, LOGL_INFO,
>> + "No previous OS image, defaulting ARBVN to 0\n");
>> + *arbvn = 0ULL;
>> + } else if (ret) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "Unable to read ARBVN from TPM (ret=%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tpm_security_arbvn_set(struct udevice *dev, u64 arbvn)
>> +{
>> + struct security_state *priv = dev_get_priv(dev);
>> + struct udevice *tpm_dev = priv->tpm_dev;
>> + u64 old_arbvn;
>> + int ret;
>> +
>> + ret = tpm_security_arbvn_get(dev, &old_arbvn);
>> + if (ret)
>> + return ret;
>> +
>> + if (arbvn < old_arbvn)
>> + return -EPERM;
>> +
>> + if (arbvn > old_arbvn) {
>> + ret = tpm2_nv_write_value(tpm_dev, priv->index_arbvn, &arbvn,
>> + sizeof(u64));
>> + if (ret) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "Unable to write ARBVN to TPM (ret=%d)\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dm_security_ops tpm_security_ops = {
>> + .arbvn_get = tpm_security_arbvn_get,
>> + .arbvn_set = tpm_security_arbvn_set,
>> +};
>> +
>> +static int tpm_security_probe(struct udevice *dev)
>> +{
>> + struct security_state *priv = dev_get_priv(dev);
>> + struct udevice *tpm_dev = priv->tpm_dev;
>> + u32 index = priv->index_arbvn;
>> + int ret;
>> +
>> + if (!tpm_dev) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "TPM device not defined in DTS\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = tpm_security_init(tpm_dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = tpm2_nv_define_space(tpm_dev, index, sizeof(u64), TPMA_NV_PPREAD |
>> + TPMA_NV_PPWRITE | TPMA_NV_PLATFORMCREATE,
>> + NULL, 0);
>> + /* NV_DEFINED is an expected error if ARBVN already initialized */
>> + if (ret == TPM2_RC_NV_DEFINED)
>> + log(UCLASS_SECURITY, LOGL_DEBUG,
>> + "ARBVN index %u already defined\n", index);
>> + else if (ret) {
>> + log(UCLASS_SECURITY, LOGL_ERR,
>> + "Unable to create ARBVN NV index (ret=%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tpm_security_remove(struct udevice *dev)
>> +{
>> + struct security_state *priv = dev_get_priv(dev);
>> +
>> + return tpm_close(priv->tpm_dev);
>> +}
>> +
>> +static int tpm_security_ofdata_to_platdata(struct udevice *dev)
>> +{
>> + const u32 phandle = (u32)dev_read_u32_default(dev, "tpm", 0);
>> + struct security_state *priv = dev_get_priv(dev);
>> + struct udevice *tpm_dev;
>> + int ret;
>> +
>> + ret = uclass_get_device_by_phandle_id(UCLASS_TPM, phandle, &tpm_dev);
>> + if (ret) {
>> + log(UCLASS_SECURITY, LOGL_ERR, "TPM node in DTS is invalid\n");
>> + return ret;
>> + }
>> +
>> + priv->index_arbvn = (u32)dev_read_u32_default(dev, "arbvn-nv-index", 0);
>> + priv->tpm_dev = tpm_dev;
>> + return 0;
>> +}
>> +
>> +static const struct udevice_id tpm_security_ids[] = {
>> + { .compatible = "tpm,security" },
>> + { }
>> +};
>> +
>> +U_BOOT_DRIVER(security_tpm) = {
>> + .name = "security_tpm",
>> + .id = UCLASS_SECURITY,
>> + .priv_auto = sizeof(struct security_state),
>> + .of_match = tpm_security_ids,
>> + .of_to_plat = tpm_security_ofdata_to_platdata,
>> + .probe = tpm_security_probe,
>> + .remove = tpm_security_remove,
>> + .ops = &tpm_security_ops,
>> +};
>> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
>> index 2b6980e441..49bf0f0ba4 100644
>> --- a/include/tpm-v2.h
>> +++ b/include/tpm-v2.h
>> @@ -321,6 +321,7 @@ enum tpm2_return_codes {
>> TPM2_RC_COMMAND_CODE = TPM2_RC_VER1 + 0x0043,
>> TPM2_RC_AUTHSIZE = TPM2_RC_VER1 + 0x0044,
>> TPM2_RC_AUTH_CONTEXT = TPM2_RC_VER1 + 0x0045,
>> + TPM2_RC_NV_UNINITIALIZED = TPM2_RC_VER1 + 0x04a,
>> TPM2_RC_NV_DEFINED = TPM2_RC_VER1 + 0x004c,
>> TPM2_RC_NEEDS_TEST = TPM2_RC_VER1 + 0x0053,
>> TPM2_RC_WARN = 0x0900,
>> --
>> 2.40.0
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
2023-08-17 23:29 ` Sean Edmond
@ 2023-08-18 3:10 ` Simon Glass
0 siblings, 0 replies; 18+ messages in thread
From: Simon Glass @ 2023-08-18 3:10 UTC (permalink / raw)
To: Sean Edmond
Cc: U-Boot Mailing List, Stephen Carlson, Ilias Apalodimas,
Abdellatif El Khlifi
Hi Sean,
On Thu, 17 Aug 2023 at 17:29, Sean Edmond <seanedmond@linux.microsoft.com>
wrote:
>
> Hi Simon,
>
> On 2023-08-17 6:41 a.m., Simon Glass wrote:
> > Hi Sean,
> >
> > On Fri, 11 Aug 2023 at 18:28, <seanedmond@linux.microsoft.com> wrote:
> >> From: Stephen Carlson <stcarlso@linux.microsoft.com>
> >>
> >> This implementation of the security uclass driver allows existing TPM2
> >> devices declared in the device tree to be referenced for storing the OS
> >> anti-rollback counter, using the TPM2 non-volatile storage API.
> >>
> >> Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
> >> ---
> >> MAINTAINERS | 1 +
> >> drivers/security/Makefile | 1 +
> >> drivers/security/security-tpm.c | 173
++++++++++++++++++++++++++++++++
> >> include/tpm-v2.h | 1 +
> >> 4 files changed, 176 insertions(+)
> >> create mode 100644 drivers/security/security-tpm.c
> > This is a bit wonky w.r.t driver model. The TPM itself should
> > implement this API, perhaps ina separate file shared with all v2 TPMs.
> > You should not be opening the device, etc. in here.
> >
> > I hope that makes sense...you effectively need to turn the TPM into a
> > multi-function device within driver model. Of course TPMs are
> > multi-function devices anyway, but here you are making their functions
> > available more explicitly with a nicer API.
> >
> > Then you can call the TPM API to do what you want, but at least you
> > know that the TPM has been probed before you start.
> >
> > Regards,
> > Simon
> >
> Let's step back for a moment to understand our intention with this
feature.
>
> We want secure storage for the anti-rollback counter. The intention is
> that this storage is provided by whatever "secure driver" (let's start
> calling it the "rollback driver"). On our platform, we're using a
> different "rollback driver" which accesses a non-TPM based secure
> storage (which we can't upstream because it's proprietary). For the
> purpose of making this feature publicly available, we've added the
> TPM-backed "rollback driver" (this patch). I'll make this intention
> more clear in the documentation, which should lead to less confusion?
>
> At the end of the day, all we require is dm_security_arbvn_get() and
> dm_security_arbvn_set(), to get/set from the secure storage (we'll
> rename these to something less ugly, because yes arbvn is gross). We
> don't want to lock this feature to the TPM, because it doesn't enable
> us/others to choose a different secure storage mechanism.
It doesn't need to be locked to the TPM. But since you have a uclass you
can have different drivers implementing the same UCLASS_ROLLBACK:
- a 'stand-alone' one that does strage and secret things
- a TPM-based one that makes TPM calls
>
> W.r.t opening/initializing the TPM. Someone has to open it? In our
> case, it's being opened earlier with our measured boot, so "-EBUSY" in
> returned on tpm_open() (we return and everyone is happy). My
> understanding is that this is the currently available way for multiple
> drivers to access the TPM. Ilias has recommended we use
> tpm_auto_start(), which is an enhancement I'm planning to make. This
> should clean thing up? If this doesn't meet your expectations, can you
> describe in more detail how we should turn the TPM into a
> "multi-function device"?
The TPM will be probed automatically by probing its child device. Not
opened, though...that would have to happen elsewhere.
But it doesn't mean you need to turn the TPM into a multi-function device.
It's just that, in most cases, others would use a TPM to provide the
rollback counters.
For testing purposes, you should probably create a device which is a child
of the sandbox TPM2 and run the tests with that.
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-08-18 3:11 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-12 0:28 [PATCH 0/5] Add anti-rollback validation feature seanedmond
2023-08-12 0:28 ` [PATCH 1/5] drivers: security: Add security devices to driver model seanedmond
2023-08-16 13:14 ` Ilias Apalodimas
2023-08-17 13:41 ` Simon Glass
2023-08-12 0:28 ` [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices seanedmond
2023-08-14 8:39 ` Ilias Apalodimas
2023-08-14 21:23 ` Sean Edmond
2023-08-16 13:55 ` Ilias Apalodimas
2023-08-17 13:41 ` Simon Glass
2023-08-17 23:29 ` Sean Edmond
2023-08-18 3:10 ` Simon Glass
2023-08-12 0:28 ` [PATCH 3/5] common: Add OS anti-rollback validation using " seanedmond
2023-08-17 13:41 ` Simon Glass
2023-08-12 0:28 ` [PATCH 4/5] common: Add OS anti-rollback grace period seanedmond
2023-08-17 13:41 ` Simon Glass
2023-08-12 0:28 ` [PATCH 5/5] dm: test: Add a test for security driver seanedmond
2023-08-17 13:41 ` Simon Glass
2023-08-17 13:41 ` [PATCH 0/5] Add anti-rollback validation feature Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).