| Message ID | 20200914115013.814079-5-hudson@trmm.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: Unified Xen hypervisor/kernel/initrd images | expand |
On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote: > If secure boot is enabled, the Xen command line arguments are ignored. > If a unified Xen image is used, then the bundled configuration, dom0 > kernel, and initrd are prefered over the ones listed in the config file. I understand that you must ignore the cfg option when using the bundled image, but is there then an alternative way for passing the basevideo and mapbs parameters? Or there's simply no way of doing so when using secure boot with a bundled image? > Unlike the shim based verification, the PE signature on a unified image > covers the all of the Xen+config+kernel+initrd modules linked into the Extra 'the'. > unified image. This also ensures that properly configured platforms > will measure the entire runtime into the TPM for unsealing secrets or > remote attestation. > > Signed-off-by: Trammell Hudson <hudson@trmm.net> > --- > xen/common/efi/boot.c | 44 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 41 insertions(+), 3 deletions(-) > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index 4b1fbc9643..e65c1f1a09 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -949,6 +949,39 @@ static void __init setup_efi_pci(void) > efi_bs->FreePool(handles); > } > > +/* > + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c > + * Secure Boot is enabled iff 'SecureBoot' is set and the system is > + * not in Setup Mode. > + */ > +static bool __init efi_secure_boot(void) > +{ > + static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; > + uint8_t secboot, setupmode; > + UINTN secboot_size = sizeof(secboot); > + UINTN setupmode_size = sizeof(setupmode); > + EFI_STATUS rc; > + > + rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, > + NULL, &secboot_size, &secboot); > + if ( rc != EFI_SUCCESS ) > + return false; > + > + rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, > + NULL, &setupmode_size, &setupmode); > + if ( rc != EFI_SUCCESS ) > + return false; > + > + if ( secboot > 1) Nit: missing space before closing parentheses. > + { > + PrintStr(L"Invalid SecureBoot variable=0x"); > + DisplayUint(secboot, 2); Maybe better to use secboot_size * 2 here since you already have the size of the variable anyway? Thanks, Roger.
On Wednesday, September 16, 2020 3:45 AM, Roger Pau Monné <roger.pau@citrix.com> wrote: > On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote: > > If secure boot is enabled, the Xen command line arguments are ignored. > > If a unified Xen image is used, then the bundled configuration, dom0 > > kernel, and initrd are prefered over the ones listed in the config file. > > I understand that you must ignore the cfg option when using the > bundled image, but is there then an alternative way for passing the > basevideo and mapbs parameters? The cfg option will be ignored regardless since a bundled config (or kernel, ramdisk, etc) takes precedence over any files, so perhaps parsing the command line is not as much of a risk as initially thought. The concern is that *any* non-signed configuration values are potentially a risk, even if we don't see exactly how the attacker can use them right now. Especially if an option is added later and we haven't thought about the security ramifications of it. > Or there's simply no way of doing so when using secure boot with a > bundled image? Should these options be available in the config file instead? That way the system owner can sign the configuration and ensure that an adversary can't change them. > > Unlike the shim based verification, the PE signature on a unified image > > covers the all of the Xen+config+kernel+initrd modules linked into the > > Extra 'the'. Fixed, along with the style issues in upcoming v5. -- Trammell
On 16.09.2020 10:50, Trammell Hudson wrote: > On Wednesday, September 16, 2020 3:45 AM, Roger Pau Monné <roger.pau@citrix.com> wrote: >> On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote: >>> If secure boot is enabled, the Xen command line arguments are ignored. >>> If a unified Xen image is used, then the bundled configuration, dom0 >>> kernel, and initrd are prefered over the ones listed in the config file. >> >> I understand that you must ignore the cfg option when using the >> bundled image, but is there then an alternative way for passing the >> basevideo and mapbs parameters? > > The cfg option will be ignored regardless since a bundled config > (or kernel, ramdisk, etc) takes precedence over any files, > so perhaps parsing the command line is not as much of a risk > as initially thought. > > The concern is that *any* non-signed configuration values are > potentially a risk, even if we don't see exactly how the attacker > can use them right now. Especially if an option is added later > and we haven't thought about the security ramifications of it. > >> Or there's simply no way of doing so when using secure boot with a >> bundled image? > > Should these options be available in the config file instead? > That way the system owner can sign the configuration and ensure > that an adversary can't change them. Not really, no. /basevideo needs evaluating very early in any event, before any (regular) output gets produced. /mapbs could be parsed later, but the early boot code intentionally does not make any attempt at parsing the command line options designated for the common parts of xen.gz and xen.efi. Yet the map_bs variable has one use in early boot code (i.e. before handing over to __start_xen()). Jan
On 14.09.2020 13:50, Trammell Hudson wrote: > If secure boot is enabled, the Xen command line arguments are ignored. > If a unified Xen image is used, then the bundled configuration, dom0 > kernel, and initrd are prefered over the ones listed in the config file. > > Unlike the shim based verification, the PE signature on a unified image > covers the all of the Xen+config+kernel+initrd modules linked into the > unified image. This also ensures that properly configured platforms > will measure the entire runtime into the TPM for unsealing secrets or > remote attestation. The command line may also include a part handed on to the Dom0 kernel. If the Dom0 kernel image comes from disk, I don't see why that part of the command line shouldn't be honored. Similarly, if the config file doesn't come from the unified image, I think Xen's command line options should also be honored. > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -949,6 +949,39 @@ static void __init setup_efi_pci(void) > efi_bs->FreePool(handles); > } > > +/* > + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c > + * Secure Boot is enabled iff 'SecureBoot' is set and the system is > + * not in Setup Mode. > + */ > +static bool __init efi_secure_boot(void) > +{ > + static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; > + uint8_t secboot, setupmode; > + UINTN secboot_size = sizeof(secboot); > + UINTN setupmode_size = sizeof(setupmode); > + EFI_STATUS rc; > + > + rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, As you need the casts here just to get rid of the const again, please don't make the variable const in the first place. > + NULL, &secboot_size, &secboot); > + if ( rc != EFI_SUCCESS ) > + return false; > + > + rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, > + NULL, &setupmode_size, &setupmode); > + if ( rc != EFI_SUCCESS ) > + return false; > + > + if ( secboot > 1) > + { > + PrintStr(L"Invalid SecureBoot variable=0x"); > + DisplayUint(secboot, 2); > + PrintStr(newline); > + } > + > + return secboot == 1 && setupmode == 0; Like for SecureBoot, values other than 0 and 1 also are reserved for SetupMode and hence would better be logged in the same way. I'm still unconvinced though that logging is enough. > @@ -1134,6 +1167,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > bool base_video = false; > char *option_str; > bool use_cfg_file; > + bool secure = false; I don't think the initializer is needed here? > @@ -1152,8 +1186,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > PrintErrMesg(L"No Loaded Image Protocol", status); > > efi_arch_load_addr_check(loaded_image); > + secure = efi_secure_boot(); > > - if ( use_cfg_file ) > + /* If UEFI Secure Boot is enabled, do not parse the command line */ > + if ( use_cfg_file && !secure ) > { If it is intentional to also change this for the shim case, then please justify the change in behavior in the description. As per the comments further up I think the ignoring of the various parts wants making depend on more than just secure boot mode anyway. Jan
On Thursday, September 17, 2020 8:51 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 14.09.2020 13:50, Trammell Hudson wrote: > > If secure boot is enabled, the Xen command line arguments are ignored. > > If a unified Xen image is used, then the bundled configuration, dom0 > > kernel, and initrd are prefered over the ones listed in the config file. > > Unlike the shim based verification, the PE signature on a unified image > > covers the all of the Xen+config+kernel+initrd modules linked into the > > unified image. This also ensures that properly configured platforms > > will measure the entire runtime into the TPM for unsealing secrets or > > remote attestation. > > The command line may also include a part handed on to the Dom0 kernel. > If the Dom0 kernel image comes from disk, I don't see why that part of > the command line shouldn't be honored. Similarly, if the config file > doesn't come from the unified image, I think Xen's command line options > should also be honored. Ignoring the command line and breaking the shim behaviour in a unified image should be ok; that is an explicit decision by the system owner to sign and configure the new image (and the shim is not used in a unified image anyway). If we have a way to detect a unified image early enough, then we can avoid the backwards incompatibility if it is not unified. That would require moving the config parsing to above the relocation call. I'm testing that now to see if it works on x86. -- Trammell
On 17.09.2020 16:05, Trammell Hudson wrote: > On Thursday, September 17, 2020 8:51 AM, Jan Beulich <jbeulich@suse.com> wrote: >> On 14.09.2020 13:50, Trammell Hudson wrote: >>> If secure boot is enabled, the Xen command line arguments are ignored. >>> If a unified Xen image is used, then the bundled configuration, dom0 >>> kernel, and initrd are prefered over the ones listed in the config file. >>> Unlike the shim based verification, the PE signature on a unified image >>> covers the all of the Xen+config+kernel+initrd modules linked into the >>> unified image. This also ensures that properly configured platforms >>> will measure the entire runtime into the TPM for unsealing secrets or >>> remote attestation. >> >> The command line may also include a part handed on to the Dom0 kernel. >> If the Dom0 kernel image comes from disk, I don't see why that part of >> the command line shouldn't be honored. Similarly, if the config file >> doesn't come from the unified image, I think Xen's command line options >> should also be honored. > > Ignoring the command line and breaking the shim behaviour in a > unified image should be ok; that is an explicit decision by the > system owner to sign and configure the new image (and the shim > is not used in a unified image anyway). > > If we have a way to detect a unified image early enough, then > we can avoid the backwards incompatibility if it is not unified. I was assuming this was easily possible, if necessary as about the first thing we do. If it's not as easy, perhaps something wants adding to make it so? > That would require moving the config parsing to above the relocation > call. I guess I don't understand why this would be. Jan
On Thursday, September 17, 2020 11:26 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 17.09.2020 16:05, Trammell Hudson wrote: > > If we have a way to detect a unified image early enough, then > > we can avoid the backwards incompatibility if it is not unified. > > I was assuming this was easily possible, if necessary as about the > first thing we do. If it's not as easy, perhaps something wants > adding to make it so? v5 of the patch (just sent) has changed the logic so that the config section is searched first thing, and if it is found, then and only then is the command line ignored. I believe this restores the older behaviour. > > That would require moving the config parsing to above the relocation > > call. > > I guess I don't understand why this would be. The early command line parsing happens before the call to efi_arch_relocate_image(), although testing in qemu did not seem to cause any problems with calling read_section() before the reloc. -- Trammell
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 4b1fbc9643..e65c1f1a09 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -949,6 +949,39 @@ static void __init setup_efi_pci(void) efi_bs->FreePool(handles); } +/* + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c + * Secure Boot is enabled iff 'SecureBoot' is set and the system is + * not in Setup Mode. + */ +static bool __init efi_secure_boot(void) +{ + static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; + uint8_t secboot, setupmode; + UINTN secboot_size = sizeof(secboot); + UINTN setupmode_size = sizeof(setupmode); + EFI_STATUS rc; + + rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, + NULL, &secboot_size, &secboot); + if ( rc != EFI_SUCCESS ) + return false; + + rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, + NULL, &setupmode_size, &setupmode); + if ( rc != EFI_SUCCESS ) + return false; + + if ( secboot > 1) + { + PrintStr(L"Invalid SecureBoot variable=0x"); + DisplayUint(secboot, 2); + PrintStr(newline); + } + + return secboot == 1 && setupmode == 0; +} + static void __init efi_variables(void) { EFI_STATUS status; @@ -1125,8 +1158,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; EFI_LOADED_IMAGE *loaded_image; EFI_STATUS status; - unsigned int i, argc; - CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; + unsigned int i, argc = 0; + CHAR16 **argv = NULL, *file_name, *cfg_file_name = NULL, *options = NULL; UINTN gop_mode = ~0; EFI_SHIM_LOCK_PROTOCOL *shim_lock; EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; @@ -1134,6 +1167,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) bool base_video = false; char *option_str; bool use_cfg_file; + bool secure = false; __set_bit(EFI_BOOT, &efi_flags); __set_bit(EFI_LOADER, &efi_flags); @@ -1152,8 +1186,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintErrMesg(L"No Loaded Image Protocol", status); efi_arch_load_addr_check(loaded_image); + secure = efi_secure_boot(); - if ( use_cfg_file ) + /* If UEFI Secure Boot is enabled, do not parse the command line */ + if ( use_cfg_file && !secure ) { UINTN offset = 0; @@ -1211,6 +1247,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION) XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n"); + if ( secure ) + PrintStr(L"UEFI Secure Boot enabled\r\n"); efi_arch_relocate_image(0);
If secure boot is enabled, the Xen command line arguments are ignored. If a unified Xen image is used, then the bundled configuration, dom0 kernel, and initrd are prefered over the ones listed in the config file. Unlike the shim based verification, the PE signature on a unified image covers the all of the Xen+config+kernel+initrd modules linked into the unified image. This also ensures that properly configured platforms will measure the entire runtime into the TPM for unsealing secrets or remote attestation. Signed-off-by: Trammell Hudson <hudson@trmm.net> --- xen/common/efi/boot.c | 44 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-)