On 2023-03-12 16:09, Ard Biesheuvel wrote:
On Sun, 12 Mar 2023 at 13:02, Evgeniy Baskov <baskov@xxxxxxxxx> wrote:
On 2023-03-11 20:31, Ard Biesheuvel wrote:
> On Sat, 11 Mar 2023 at 16:02, Evgeniy Baskov <baskov@xxxxxxxxx> wrote:
>>
>> On 2023-03-10 18:17, Ard Biesheuvel wrote:
>> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@xxxxxxxxx> 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@xxxxxxxxxx>
>> >> Signed-off-by: Evgeniy Baskov <baskov@xxxxxxxxx>
>> >
>> > 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)
Yeah, but more common loaders will put the compressed data in a
writable region. I'd prefer to have a simple and common baseline where
we always just use R-X for all text and rodata, and RW- for everything
else.
Hmm... I'll remove the .rodata for now then. If anything changes I can
always submit it as a separate patch later anyways.
