linux-hardening - Re: [PATCH v4 20/26] x86/build: Make generated PE more spec compliant

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 12 Mar 2023 15:01:53 +0300
From:   Evgeniy Baskov <baskov@...ras.ru>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     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>,
        Peter Jones <pjones@...hat.com>,
        "Limonciello, Mario" <mario.limonciello@....com>,
        joeyli <jlee@...e.com>, 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 v4 20/26] x86/build: Make generated PE more spec compliant

On 2023-03-11 20:31, Ard Biesheuvel wrote:
> On Sat, 11 Mar 2023 at 16:02, Evgeniy Baskov <baskov@...ras.ru> wrote:
>> 
>> On 2023-03-10 18:17, Ard Biesheuvel wrote:
>> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@...ras.ru> wrote:
>> >>
>> >> Currently kernel image is not fully compliant PE image, so it may
>> >> fail to boot with stricter implementations of UEFI PE loaders.
>> >>
>> >> Set minimal alignments and sizes specified by PE documentation [1]
>> >> referenced by UEFI specification [2]. Align PE header to 8 bytes.
>> >>
>> >> Generate PE sections dynamically. This simplifies code, since with
>> >> current implementation all of the sections needs to be defined in
>> >> header.S, where most section header fields do not hold valid values,
>> >> except for their names. Before the change, it also held flags,
>> >> but now flags depend on kernel configuration and it is simpler
>> >> to set them from build.c too.
>> >>
>> >> Setup sections protection. Since we cannot fit every needed section,
>> >> set a part of protection flags dynamically during initialization.
>> >> This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set.
>> >>
>> >> [1]
>> >> https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx
>> >> [2]
>> >> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf
>> >>
>> >> Tested-by: Peter Jones <pjones@...hat.com>
>> >> Signed-off-by: Evgeniy Baskov <baskov@...ras.ru>
>> >
>> > I would prefer it if we didn't rewrite the build tool this way.
>> >
>> > Having the sections in header.S in the order they appear in the binary
>> > is rather useful, and I don't think we should manipulate the section
>> > flags based on whether CONFIG_DXE_MEM_ATTRIBUTES is set. I also don't
>> > think we need more than .text / .,data (as discussed in the other
>> > thread on linux-efi@)
>> >
>> > Furthermore, I had a look at the audk PE loader [0], and I think it is
>> > being overly pedantic.
>> >
>> > The PE/COFF spec does not require that all sections are virtually
>> > contiguous, and it does not require that the file content is
>> > completely covered by either the header or by a section.
>> >
>> > So what I would prefer to do is the following:
>> >
>> > Sections:
>> > Idx Name          Size     VMA              Type
>> >   0 .reloc        00000200 0000000000002000 DATA
>> >   1 .compat       00000200 0000000000003000 DATA
>> >   2 .text         00bee000 0000000000004000 TEXT
>> >   3 .data         00002200 0000000000bf2000 DATA
>> >
>> > using 4k section alignment and 512 byte file alignment, and a header
>> > size of 0x200 as before (This requires my patch that allows the setup
>> > header to remain unmapped when running the stub [1])
>> >
>> > The reloc and compat payloads are placed at the end of the setup data
>> > as before, but increased in size to 512 bytes each, and then mapped
>> > non-1:1 into the RVA space.
>> >
>> > This works happily with both the existing PE loader as well as the
>> > audk one, but with the pedantic flags disabled.
>> >
>> 
>> This makes sense. I'll change this patch to use this layout and
>> to keep sections in headers.S before sending v5. (and I guess I'll
>> make the compressed kernel a part of .text). I have a few questions
>> though:
>> 
>> This layout assumes having the local copy of the bootparams as
>> in your RFC patches, right?
>> 
> 
> Indeed. Otherwise, the setup header may not have been copied to memory
> by the loader.
> 
>> Can I keep the .rodata -- 5th section fits in the section table
>> without much work?
>> 
> 
> You could, but at least the current PE/COFF loader in EDK2 will map it
> read/write, as it only distinguishes between executable sections and
> non-executable sections.
> 

At least it will slightly improve security for some implementations
(e.g. audk, while being overly strict support RO sections)

>> Also, why .reloc is at offset 0x2000 and not just 0x1000, is there
>> anything important I am missing? I understand that is cannot be 0
>> and should be aligned on page size, but nothing else comes to my
>> mind...
>> 
> 
> That was just arbitrary, because the raw allocations of reloc and
> compat are also allocated towards the end. But I guess starting at
> 0x1000 for .reloc makes more sense so feel free to change that.

Thanks for clarifications!