On Wed, Oct 26, 2022 at 08:52:24AM -0400, James Bottomley wrote: > On Wed, 2022-10-26 at 10:59 +0100, Daniel P. Berrangé wrote: > > On Tue, Oct 25, 2022 at 07:38:43PM -0400, Cole Robinson wrote: > > > > > > This bytes([0]) NUL byte ends up in the efi_secret /sys path. > > > Dropping > > > it doesn't seem to impact injecting the secret at all > > > > The specs/sev-guest-firmware.rst files does not specify anything in > > particular about the injected secret format. The rules for the format > > will vary according to the GUID used for the entry. In this case what > > I've called the DISK_PW_GUID is the same as the GUID used in James > > Bottomley's grub patch: > > > > https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00260.html > > That's way too old; the latest version is here: > > https://lists.gnu.org/archive/html/grub-devel/2022-02/msg00064.html > > The specific change was to update to use the new disk protector API. Ah thanks, I should have googled harder ! > > In particular note > > > > + /* > > + * the passphrase must be a zero terminated string because > > the > > + * password routines call grub_strlen () to find its size > > + */ > > > > As written, the code just passes around a pointer to the data in the > > secret table, so it can't simply add a NUL terminator itself. > > > > See also James' python demo script for injection which adds a NUL: > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03691.html > > Which makes all of this unnecessary. The disk protector API takes a > pointer and a length, so no longer needs to call grub_strlen on the > data. Thus if we don't need backwards compatibility (or older versions > of grub have the disk protector API backported), the trailing NULL can > be removed. Assuming no distros have shipped with the not-yet-merged grub patches applied to their builds, I'd suggest we can ignore backwards compatibility concerns, and just focus on what will eventually be merged in grub upstream.... > The patch here: > > https://lists.gnu.org/archive/html/grub-devel/2022-02/msg00065.html > > Still has the grub_strlen, but it's now in the patch series not grub > itself so it could be eliminated by expanding the get API to retrieve > the length as well as the secret. ....which suggests we can just go for eliminating the strlen call and document that the existing GUID is intended to be 8-bit clean for binary keys, with no trailing NUL present. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
