Re: [PATCH v6 04/10] modules: load module sections at page-aligned addresses

[Daniel Kiper]

On Thu, Sep 19, 2024 at 05:31:58PM +0100, Mate Kukri wrote:
> Currently we load module sections at whatever alignment gcc+ld happened
> to dump into the ELF section header, which is often less then the page
> size. Since NX protections are page based, this alignment must be
> rounded up to page size on platforms supporting NX protections.
>
> This patch switches most EFI platforms to load module sections at 4kB
> page-aligned addresses.  To do so, it adds an new per-arch function,
> grub_arch_dl_min_alignment(), which returns the alignment needed for
> dynamically loaded sections (in bytes).  Currently it sets it to 4096
> when GRUB_MACHINE_EFI is true on x86_64, i386, arm, arm64, and emu, and
> 1-byte alignment on everything else.
>
> It then changes the allocation size computation and the loader code in
> grub_dl_load_segments() to align the locations and sizes up to these
> boundaries, and fills any added padding with zeros.
>
> All of this happens before relocations are applied, so the relocations
> factor that in with no change.

The commit message is stale. Please update it to reflect what current
patch does.

> Signed-off-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
> Signed-off-by: Mate Kukri <mate.kukri@canonical.com>
> ---
>  grub-core/kern/dl.c | 52 +++++++++++++++++++++++++++++++--------------
>  include/grub/dl.h   |  9 ++++++++
>  2 files changed, 45 insertions(+), 16 deletions(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 37db9fab0..47705f5af 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -224,25 +224,35 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>  {
>    unsigned i;
>    const Elf_Shdr *s;
> -  grub_size_t tsize = 0, talign = 1;
> +  grub_size_t tsize = 0, talign = 1, arch_addralign = 1;
>  #if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv) && \
>    !defined (__loongarch__)
>    grub_size_t tramp;
> +  grub_size_t tramp_align;
>    grub_size_t got;
> +  grub_size_t got_align;
>    grub_err_t err;
>  #endif
>    char *ptr;
>
> +  arch_addralign = GRUB_DL_ALIGN;
> +
>    for (i = 0, s = (const Elf_Shdr *)((const char *) e + e->e_shoff);
>         i < e->e_shnum;
>         i++, s = (const Elf_Shdr *)((const char *) s + e->e_shentsize))
>      {
> +      grub_size_t sh_addralign;
> +      grub_size_t sh_size;
> +
>        if (s->sh_size == 0 || !(s->sh_flags & SHF_ALLOC))
>       continue;
>
> -      tsize = ALIGN_UP (tsize, s->sh_addralign) + s->sh_size;
> -      if (talign < s->sh_addralign)
> -     talign = s->sh_addralign;
> +      sh_addralign = ALIGN_UP(s->sh_addralign, arch_addralign);

Coding style, missing space before "("...

> +      sh_size = ALIGN_UP(s->sh_size, sh_addralign);

Ditto and below please...

> +
> +      tsize = ALIGN_UP (tsize, sh_addralign) + sh_size;
> +      if (talign < sh_addralign)
> +     talign = sh_addralign;
>      }
>
>  #if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv) && \
> @@ -250,12 +260,18 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>    err = grub_arch_dl_get_tramp_got_size (e, &tramp, &got);
>    if (err)
>      return err;
> -  tsize += ALIGN_UP (tramp, GRUB_ARCH_DL_TRAMP_ALIGN);
> -  if (talign < GRUB_ARCH_DL_TRAMP_ALIGN)
> -    talign = GRUB_ARCH_DL_TRAMP_ALIGN;
> -  tsize += ALIGN_UP (got, GRUB_ARCH_DL_GOT_ALIGN);
> -  if (talign < GRUB_ARCH_DL_GOT_ALIGN)
> -    talign = GRUB_ARCH_DL_GOT_ALIGN;
> +  tramp_align = GRUB_ARCH_DL_TRAMP_ALIGN;
> +  if (tramp_align < arch_addralign)
> +    tramp_align = arch_addralign;

tramp_align = grub_max (tramp_align, arch_addralign) ?

> +  tsize += ALIGN_UP (tramp, tramp_align);
> +  if (talign < tramp_align)
> +    talign = tramp_align;

talign = grub_max (talign, tramp_align) ?

... and probably below...

> +  got_align = GRUB_ARCH_DL_GOT_ALIGN;
> +  if (got_align < arch_addralign)
> +    got_align = arch_addralign;
> +  tsize += ALIGN_UP (got, got_align);
> +  if (talign < got_align)
> +    talign = got_align;
>  #endif
>
>  #ifdef GRUB_MACHINE_EMU
> @@ -272,6 +288,9 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>         i < e->e_shnum;
>         i++, s = (Elf_Shdr *)((char *) s + e->e_shentsize))
>      {
> +      grub_size_t sh_addralign = ALIGN_UP(s->sh_addralign, arch_addralign);
> +      grub_size_t sh_size = ALIGN_UP(s->sh_size, sh_addralign);
> +
>        if (s->sh_flags & SHF_ALLOC)
>       {
>         grub_dl_segment_t seg;
> @@ -284,17 +303,18 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>           {
>             void *addr;
>
> -           ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, s->sh_addralign);
> +           ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, sh_addralign);
>             addr = ptr;
> -           ptr += s->sh_size;
> +           ptr += sh_size;
>
>             switch (s->sh_type)
>               {
>               case SHT_PROGBITS:
>                 grub_memcpy (addr, (char *) e + s->sh_offset, s->sh_size);
> +               grub_memset ((char *)addr + s->sh_size, 0, sh_size - 
> s->sh_size);

Missing space after ")"...

Daniel