[PATCH 2/5] drivers: security: Add TPM2 implementation of security devices

Ilias Apalodimas ilias.apalodimas at linaro.org
Mon Aug 14 10:39:24 CEST 2023


Hi Sean

On Sat, 12 Aug 2023 at 03:28, <seanedmond at linux.microsoft.com> wrote:
>
> From: Stephen Carlson <stcarlso at 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 at 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


More information about the U-Boot mailing list