Re: [PATCH v2 1/5] efi/x86: Drop redundant .bss section

[Ard Biesheuvel <ardb@kernel.org>]

On Fri, 21 Feb 2020 at 17:40, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Feb 17, 2020 at 03:48:18PM +0100, Ard Biesheuvel wrote:
> > In commit
> >
> >   c7fb93ec51d462ec ("x86/efi: Include a .bss section within the PE/COFF headers")
> >
> > we added a separate .bss section to the PE/COFF header of the compressed
> > kernel describing the static memory footprint of the decompressor, to
> > ensure that it has enough headroom to decompress itself.
> >
> > We can achieve the exact same result by increasing the virtual size of
> > the .text section, without changing the raw size, which, as per the
> > PE/COFF specification, requires the loader to zero initialize the delta.
> >
> > Doing so frees up a slot in the section table, which we will use later
> > to describe the mixed mode entrypoint.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/boot/header.S      | 21 +-----------
> >  arch/x86/boot/tools/build.c | 35 ++++++++------------
> >  2 files changed, 14 insertions(+), 42 deletions(-)
> >
> > diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> > index 55e669d29e54..0c8c5a52f1f0 100644
> > --- a/arch/x86/boot/tools/build.c
> > +++ b/arch/x86/boot/tools/build.c
> > @@ -203,10 +203,12 @@ static void update_pecoff_setup_and_reloc(unsigned int size)
> >       put_unaligned_le32(10, &buf[reloc_offset + 4]);
> >  }
> >
> > -static void update_pecoff_text(unsigned int text_start, unsigned int file_sz)
> > +static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
> > +                            unsigned int init_sz)
> >  {
> >       unsigned int pe_header;
> >       unsigned int text_sz = file_sz - text_start;
> > +     unsigned int bss_sz = init_sz - file_sz;
> >
> >       pe_header = get_unaligned_le32(&buf[0x3c]);
> >
> > @@ -216,28 +218,19 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz)
> >        */
> >       put_unaligned_le32(file_sz - 512, &buf[pe_header + 0x1c]);
> >
> > -     /*
> > -      * Address of entry point for PE/COFF executable
> > -      */
> > -     put_unaligned_le32(text_start + efi_pe_entry, &buf[pe_header + 0x28]);
> > -
> > -     update_pecoff_section_header(".text", text_start, text_sz);
> > -}
> > -
> > -static void update_pecoff_bss(unsigned int file_sz, unsigned int init_sz)
> > -{
> > -     unsigned int pe_header;
> > -     unsigned int bss_sz = init_sz - file_sz;
> > -
> > -     pe_header = get_unaligned_le32(&buf[0x3c]);
> > -
> >       /* Size of uninitialized data */
> >       put_unaligned_le32(bss_sz, &buf[pe_header + 0x24]);
>
> Should this still be populated given that there's no .bss section any
> more?

Good point. The PE/COFF spec is explicit, for a change, and
specifically mentions that this should be the combined sizeof all BSS
sections. It doesn't really specify how one could have multiple BSS
sections, but the wording does support your view that this should be
zero, and the value of bss_sz added to the SizeOfText field.

I couldn't find any code in EDK2 that actually references this field
(apart from the ELF to PE/COFF converter that always sets it to 0x0),
and so I don't think it really matters.