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

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:07PM +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.
>
> Original-Author: Peter Jones <pjones@redhat.com>
> Original-Author: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Mate Kukri <mate.kukri@canonical.com>
> ---
>  docs/grub-dev.texi          |  6 ++---
>  grub-core/kern/arm/dl.c     | 13 +++++++++
>  grub-core/kern/arm64/dl.c   | 13 +++++++++
>  grub-core/kern/dl.c         | 53 ++++++++++++++++++++++++++-----------
>  grub-core/kern/emu/full.c   | 13 +++++++++
>  grub-core/kern/i386/dl.c    | 13 +++++++++
>  grub-core/kern/ia64/dl.c    |  9 +++++++
>  grub-core/kern/mips/dl.c    |  8 ++++++
>  grub-core/kern/powerpc/dl.c |  9 +++++++
>  grub-core/kern/riscv/dl.c   | 13 +++++++++
>  grub-core/kern/sparc64/dl.c |  9 +++++++
>  grub-core/kern/x86_64/dl.c  | 13 +++++++++
>  include/grub/dl.h           |  2 ++
>  13 files changed, 155 insertions(+), 19 deletions(-)
>
> diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
> index 1276c5930..2f782cda5 100644
> --- a/docs/grub-dev.texi
> +++ b/docs/grub-dev.texi
> @@ -996,9 +996,9 @@ declare startup asm file ($cpu_$platform_startup) as well 
> as any other files
>  (e.g. init.c and callwrap.S) (e.g. $cpu_$platform = 
> kern/$cpu/$platform/init.c).
>  At this stage you will also need to add dummy dl.c and cache.S with functions
>  grub_err_t grub_arch_dl_check_header (void *ehdr), grub_err_t
> -grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr) (dl.c) and
> -void grub_arch_sync_caches (void *address, grub_size_t len) (cache.S). They
> -won't be used for now.
> +grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr) (dl.c), 
> grub_uint32_t
> +grub_arch_dl_min_alignment (void), and void grub_arch_sync_caches (void
> +*address, grub_size_t len) (cache.S). They won't be used for now.
>
>  You will need to create directory include/$cpu/$platform and a file
>  include/$cpu/types.h. The latter following this template:
> diff --git a/grub-core/kern/arm/dl.c b/grub-core/kern/arm/dl.c
> index eab9d17ff..926073793 100644
> --- a/grub-core/kern/arm/dl.c
> +++ b/grub-core/kern/arm/dl.c
> @@ -278,3 +278,16 @@ grub_arch_dl_check_header (void *ehdr)
>
>    return GRUB_ERR_NONE;
>  }
> +
> +/*
> + * Tell the loader what our minimum section alignment is.
> + */
> +grub_size_t
> +grub_arch_dl_min_alignment (void)
> +{
> +#ifdef GRUB_MACHINE_EFI
> +  return 4096;
> +#else
> +  return 1;
> +#endif
> +}

I would define this as a constant in the include/grub/efi/memory.h.
OK, we have to have definition for non-EFI case somewhere as well.

> diff --git a/grub-core/kern/arm64/dl.c b/grub-core/kern/arm64/dl.c
> index a2b5789a9..95c6d5bf4 100644
> --- a/grub-core/kern/arm64/dl.c
> +++ b/grub-core/kern/arm64/dl.c
> @@ -196,3 +196,16 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
>
>    return GRUB_ERR_NONE;
>  }
> +
> +/*
> + * Tell the loader what our minimum section alignment is.
> + */
> +grub_size_t
> +grub_arch_dl_min_alignment (void)
> +{
> +#ifdef GRUB_MACHINE_EFI
> +  return 4096;
> +#else
> +  return 1;
> +#endif
> +}

Ditto and below as well please...

> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 37db9fab0..8338f7436 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_arch_dl_min_alignment ();
> +
>    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);
> +      sh_size = ALIGN_UP(s->sh_size, sh_addralign);
> +
> +      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;
> +  tsize += ALIGN_UP (tramp, tramp_align);
> +  if (talign < tramp_align)
> +    talign = tramp_align;
> +  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,19 @@ 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);

You do not need wrap line here. I am OK with lines (a bit) longer than 80 chars.

>                 break;
>               case SHT_NOBITS:
> -               grub_memset (addr, 0, s->sh_size);
> +               grub_memset (addr, 0, sh_size);
>                 break;
>               }
>

Daniel