Message-ID: <fc262405-451e-3842-9b08-bc36614e810a@amd.com>
Date:   Tue, 8 Nov 2022 12:17:19 -0600
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Evgeniy Baskov <baskov@...ras.ru>
Cc:     Ard Biesheuvel <ardb@...nel.org>, Peter Jones <pjones@...hat.com>,
        Borislav Petkov <bp@...en8.de>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alexey Khoroshilov <khoroshilov@...ras.ru>,
        lvc-project@...uxtesting.org, x86@...nel.org,
        linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 00/23] x86_64: Improvements at compressed kernel stage

On 11/8/2022 01:01, Evgeniy Baskov wrote:
> On 2022-11-04 21:21, Limonciello, Mario wrote:
>> On 10/25/2022 09:12, Evgeniy Baskov wrote:
>>> ...
>>>
>>
>> Hi,
>>
>> I was talking to Peter Jones recently about what was still missing for
>> NX support in the kernel and he pointed me at this series.
>>
>> So I had a try with this series on top of:
>>
>> ee6050c8af96 ("Merge tag 'ata-6.1-rc4' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata")
>>
>> Unfortunately I can't boot the system with this series applied.
>> This is not on a system that enforces NX pre-boot (but that was my
>> goal after I could prove booting on something that doesn't).
>> I didn't apply Peter's patch 6 you referenced in your cover letter,
>> but I don't expect that's the reason for the failure.
>>
>> I get:
>>
>> "Failed to allocate space for tmp_cmdline"
>>
>>    -- System Halted
>>
>> This is early enough [1] that I don't have anything else output to a
>> serial log from the kernel.
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fd4013bc4d49f6da8178a340348369bb9920225c9%2Farch%2Fx86%2Fboot%2Fcompressed%2Fkaslr.c%23L268&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C9280e92e85bc4e2b52cd08dac15704a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638034876740462327%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=oiCJUa8M3x%2FYCxVjJj98R7iU%2FzIj%2FQxdVOWbnqGWCNI%3D&amp;reserved=0
>>
>> Since this is only in the kaslr path, I tried to turn that off with
>> 'nokaslr' on the kernel command line.
>>
>> I then get a failure of:
>>
>> "Out of memory while allocating zstd_dctx"
>>
>>   -- System Halted
>>
>> This kernel was booted from the following path:
>> -> Insyde BIOS
>> --> shim (from Fedora 36 repository)
>> ---> GRUB (from Peter for Fedora 36 w/ some level NX support)
>> ----> kernel binary (self-built)
>>
>> The BIOS on this system doesn't validate NX, but also the shim binary
>> did not have the NX bit set in the PE header.
>>
>> Your cover letter referenced CONFIG_EFI_STUB_EXTRACT_DIRECT but I
>> didn't find this option in the series.  I also tried both with
>> CONFIG_EFI_DXE_MEM_ATTRIBUTES=y or unset, same result.
> 
> Hi,
> 
> Thanks for your feedback!
> 

Sure!

> CONFIG_EFI_STUB_EXTRACT_DIRECT option was removed in v2 of the series
> and direct extraction is unconditional now.
> 
> You are getting really weird errors, which unfortunately I am unable
> to reproduce yet. I've tried booting with fedora's grub and the series
> applied on top of ee6050c8af96, but it did boot successfully.

Well so I expect the unique difference is that I'm using Peter's GRUB 
that has some NX support landed.  He has binaries for it here:

https://blog.uncooperative.org/~pjones/nx/repo/

*Theoretically* a BIOS that enforces NX should be able to boot a shim with
the NX compat bit set which should be able to boot that GRUB supporting 
NX which should be able to boot this series.

> 
>  From the error messages it's some problem with malloc() implementation
> of compressed kernel code. I suspect that malloc_ptr inside .bss is not
> zeroed. This should not happen when booting via either non-UEFI
> interface, or via UEFI (when kernel is properly loaded as PE image).
> The problem, I think, arises when kernel is loaded as a blob, but EFI
> handover protocol is used to start its execution. This is what grub
> seems to be doing.
> 
> Can you please try booting with patches below applied on top?
> If this fixes the problem, I'll include these changes in v3.

Yup, spot on.  I can the kernel from Peter's NX enabled GRUB now with:
* 6.1-rc4
* Your existing 23 patch series
* this new patch

Thanks!!

Would you mind CC me when you submit v3?  As I have an interest in 
seeing NX support I'd like to continue to follow along on the series.

Anything in the series you don't change in any material way from v2 
please feel free to include:

Tested-by: Mario Limonciello <mario.limonciello@....com>

> 
> Thanks,
> Evgeniy Baskov
> -- 
> diff --git a/arch/x86/boot/compressed/head_32.S 
> b/arch/x86/boot/compressed/head_32.S
> index 9871cc8466fb..69811d9ab4ce 100644
> --- a/arch/x86/boot/compressed/head_32.S
> +++ b/arch/x86/boot/compressed/head_32.S
> @@ -152,6 +152,14 @@ SYM_FUNC_END(startup_32)
> 
>   #ifdef CONFIG_EFI_STUB
>   SYM_FUNC_START(efi32_stub_entry)
> +    /* Clear BSS */
> +    xorl    %eax, %eax
> +    leal    _bss@...OFF(%ebx), %edi
> +    leal    _ebss@...OFF(%ebx), %ecx
> +    subl    %edi, %ecx
> +    shrl    $2, %ecx
> +    rep    stosl
> +
>       add    $0x4, %esp
>       movl    8(%esp), %esi    /* save boot_params pointer */
>       call    efi_main
> diff --git a/arch/x86/boot/compressed/head_64.S 
> b/arch/x86/boot/compressed/head_64.S
> index 2bb0e6da08c0..384706d12354 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -553,8 +553,20 @@ SYM_CODE_END(startup_64)
>   #ifdef CONFIG_EFI_STUB
>       .org 0x390
>   SYM_FUNC_START(efi64_stub_entry)
> +    /* Preserve first parameter */
> +    movq    %rdi, %r10
> +
> +    /* Clear BSS */
> +    xorl    %eax, %eax
> +    leaq    _bss(%rip), %rdi
> +    leaq    _ebss(%rip), %rcx
> +    subq    %rdi, %rcx
> +    shrq    $3, %rcx
> +    rep    stosq
> +
>       and    $~0xf, %rsp            /* realign the stack */
>       movq    %rdx, %rbx            /* save boot_params pointer */
> +    movq    %r10, %rdi
>       call    efi_main
> 
>       cld
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c 
> b/drivers/firmware/efi/libstub/x86-stub.c
> index 95a69c37518e..1a2c52e77c6e 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -24,7 +24,7 @@
> 
>   const efi_system_table_t *efi_system_table;
>   extern u32 image_offset;
> -static efi_loaded_image_t *image = NULL;
> +static efi_loaded_image_t *image __section(".data");
> 
>   extern char _head[], _ehead[];
>   extern char _compressed[], _ecompressed[];

Powered by blists - more mailing lists