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