Re: [PATCH 1/4] tsm: Add TVM Measurement Register support

["Huang, Kai" <kai.huang@intel.com>]

Hi Cedric,

[...]

> +static ssize_t tmr_digest_read(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> +			       char *page, loff_t off, size_t count)

Better to rename 'page' to 'buffer'?

Since page normally implies 4KB alignment but I don't see we need the 
alignment here.

> +{
> +	const struct tsm_measurement_register *mr;
> +	struct tmr_provider *pvd;
> +	int rc;
> +
> +	if (off < 0 || off > attr->size)
> +		return -EINVAL;
> +
> +	count = min(count, attr->size - (size_t)off);
> +	if (!count)
> +		return count;
> +
> +	mr = (typeof(mr))attr->private;
> +	pvd = tmr_mr_to_provider(mr, kobj);
> +	rc = down_read_interruptible(&pvd->rwsem);
> +	if (rc)
> +		return rc;
> +
> +	if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
> +		up_read(&pvd->rwsem);
> +
> +		rc = down_write_killable(&pvd->rwsem);
> +		if (rc)
> +			return rc;
> +
> +		if (!pvd->in_sync) {
> +			rc = tmr_call_refresh(pvd, mr);
> +			pvd->in_sync = !rc;
> +		}
> +
> +		downgrade_write(&pvd->rwsem);
> +	}
> +
> +	if (!rc)
> +		memcpy(page, mr->mr_value + off, count);
> +
> +	up_read(&pvd->rwsem);
> +	return rc ?: count;
> +}
> +
> +static ssize_t tmr_digest_write(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> +				char *page, loff_t off, size_t count)
> +{
> +	const struct tsm_measurement_register *mr;
> +	struct tmr_provider *pvd;
> +	ssize_t rc;
> +
> +	if (off != 0 || count != attr->size)
> +		return -EINVAL;
> +
> +	mr = (typeof(mr))attr->private;
> +	pvd = tmr_mr_to_provider(mr, kobj);
> +	rc = down_write_killable(&pvd->rwsem);
> +	if (rc)
> +		return rc;
> +
> +	if (mr->mr_flags & TSM_MR_F_X)
> +		rc = tmr_call_extend(pvd, mr, page);
> +	else
> +		memcpy(mr->mr_value, page, count);
> +
> +	if (!rc)
> +		pvd->in_sync = false;
> +
> +	up_write(&pvd->rwsem);
> +	return rc ?: count;
> +}

The logic around using pvd->in_sync is kinda complicated.  MR operations 
seem like a classic reader/writer contention problem and I am not sure 
why pvd->in_sync is needed.  Could you help to clarify?

[...]

> +
> +/**
> + * struct tsm_measurement_register - describes an architectural measurement register (MR)
> + * @mr_name: name of the MR
> + * @mr_value: buffer containing the current value of the MR
> + * @mr_size: size of the MR - typically the digest size of @mr_hash
> + * @mr_flags: bitwise OR of flags defined in enum tsm_measurement_register_flag
> + * @mr_hash: optional hash identifier defined in include/uapi/linux/hash_info.h
> + *
> + * A CC guest driver provides this structure to detail the measurement facility supported by the
> + * underlying CC hardware. After registration via `tsm_register_measurement`, the CC guest driver
> + * must retain this structure until it is unregistered using `tsm_unregister_measurement`.
> + */
> +struct tsm_measurement_register {
> +	const char *mr_name;
> +	void *mr_value;
> +	u32 mr_size;
> +	u32 mr_flags;
> +	enum hash_algo mr_hash;
> +};
> +
> +/**
> + * enum tsm_measurement_register_flag - properties of an MR
> + * @TSM_MR_F_X: this MR supports the extension semantics on write
> + * @TSM_MR_F_W: this MR is writable

Why a MR can be written w/o being extended?  What is the use case of this?

> + * @TSM_MR_F_R: this MR is readable. This should typically be set
> + * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR

Why one MR can be changed by writing to other MRs?

> + * @TSM_MR_F_F: present this MR as a file (instead of a directory)
> + * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
> + * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
> + */
> +enum tsm_measurement_register_flag {
> +	TSM_MR_F_X = 1,
> +	TSM_MR_F_W = 2,
> +	TSM_MR_F_R = 4,
> +	TSM_MR_F_L = 8,
> +	TSM_MR_F_F = 16,
> +	TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
> +	TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
> +};

I am not sure whether we need so many flags.  To me seems like we only need:

  - TSM_MR_ENABLED:  The MR has been initialized with a certain algo.
  - TSM_MR_UNLOCKED: The MR is writable and any write will extend it.
  - TSM_MR_LOCKED:   The MR is locked and finalized.

The TSM_MR_ENABLED may not be needed either, but I think it's better to 
have it so that the kernel can reject both read/write from userspace.

> +
> +#define TSM_MR_(mr, hash)                                                           \
> +	.mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash = HASH_ALGO_##hash, \
> +	.mr_flags = TSM_MR_F_R
> +
> +/**
> + * struct tsm_measurement - define CC specific MRs and methods for updating them
> + * @name: name of the measurement provider
> + * @mrs: array of MR definitions ending with mr_name set to %NULL
> + * @refresh: invoked to update the specified MR
> + * @extend: invoked to extend the specified MR with mr_size bytes
> + */
> +struct tsm_measurement {
> +	const char *name;
> +	const struct tsm_measurement_register *mrs;
> +	int (*refresh)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr);
> +	int (*extend)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr,
> +		      const u8 *data);
> +};

 From the description above, I don't quite follow what does ->refresh() 
do exactly.  Could you clarify why we need it?