Re: [PATCH v4 06/10] nx: set page permissions for loaded modules.

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:09PM +0100, Mate Kukri wrote:
> For NX, we need to set write and executable permissions on the sections
> of grub modules when we load them.
>
> On sections with SHF_ALLOC set, which is typically everything except
> .modname and the symbol and string tables, this patch clears the Read
> Only flag on sections that have the ELF flag SHF_WRITE set, and clears
> the No eXecute flag on sections with SHF_EXECINSTR set.  In all other
> cases it sets both flags.
>
> Original-Author: Peter Jones <pjones@redhat.com>
> Original-Author: Robbie Harwood <rharwood@redhat.com>
> Original-Author: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Mate Kukri <mate.kukri@canonical.com>
> ---
>  grub-core/kern/dl.c | 104 ++++++++++++++++++++++++++++++++++++++------
>  include/grub/dl.h   |  46 ++++++++++++++++++++
>  2 files changed, 137 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 8338f7436..3341d78d6 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -616,25 +616,97 @@ grub_dl_relocate_symbols (grub_dl_t mod, void *ehdr)
>       grub_dl_segment_t seg;
>       grub_err_t err;
>
> -     /* Find the target segment.  */
> -     for (seg = mod->segment; seg; seg = seg->next)
> -       if (seg->section == s->sh_info)
> -         break;
> +     seg = grub_dl_find_segment(mod, s->sh_info);
> +        if (!seg)
> +       continue;

This hunk is suspicious...

> -     if (seg)
> -       {
> -         if (!mod->symtab)
> -           return grub_error (GRUB_ERR_BAD_MODULE, "relocation without 
> symbol table");
> +     if (!mod->symtab)
> +       return grub_error (GRUB_ERR_BAD_MODULE, "relocation without symbol 
> table");
>
> -         err = grub_arch_dl_relocate_symbols (mod, ehdr, s, seg);
> -         if (err)
> -           return err;
> -       }
> +     err = grub_arch_dl_relocate_symbols (mod, ehdr, s, seg);
> +     if (err)
> +       return err;
>        }
>
>    return GRUB_ERR_NONE;
>  }
>
> +/* Only define this on EFI to save space in core */
> +#ifdef GRUB_MACHINE_EFI
> +static grub_err_t
> +grub_dl_set_mem_attrs (grub_dl_t mod, void *ehdr)
> +{
> +  unsigned i;

s/unsigned/unsigned int/

> +  const Elf_Shdr *s;
> +  const Elf_Ehdr *e = ehdr;
> +  grub_err_t err;
> +#if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv)
> +  grub_size_t arch_addralign = grub_arch_dl_min_alignment ();
> +  grub_addr_t tgaddr;
> +  grub_size_t tgsz;
> +#endif
> +
> +  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_dl_segment_t seg;
> +      grub_uint64_t set_attrs = GRUB_MEM_ATTR_R;
> +      grub_uint64_t clear_attrs = GRUB_MEM_ATTR_W|GRUB_MEM_ATTR_X;
> +
> +      seg = grub_dl_find_segment(mod, i);
> +      if (!seg)
> +     continue;
> +
> +      if (seg->size == 0 || !(s->sh_flags & SHF_ALLOC))
> +     continue;
> +
> +      if (s->sh_flags & SHF_WRITE)
> +     {
> +       set_attrs |= GRUB_MEM_ATTR_W;
> +       clear_attrs &= ~GRUB_MEM_ATTR_W;
> +     }
> +
> +      if (s->sh_flags & SHF_EXECINSTR)
> +     {
> +       set_attrs |= GRUB_MEM_ATTR_X;
> +       clear_attrs &= ~GRUB_MEM_ATTR_X;
> +     }
> +
> +      err = grub_update_mem_attrs ((grub_addr_t)(seg->addr), seg->size,
> +                                set_attrs, clear_attrs);
> +      if (err != GRUB_ERR_NONE)
> +     return err;
> +    }
> +
> +#if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv)
> +  tgaddr = grub_min((grub_addr_t)mod->tramp, (grub_addr_t)mod->got);
> +  tgsz = grub_max((grub_addr_t)mod->trampptr, (grub_addr_t)mod->gotptr) - 
> tgaddr;

Wrong coding style for function calls and casts.

