[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