Hi Sean On Sat, 12 Aug 2023 at 03:28, <seanedm...@linux.microsoft.com> wrote: > > From: Stephen Carlson <stcar...@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 <stcar...@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