Re: [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot
On 12 January 2018 at 11:24, Daniel Kiper wrote: > Hi Ard, > > On Thu, Jan 11, 2018 at 12:51:07PM +, Ard Biesheuvel wrote: >> On 9 January 2018 at 14:22, Daniel Kiper wrote: >> > Hi, >> > >> > Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel >> > may not even know that it runs on secure boot enabled platform. >> >> Hi Daniel, >> >> I must say, I am not too thrilled with the approach you have chosen >> here. #including .c files in other .c files, and using #defines to >> override C functions or other stub functionality is rather fragile. In > > TBH I do not like it too. Sadly I have not find a better solution for > that. I wish to avoid code duplication as much as possible because > otherwise it will fall out of sync sooner or later (usually sooner). > Similar thing happened in different part of Xen EFI code a few months ago. > >> particular, it means we have to start caring about not breaking >> Xen/x86 code when making modifications to the EFI stub, and that code >> is already difficult enough to maintain, given that it is shared >> between ARM, arm64 and x86, and runs either from the decompressor or >> the kernel proper (arm64) but in the context of the UEFI firmware. > > I understand that. > >> None of the stub code currently runs in ordinary kernel context. > > Yep. > >> So please, could you try to find another way to do this? > > I am happy to improve the situation, however, I am afraid that it is > difficult here. Stub and kernel proper are separate entities and simple > linking does not work. So, It seems to me that only play with includes > will allow us to not duplicate the code. However, if you have a better > idea I am happy to implement it. > Actually, there is another reason why it does not make sense to reuse that code. This code /* * See if a user has put the shim into insecure mode. If so, and if the * variable doesn't have the runtime attribute set, we might as well * honor that. */ size = sizeof(moksbstate); status = get_efi_var(L"MokSBState", &shim_guid, &attr, &size, &moksbstate); /* If it fails, we don't care why. Default to secure */ if (status != EFI_SUCCESS) goto secure_boot_enabled; if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1) return efi_secureboot_mode_disabled; will always fail after exiting boot services, so it makes no sense to call it from xen_efi_init(). So I suggest you just clone the function and only keep the pieces that make sense for Xen. -- Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot
Hi Ard, On Thu, Jan 11, 2018 at 12:51:07PM +, Ard Biesheuvel wrote: > On 9 January 2018 at 14:22, Daniel Kiper wrote: > > Hi, > > > > Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel > > may not even know that it runs on secure boot enabled platform. > > Hi Daniel, > > I must say, I am not too thrilled with the approach you have chosen > here. #including .c files in other .c files, and using #defines to > override C functions or other stub functionality is rather fragile. In TBH I do not like it too. Sadly I have not find a better solution for that. I wish to avoid code duplication as much as possible because otherwise it will fall out of sync sooner or later (usually sooner). Similar thing happened in different part of Xen EFI code a few months ago. > particular, it means we have to start caring about not breaking > Xen/x86 code when making modifications to the EFI stub, and that code > is already difficult enough to maintain, given that it is shared > between ARM, arm64 and x86, and runs either from the decompressor or > the kernel proper (arm64) but in the context of the UEFI firmware. I understand that. > None of the stub code currently runs in ordinary kernel context. Yep. > So please, could you try to find another way to do this? I am happy to improve the situation, however, I am afraid that it is difficult here. Stub and kernel proper are separate entities and simple linking does not work. So, It seems to me that only play with includes will allow us to not duplicate the code. However, if you have a better idea I am happy to implement it. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot
On 9 January 2018 at 14:22, Daniel Kiper wrote: > Hi, > > Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel > may not even know that it runs on secure boot enabled platform. > Hi Daniel, I must say, I am not too thrilled with the approach you have chosen here. #including .c files in other .c files, and using #defines to override C functions or other stub functionality is rather fragile. In particular, it means we have to start caring about not breaking Xen/x86 code when making modifications to the EFI stub, and that code is already difficult enough to maintain, given that it is shared between ARM, arm64 and x86, and runs either from the decompressor or the kernel proper (arm64) but in the context of the UEFI firmware. None of the stub code currently runs in ordinary kernel context. So please, could you try to find another way to do this? Thanks, Ard. > > arch/x86/xen/Makefile |4 +++- > arch/x86/xen/efi.c | 14 + > drivers/firmware/efi/libstub/secureboot-core.c | 77 > + > drivers/firmware/efi/libstub/secureboot.c | 66 > +-- > 4 files changed, 99 insertions(+), 62 deletions(-) > > Daniel Kiper (4): > efi/stub: Extract efi_get_secureboot() to separate file > x86/xen/efi: Initialize boot_params.secure_boot in xen_efi_init() > efi: Tweak efi_get_secureboot() and its data section assignment > efi: Rename efi_get_secureboot() to __efi_get_secureboot() and make it > static > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot
Hi, Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel may not even know that it runs on secure boot enabled platform. Daniel arch/x86/xen/Makefile |4 +++- arch/x86/xen/efi.c | 14 + drivers/firmware/efi/libstub/secureboot-core.c | 77 + drivers/firmware/efi/libstub/secureboot.c | 66 +-- 4 files changed, 99 insertions(+), 62 deletions(-) Daniel Kiper (4): efi/stub: Extract efi_get_secureboot() to separate file x86/xen/efi: Initialize boot_params.secure_boot in xen_efi_init() efi: Tweak efi_get_secureboot() and its data section assignment efi: Rename efi_get_secureboot() to __efi_get_secureboot() and make it static -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
