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 :|