Do you mean that this crash is introduced and not fixed in this patch
series? Can't we fix it by bumping total allocation size to multiple
of page size?
On Fri, May 24, 2024 at 2:06 PM Mate Kukri <mate.ku...@canonical.com> wrote:
>
> From: Laszlo Ersek <ler...@redhat.com>
>
> On aarch64 UEFI, we currently have a crasher:
>
> grub_dl_load_core()
> grub_dl_load_core_noinit()
>
> /* independent allocation: must remain writable */
> mod = grub_zalloc();
>
> /* allocates module image with incorrect tail alignment */
> grub_dl_load_segments()
>
> /* write-protecting the module image makes "mod" read-only! */
> grub_dl_set_mem_attrs()
> grub_update_mem_attrs()
>
> grub_dl_init()
> /* page fault, crash */
> mod->next = ...;
>
> - Commit 887f1d8fa976 ("modules: load module sections at page-aligned
> addresses", 2023-02-08) forgot to page-align the allocation of the
> trampolines and GOT areas of grub2 modules, in grub_dl_load_segments().
>
> - Commit ad1b904d325b ("nx: set page permissions for loaded modules.",
> 2023-02-08) calculated a common bounding box for the trampolines and GOT
> areas in grub_dl_set_mem_attrs(), rounded the box size up to a whole
> multiple of EFI page size ("arch_addralign"), and write-protected the
> resultant page range.
>
> Consequently, grub_dl_load_segments() places the module image in memory
> such that its tail -- the end of the trampolines and GOT areas -- lands at
> the head of a page whose tail in turn contains independent memory
> allocations, such as "mod". grub_dl_set_mem_attrs() will then unwittingly
> write-protect these other allocations too.
>
> But "mod" must remain writable: we assign "mod->next" in grub_dl_init()
> subsequently. Currently we crash there with a page fault / permission
> fault.
>
> (The crash is not trivial to hit: the tramp/GOT areas are irrelevant on
> x86_64, plus the page protection depends on the UEFI platform firmware
> providing EFI_MEMORY_ATTRIBUTE_PROTOCOL. In practice, the crash is
> restricted to aarch64 edk2 (ArmVirtQemu) builds containing commit
> 1c4dfadb4611, "ArmPkg/CpuDxe: Implement EFI memory attributes protocol",
> 2023-03-16.)
>
> Example log before the patch:
>
> > kern/dl.c:736: updating attributes for GOT and trampolines ("video_fb")
> > kern/efi/mm.c:927: set +rx -w on 0x13b88b000-0x13b88bfff before:rwx
> > after:r-x
> > kern/dl.c:744: done updating module memory attributes for "video_fb"
> > kern/dl.c:639: flushing 0xe4f0 bytes at 0x13b87d000
> > kern/arm64/cache.c:42: D$ line size: 64
> > kern/arm64/cache.c:43: I$ line size: 64
> > kern/dl.c:839: module name: video_fb
> > kern/dl.c:840: init function: 0x0
> > kern/dl.c:865: Initing module video_fb
> >
> > Synchronous Exception at 0x000000013B8A76EC
> > PC 0x00013B8A76EC
> >
> > X0 0x000000013B88B960 X1 0x0000000000000000 X2 0x000000013F93587C
> > X3 0x0000000000000075
> >
> > SP 0x00000000470745C0 ELR 0x000000013B8A76EC SPSR 0x60000205 FPSR
> > 0x00000000
> > ESR 0x9600004F FAR 0x000000013B88B9D0
> >
> > ESR : EC 0x25 IL 0x1 ISS 0x0000004F
> >
> > Data abort: Permission fault, third level
>
> Note the following:
>
> - The whole 4K page at 0x1_3B88_B000 is write-protected.
>
> - The "video_fb" module actually lives at [0x1_3B87_D000, 0x1_3B88_B4F0)
> -- left-inclusive, right-exclusive --; that is, in the last page (at
> 0x1_3B88_B000), it only occupies the first 0x4F0 bytes.
>
> - The instruction at 0x1_3B8A_76EC faults. Not shown here, but it is a
> store instruction, which writes to the field at offset 0x70 of the
> structure pointed-to by the X0 register. This is the "mod->next"
> assignment from grub_dl_init().
>
> - The faulting address is therefore (X0 + 0x70), i.e., 0x1_3B88_B9D0. This
> is indeed the value held in the FAR register.
>
> - The faulting address 0x1_3B88_B9D0 falls in the above-noted page (at
> 0x1_3B88_B000), namely at offset 0x9D0. This is *beyond* the first 0x4F0
> bytes that the very tail of the "video_fb" module occupies at the front
> of that page.
>
> For now, add a self-check that reports this bug (and prevents the crash by
> skipping the write protection).
>
> Example log after the patch:
>
> > kern/dl.c:742:BUG: trying to protect pages outside of module allocation
> > ("video_fb"): module base 0x13b87d000, size 0xe4f0; tramp/GOT base
> > 0x13b88b000, size 0x1000
>
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> (cherry picked from commit deb4c03d7658f0695db8217896c2830009b7469d)
> Signed-off-by: Jan Setje-Eilers <jan.setjeeil...@oracle.com>
> Signed-off-by: Mate Kukri <mate.ku...@canonical.com>
> ---
> grub-core/kern/dl.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 9f31ad3b9..042033a90 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -645,7 +645,7 @@ grub_dl_set_mem_attrs (grub_dl_t mod, void *ehdr)
> #if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv)
> grub_size_t arch_addralign = grub_arch_dl_min_alignment ();
> grub_addr_t tgaddr;
> - grub_uint64_t tgsz;
> + grub_size_t tgsz;
> #endif
>
> grub_dprintf ("modules", "updating memory attributes for \"%s\"\n",
> @@ -698,6 +698,15 @@ grub_dl_set_mem_attrs (grub_dl_t mod, void *ehdr)
>
> grub_dprintf ("modules", "updating attributes for GOT and
> trampolines\n",
> mod->name);
> + 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);
> grub_update_mem_attrs (tgaddr, tgsz, GRUB_MEM_ATTR_R|GRUB_MEM_ATTR_X,
> GRUB_MEM_ATTR_W);
> }
> --
> 2.39.2
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
--
Regards
Vladimir 'phcoder' Serbinenko
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
