| 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: <e9a0953921530a6ec058b8236e45d2f4217d9c0e.camel@intel.com>
Date: Mon, 17 Feb 2025 10:44:49 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"bp@...en8.de" <bp@...en8.de>, "x86@...nel.org" <x86@...nel.org>,
"mingo@...hat.com" <mingo@...hat.com>, "Williams, Dan J"
<dan.j.williams@...el.com>, "kirill.shutemov@...ux.intel.com"
<kirill.shutemov@...ux.intel.com>, "Xing, Cedric" <cedric.xing@...el.com>,
"tglx@...utronix.de" <tglx@...utronix.de>, "hpa@...or.com" <hpa@...or.com>
CC: "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] tsm: Add TVM Measurement Register support
On Mon, 2025-02-17 at 13:17 +1300, Huang, Kai wrote:
> 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?
>
> [...]
[...]
> >
> > +#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?
>
After reading patch 4, I figured out the purpose of the pvd->in_sync and the
refresh() myself:
Each call of ->extend() will update the value of the MR, but only in the
hardware/firmware. The MR value maintained by the kernel is unchanged so it
will be out-of-date after the ->extend(). Therefore the kernel needs to keep
them synced first when userspace reads the MR, using the ->refresh().
It's also feasible to always sync/refresh after each extend(), eliminating the
pvd->in_sync logic completely, but it's not desired -- because the sync
operation talks to the hardware/firmware thus could be time-consuming, and we in
general only needs to sync before reading the value to the userspace. E.g.,
this saves a lot of sync operation if userspace extends the MR multi-times with
large chunk of data and then reads the final value once.
So the pvd->in_sync logic seems useful to me, but I think it would be better to
a comment to explain this in both tmr_digest_read() and tmr_digest_write().
E.g., something like below before the code which checks pvd->in_sync in the
tmr_digest_read()?
/*
* Each write-and-extend to the MR will update the MR, but only in
* TSM hardware/firmware, leaving the MR value maintained by the kernel
* out-of-date. Sync MR value in TSM hardware/firmware to the kernel
* before returning to the userspace in this case.
*/
if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
...
The description of the refresh() callback is also not clear to me. Perhaps
something like below?
/*
* ...
* @refresh: sync MR value in TSM hardware/firmware to the kernel
buffer
* ...
*/
Powered by blists - more mailing lists