2020-03-01 23:07:03

by Arvind Sankar

[permalink] [raw]
Subject: [PATCH 4/5] efi/x86: Remove extra headroom for setup block

commit 223e3ee56f77 ("efi/x86: add headroom to decompressor BSS to
account for setup block") added headroom to the PE image to account for
the setup block, which wasn't used for the decompression buffer.

Now that we decompress from the start of the image, this is no longer
required.

Add a check to make sure that the head section of the compressed kernel
won't overwrite itself while relocating. This is only for
future-proofing as with current limits on the setup and the actual size
of the head section, this can never happen.

Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/boot/tools/build.c | 28 ++++++++++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 90d403dfec80..3d03ad753ed5 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -65,6 +65,8 @@ unsigned long efi_pe_entry;
unsigned long efi32_pe_entry;
unsigned long kernel_info;
unsigned long startup_64;
+unsigned long _ehead;
+unsigned long _end;

/*----------------------------------------------------------------------*/

@@ -232,7 +234,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
{
unsigned int pe_header;
unsigned int text_sz = file_sz - text_start;
- unsigned int bss_sz = init_sz + text_start - file_sz;
+ unsigned int bss_sz = init_sz - file_sz;

pe_header = get_unaligned_le32(&buf[0x3c]);

@@ -259,7 +261,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
put_unaligned_le32(file_sz - 512 + bss_sz, &buf[pe_header + 0x1c]);

/* Size of image */
- put_unaligned_le32(init_sz + text_start, &buf[pe_header + 0x50]);
+ put_unaligned_le32(init_sz, &buf[pe_header + 0x50]);

/*
* Address of entry point for PE/COFF executable
@@ -360,6 +362,8 @@ static void parse_zoffset(char *fname)
PARSE_ZOFS(p, efi32_pe_entry);
PARSE_ZOFS(p, kernel_info);
PARSE_ZOFS(p, startup_64);
+ PARSE_ZOFS(p, _ehead);
+ PARSE_ZOFS(p, _end);

p = strchr(p, '\n');
while (p && (*p == '\r' || *p == '\n'))
@@ -444,6 +448,26 @@ int main(int argc, char ** argv)
put_unaligned_le32(sys_size, &buf[0x1f4]);

init_sz = get_unaligned_le32(&buf[0x260]);
+#ifdef CONFIG_EFI_STUB
+ /*
+ * The decompression buffer will start at ImageBase. When relocating
+ * the compressed kernel to its end, we must ensure that the head
+ * section does not get overwritten. The head section occupies
+ * [i, i + _ehead), and the destination is [init_sz - _end, init_sz).
+ *
+ * At present these should never overlap, because i is at most 32k
+ * because of SETUP_SECT_MAX, _ehead is less than 1k, and the
+ * calculation of INIT_SIZE in boot/header.S ensures that
+ * init_sz - _end is at least 64k.
+ *
+ * For future-proofing, increase init_sz if necessary.
+ */
+
+ if (init_sz - _end < i + _ehead) {
+ init_sz = (i + _ehead + _end + 4095) & ~4095;
+ put_unaligned_le32(init_sz, &buf[0x260]);
+ }
+#endif
update_pecoff_text(setup_sectors * 512, i + (sys_size * 16), init_sz);

efi_stub_entry_update();
--
2.24.1


2020-03-02 04:22:02

by Mika Penttilä

[permalink] [raw]
Subject: Re: [PATCH 4/5] efi/x86: Remove extra headroom for setup block



On 2.3.2020 1.05, Arvind Sankar wrote:
> commit 223e3ee56f77 ("efi/x86: add headroom to decompressor BSS to
> account for setup block") added headroom to the PE image to account for
> the setup block, which wasn't used for the decompression buffer.
>
> Now that we decompress from the start of the image, this is no longer
> required.
>
> Add a check to make sure that the head section of the compressed kernel
> won't overwrite itself while relocating. This is only for
> future-proofing as with current limits on the setup and the actual size
> of the head section, this can never happen.
>
> Signed-off-by: Arvind Sankar <[email protected]>

To make clear, the kernel (head_32.s and head_64.s) still relocates
itself to the end of the buffer and does in-place decompression. So this
is just to make init sz smaller.


> ---
> arch/x86/boot/tools/build.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index 90d403dfec80..3d03ad753ed5 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -65,6 +65,8 @@ unsigned long efi_pe_entry;
> unsigned long efi32_pe_entry;
> unsigned long kernel_info;
> unsigned long startup_64;
> +unsigned long _ehead;
> +unsigned long _end;
>
> /*----------------------------------------------------------------------*/
>
> @@ -232,7 +234,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
> {
> unsigned int pe_header;
> unsigned int text_sz = file_sz - text_start;
> - unsigned int bss_sz = init_sz + text_start - file_sz;
> + unsigned int bss_sz = init_sz - file_sz;
>
> pe_header = get_unaligned_le32(&buf[0x3c]);
>
> @@ -259,7 +261,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
> put_unaligned_le32(file_sz - 512 + bss_sz, &buf[pe_header + 0x1c]);
>
> /* Size of image */
> - put_unaligned_le32(init_sz + text_start, &buf[pe_header + 0x50]);
> + put_unaligned_le32(init_sz, &buf[pe_header + 0x50]);
>
> /*
> * Address of entry point for PE/COFF executable
> @@ -360,6 +362,8 @@ static void parse_zoffset(char *fname)
> PARSE_ZOFS(p, efi32_pe_entry);
> PARSE_ZOFS(p, kernel_info);
> PARSE_ZOFS(p, startup_64);
> + PARSE_ZOFS(p, _ehead);
> + PARSE_ZOFS(p, _end);
>
> p = strchr(p, '\n');
> while (p && (*p == '\r' || *p == '\n'))
> @@ -444,6 +448,26 @@ int main(int argc, char ** argv)
> put_unaligned_le32(sys_size, &buf[0x1f4]);
>
> init_sz = get_unaligned_le32(&buf[0x260]);
> +#ifdef CONFIG_EFI_STUB
> + /*
> + * The decompression buffer will start at ImageBase. When relocating
> + * the compressed kernel to its end, we must ensure that the head
> + * section does not get overwritten. The head section occupies
> + * [i, i + _ehead), and the destination is [init_sz - _end, init_sz).
> + *
> + * At present these should never overlap, because i is at most 32k
> + * because of SETUP_SECT_MAX, _ehead is less than 1k, and the
> + * calculation of INIT_SIZE in boot/header.S ensures that
> + * init_sz - _end is at least 64k.
> + *
> + * For future-proofing, increase init_sz if necessary.
> + */
> +
> + if (init_sz - _end < i + _ehead) {
> + init_sz = (i + _ehead + _end + 4095) & ~4095;
> + put_unaligned_le32(init_sz, &buf[0x260]);
> + }
> +#endif
> update_pecoff_text(setup_sectors * 512, i + (sys_size * 16), init_sz);
>
> efi_stub_entry_update();


Attachments:
pEpkey.asc (3.08 kB)
pEpkey.asc

2020-03-03 04:15:06

by Arvind Sankar

[permalink] [raw]
Subject: Re: [PATCH 4/5] efi/x86: Remove extra headroom for setup block

On Mon, Mar 02, 2020 at 04:21:30AM +0000, Mika Penttilä wrote:
>
>
> On 2.3.2020 1.05, Arvind Sankar wrote:
> > commit 223e3ee56f77 ("efi/x86: add headroom to decompressor BSS to
> > account for setup block") added headroom to the PE image to account for
> > the setup block, which wasn't used for the decompression buffer.
> >
> > Now that we decompress from the start of the image, this is no longer
> > required.
> >
> > Add a check to make sure that the head section of the compressed kernel
> > won't overwrite itself while relocating. This is only for
> > future-proofing as with current limits on the setup and the actual size
> > of the head section, this can never happen.
> >
> > Signed-off-by: Arvind Sankar <[email protected]>
>
> To make clear, the kernel (head_32.s and head_64.s) still relocates
> itself to the end of the buffer and does in-place decompression. So this
> is just to make init sz smaller.
>
>

Not init_size itself, but it reduces the size allocated for the PE
image. Do you want me to update the comment to make that clearer?