> +  if (tgsz)
> +    {
> +      tgsz = ALIGN_UP(tgsz, arch_addralign);
> +
> +      if (tgaddr < (grub_addr_t)mod->base ||
> +          tgsz > (grub_addr_t)-1 - tgaddr ||
> +       tgaddr + tgsz > (grub_addr_t)mod->base + mod->sz)

Wrong casts coding style...

> +     return grub_error (GRUB_ERR_BUG,
> +                        "BUG: trying to protect pages outside of module "
> +                        "allocation (\"%s\"): module base %p, size 0x%"
> +                        PRIxGRUB_SIZE "; tramp/GOT base 0x%" PRIxGRUB_ADDR
> +                        ", size 0x%" PRIxGRUB_SIZE,
> +                        mod->name, mod->base, mod->sz, tgaddr, tgsz);
> +      err = grub_update_mem_attrs (tgaddr, tgsz, 
> GRUB_MEM_ATTR_R|GRUB_MEM_ATTR_X,
> +                                GRUB_MEM_ATTR_W);

Missing space before and after "|" (I saw similar mistakes in the other
places too). And you do not need to wrap this line.

> +      if (err != GRUB_ERR_NONE)
> +     return err;
> +    }
> +#endif
> +
> +  return GRUB_ERR_NONE;
> +}
> +#endif
> +
>  /* Load a module from core memory.  */
>  grub_dl_t
>  grub_dl_load_core_noinit (void *addr, grub_size_t size)
> @@ -668,6 +740,7 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
>    mod->ref_count = 1;
>
>    grub_dprintf ("modules", "relocating to %p\n", mod);
> +
>    /* Me, Vladimir Serbinenko, hereby I add this module check as per new
>       GNU module policy. Note that this license check is informative only.
>       Modules have to be licensed under GPLv3 or GPLv3+ (optionally
> @@ -681,7 +754,12 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
>        || grub_dl_resolve_dependencies (mod, e)
>        || grub_dl_load_segments (mod, e)
>        || grub_dl_resolve_symbols (mod, e)
> -      || grub_dl_relocate_symbols (mod, e))
> +      || grub_dl_relocate_symbols (mod, e)
> +#ifdef GRUB_MACHINE_EFI
> +      || grub_dl_set_mem_attrs (mod, e))
> +#else
> +     )
> +#endif
>      {
>        mod->fini = 0;
>        grub_dl_unload (mod);
> diff --git a/include/grub/dl.h b/include/grub/dl.h
> index 8a37cc16f..84a249834 100644
> --- a/include/grub/dl.h
> +++ b/include/grub/dl.h
> @@ -27,6 +27,7 @@
>  #include <grub/elf.h>
>  #include <grub/list.h>
>  #include <grub/misc.h>
> +#include <grub/mm.h>
>  #endif
>
>  /*
> @@ -254,6 +255,51 @@ grub_dl_is_persistent (grub_dl_t mod)
>    return mod->persistent;
>  }
>
> +#pragma GCC diagnostic ignored "-Wcast-align"

Why? Please add a comment before pragma. And this suggests you should
probably use grub_unaligned_*() instead.

> +static inline const char *
> +grub_dl_get_section_name (const Elf_Ehdr *e, const Elf_Shdr *s)
> +{
> +  Elf_Shdr *str_s;
> +  const char *str;
> +
> +  str_s = (Elf_Shdr *) ((char *) e + e->e_shoff + e->e_shstrndx * 
> e->e_shentsize);
> +  str = (char *) e + str_s->sh_offset;
> +
> +  return str + s->sh_name;
> +}
> +
> +static inline long
> +grub_dl_find_section_index (Elf_Ehdr *e, const char *name)
> +{
> +  Elf_Shdr *s;
> +  const char *str;
> +  unsigned i;

s/unsigned/long/ and then you do not need cast below...

> +
> +  s = (Elf_Shdr *) ((char *) e + e->e_shoff + e->e_shstrndx * 
> e->e_shentsize);
> +  str = (char *) e + s->sh_offset;
> +
> +  for (i = 0, s = (Elf_Shdr *) ((char *) e + e->e_shoff);
> +       i < e->e_shnum;
> +       i++, s = (Elf_Shdr *) ((char *) s + e->e_shentsize))
> +    if (grub_strcmp (str + s->sh_name, name) == 0)
> +      return (long)i;

s/(long)i/(long) i/

> +  return -1;
> +}
> +
> +/* Return the segment for a section of index N */
> +static inline grub_dl_segment_t
> +grub_dl_find_segment (grub_dl_t mod, unsigned n)
> +{
> +  grub_dl_segment_t seg;
> +
> +  for (seg = mod->segment; seg; seg = seg->next)
> +    if (seg->section == n)
> +      return seg;
> +
> +  return NULL;
> +}

I think you should restore "-Wcast-align" here. If it is really needed...

Daniel