| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <828df2dd-a099-4146-96fe-0915cfa2e4b5@intel.com>
Date: Mon, 17 Feb 2025 13:17:39 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: Cedric Xing <cedric.xing@...el.com>, Dan Williams
<dan.j.williams@...el.com>, "Kirill A. Shutemov"
<kirill.shutemov@...ux.intel.com>, Dave Hansen <dave.hansen@...ux.intel.com>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, <x86@...nel.org>, "H. Peter Anvin"
<hpa@...or.com>
CC: <linux-kernel@...r.kernel.org>, <linux-coco@...ts.linux.dev>
Subject: Re: [PATCH 1/4] tsm: Add TVM Measurement Register support
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?
Powered by blists - more mailing lists