On Thu, Sep 19, 2024 at 05:32:00PM +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
This part of commit message does not (exactly) reflect what the code
does. Especially in the SHF_EXECINSTR case. Additionally, I would
suggest to split this long sentence to a few shorter ones. Or use
bullets to make things clear...
> cases it sets both flags.
>
> Signed-off-by: Peter Jones <pjo...@redhat.com>
> Signed-off-by: Robbie Harwood <rharw...@redhat.com>
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Jan Setje-Eilers <jan.setjeeil...@oracle.com>
> Signed-off-by: Mate Kukri <mate.ku...@canonical.com>
> ---
> grub-core/kern/dl.c | 85 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 47705f5af..54daa7f30 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -634,6 +634,84 @@ grub_dl_relocate_symbols (grub_dl_t mod, void *ehdr)
> 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;
> + const Elf_Shdr *s;
> + const Elf_Ehdr *e = ehdr;
> + grub_err_t err;
> +#if !defined (__i386__) && !defined (__x86_64__) && !defined (__riscv)
What about ARM and loongarch64 architectures? Anything else?
> + grub_size_t arch_addralign = GRUB_DL_ALIGN;
> + 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;
> +
> + for (seg = mod->segment; seg; seg = seg->next)
> + if (seg->section == s->sh_info)
> + break;
> +
> + 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)
What about other archs? I can see this second time in this short code.
This is error prone. Would not it be easier to introduce a constant,
e.g., ARCH_NX (probably not good example)? And first of all why this
code is arch specific? I think it requires at least a comment...
> + 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;
> +
> + 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)
> + 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);
> + 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)
> @@ -680,7 +758,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))
Again, if you define an empty grub_dl_set_mem_attrs() counterpart for
non-EFI case this part of code will be simplified.
> +#else
> + )
> +#endif
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
