Messages in this thread | | | From | Dionna Amalie Glaze <> | Date | Thu, 31 Aug 2023 14:42:56 -0700 | Subject | Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports |
| |
This is clean and approachable. Thanks for your implementation.
> +static int try_advance_write_generation(struct tsm_report *report) > +{ > + lockdep_assert_held_write(&tsm_rwsem); > + > + /* > + * malicious or broken userspace is attempting to wrap read_generation, > + * stop accepting updates until current report configuration is read. > + */ > + if (report->write_generation == report->read_generation - 1) > + return -EBUSY; > + report->write_generation++; > + return 0; > +} > +
This took me a while to wrap my head around. The property we want is that when we read outblob, it is the result of the attribute changes since the last read. If we write to an attribute 2^64 times, we could get write_generation to wrap around to equal read_generation without actually reading outblob. So we could end up given a badly cached result when we read outblob? Is that what this is preventing?
I think I would word this to say,
"Malicious or broken userspace has written enough times for read_generation == write_generation by modular arithmetic without an interim read. Stop accepting updates until the current report configuration is read."
I think that at least in the SEV-SNP case, we can double-check from userspace that the report has values that we expected to configure the get_report with, so this isn't really an issue. I'm not sure what the use is of configuration that doesn't lead to observable (and checkable) differences, but I suppose this check doesn't hurt.
-- -Dionna Glaze, PhD (she/her)
| |