Re: [PATCH 2/8] drivers: rollback: Add TPM2 implementation of rollback devices

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Sean,

On Tue, Sep 12, 2023 at 02:47:25AM -0700, seanedmond@linux.microsoft.com wrote:
> From: Stephen Carlson <stcarlso@linux.microsoft.com>
> 
> This implementation of the rollback 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.  The
> rollback device must be a child of the TPM device.  For example:
> 
> 	tpm2 {
> 		compatible = "sandbox,tpm2";
> 
> 		rollback@1 {
> 			compatible = "tpm,rollback";
> 			rollback-nv-index = <0x1001007>;
> 		};
> 	};
> 
This node is part of the DT specification right? If we accept this, we
should figure out if we can add that to the specification.

[...]

> +/*
> + * Copyright (c) 2021 Microsoft, Inc
> + * Written by Stephen Carlson <stcarlso@microsoft.com>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <rollback.h>
> +#include <tpm_api.h>
> +
> +struct rollback_state {
> +	u32 nv_index;
> +	struct udevice *tpm_dev;
> +};
> +
> +static int tpm_rollback_idx_get(struct udevice *dev, u64 *rollback_idx)
> +{
> +	struct rollback_state *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	if (!rollback_idx)
> +		return -EINVAL;
> +
> +	ret = tpm2_nv_read_value(priv->tpm_dev, priv->nv_index, rollback_idx, sizeof(u64));
> +	if (ret) {
> +		log(UCLASS_ROLLBACK, LOGL_ERR,

Wouldn't the init function below make sure the index is created? IOW
shouldn't this be a log_err()?

> +		    "Unable to read rollback number from TPM (ret=%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int tpm_rollback_idx_set(struct udevice *dev, u64 rollback_idx)
> +{
> +	int ret;
> +	struct rollback_state *priv = dev_get_priv(dev);
> +
> +	ret = tpm2_nv_write_value(priv->tpm_dev, priv->nv_index, &rollback_idx, sizeof(u64));
> +	if (ret) {
> +		log(UCLASS_ROLLBACK, LOGL_ERR,
> +		    "Unable to write anti-rollback version to TPM (ret=%d)\n", ret);

log_err()

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct rollback_ops tpm_rollback_ops = {
> +	.rollback_idx_get		= tpm_rollback_idx_get,
> +	.rollback_idx_set		= tpm_rollback_idx_set,
> +};
> +
> +static int tpm_rollback_probe(struct udevice *dev)
> +{
> +	struct rollback_state *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	/* initialize the TPM rollback counter NV index
> +	 * and initial to 0.  Note, this driver provides
> +	 * a NULL policy.
> +	 */
> +	ret = tpm_rollback_counter_init(priv->tpm_dev, priv->nv_index,
> +					NULL, 0);
> +	if (ret) {
> +		log(UCLASS_ROLLBACK, LOGL_ERR,
> +		    "TPM rollback init failed (ret=%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tpm_rollback_remove(struct udevice *dev)
> +{
> +	struct rollback_state *priv = dev_get_priv(dev);
> +
> +	return tpm_close(priv->tpm_dev);

I am not sure what this function is supposed to be doing. Why is it called
remove but only closes the tpm?

[...]

> +int tpm_rollback_counter_init(struct udevice *dev, u32 nv_index,
> +			      const u8 *nv_policy, size_t nv_policy_size)
> +{
> +	int ret;
> +	u64 data;
> +	u64 data0 = 0;
> +
> +	ret = tpm_open(dev);
> +	if (ret == -EBUSY) {
> +		log(UCLASS_ROLLBACK, LOGL_DEBUG,
> +		    "Existing TPM session found, reusing\n");
> +	} else {
> +		if (ret) {
> +			log(UCLASS_ROLLBACK, LOGL_ERR,
> +			    "TPM initialization failed (ret=%d)\n", ret);
> +			return ret;
> +		}
> +
> +		ret = tpm2_auto_start(dev);
> +		if (ret) {
> +			log(UCLASS_ROLLBACK, LOGL_ERR,
> +			    "TPM startup failed (ret=%d)\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (ret) {
> +		log_err("TPM startup failed\n");
> +		return ret;
> +	}
> +
> +	/* test reading NV index from TPM */
> +	ret = tpm2_nv_read_value(dev, nv_index, &data, sizeof(u64));
> +
> +	if (ret) {
> +		/*read failed.  Assume the NV index hasn't been defined yet */
> +		ret = tpm2_nv_define_space(dev, nv_index, sizeof(u64), TPMA_NV_PPREAD |
> +					   TPMA_NV_PPWRITE | TPMA_NV_PLATFORMCREATE,
> +					   nv_policy, nv_policy_size);
> +

Since the policy is NULL and no password is provided how do we trust this counter? 

The TPMs allow the creation of an 'NV Counter Index' which would be better
for what you are trying to do here.

> +		/* initialize the rollback counter to 0 */
> +		if (ret == TPM2_RC_NV_DEFINED || !ret)
> +			ret = tpm2_nv_write_value(dev, nv_index, &data0, sizeof(u64));
> +	}
> +
> +	return ret;
> +}
> -- 
> 2.40.0
> 

Thanks
/Ilias