[PATCH 0/5] Add anti-rollback validation feature
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
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 */
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 DMhelpThis option enables support for the security uclass which supportsdevices intended to provide additional security features duringboot. These devices might encapsulate existing features of TPMor TEE devices, but can also be dedicated security processorsimplemented in specific hardware.+config SECURITY_SANDBOX
bool "Enable sandbox security driver"depends on DM_SECURITYhelpThis driver supports a simulated security device that uses volatilememory to store secure data and begins uninitialized. Thisimplementation allows OS images with security requirements to beloaded in the sandbox environment.+config SECURITY_TPM
bool "Enable TPM security driver"depends on TPM && TPM_V2 && DM_SECURITYhelpThis driver supports a security device based on existing TPMfunctionality.
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
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
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,
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
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
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
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
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
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
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
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_SIGNATUREdefault nhelpEnables FIT image anti-rollback protection. This feature is requiredwhen a platform needs to retire previous versions of FIT images due tosecurity 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
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);
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_ARBPdefault nhelpEnables a one unit grace period for FIT image anti-rollback protection,where anti-rollback protection will still accept a FIT image with ananti-rollback version one less than the current number, but will notupdate 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
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);
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
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
participants (4)
-
Ilias Apalodimas -
Sean Edmond -
seanedmond＠linux.microsoft.com
-
Simon Glass