Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

> OK so what we could do is the following:
> 
> ------------8<--------------
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..39fa2a70a7f1 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -233,6 +233,7 @@ void __init efi_init(void)
>  static bool __init efi_virtmap_init(void)
>  {
>         efi_memory_desc_t *md;
> +       u64 prev_end = 0;
> 
>         for_each_efi_memory_desc(&memmap, md) {
>                 u64 paddr, npages, size;
> @@ -256,13 +257,26 @@ static bool __init efi_virtmap_init(void)
>                  * executable, everything else can be mapped with the XN bits
>                  * set.
>                  */
> -               if (!is_normal_ram(md))
> +               if (!is_normal_ram(md)) {
>                         prot = __pgprot(PROT_DEVICE_nGnRE);
> -               else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +               } else if (md->type == EFI_RUNTIME_SERVICES_CODE) {
>                         prot = PAGE_KERNEL_EXEC;
> -               else
> +               } else {
> +                       /*
> +                        * If we are running with >4 KB pages and the current
> +                        * region shares a page frame with the preceding one,
> +                        * we should not map the leading page again since doing
> +                        * so may take its executable permissions away.
> +                        */
> +                       if (PAGE_SIZE > EFI_PAGE_SIZE && paddr < prev_end) {
> +                               paddr += PAGE_SIZE;
> +                               size -= PAGE_SIZE;
> +                               if (!size)
> +                                       continue;
> +                       }
>                         prot = PAGE_KERNEL;
> -
> +               }
> +               prev_end = paddr + size;
>                 create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
>         }
>         return true;
> ------------8<--------------
> 
> This will ensure that only the pages that are shared between 2 or more
> regions may have their permissions upgraded, but only if any of these
> regions requires it.
> 
> I prefer the much simpler previous version, though, and I think it is
> more suitable for -stable. I can always follow up with an improvement
> like this for v4.3-late.

Ok. Let's go with your previous version for now.

Could you put something in the commit message about this limitation, so
that we don't forget?

> >> >>               else
> >> >>                       prot = PAGE_KERNEL;
> >> >> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> >> >> index e29560e6b40b..cb4e9c4de952 100644
> >> >> --- a/drivers/firmware/efi/libstub/arm-stub.c
> >> >> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> >> >> @@ -13,6 +13,7 @@
> >> >>   */
> >> >>
> >> >>  #include <linux/efi.h>
> >> >> +#include <linux/sort.h>
> >> >
> >> > Sort isn't an inline in this header. I thought it wasn't safe to call
> >> > arbitary kernel functions from the stub?
> >> >
> >>
> >> We call string functions, cache maintenance functions, libfdt
> >> functions etc etc so it seems not everyone got the memo :-)
> >>
> >> I agree that treating vmlinux both as a static library and as a
> >> payload from the stub's pov is a bit sloppy, and I do remember
> >> discussing this, but for the life of me, I can't remember the exact
> >> issue, other than the use of adrp/add and adrp/ldr pairs, which we
> >> fixed by setting the PE/COFF section alignment to 4 KB.
> >
> > I only had a vague recollection that there was a problem, which I
> > thought was more to do with potential use of absolute kernel virtual
> > addresses, which would be incorrect in the context of an EFI
> > application.
> >
> 
> That was it, of course. Unlike the x86 stub, which is built with -fPIC
> (as is the ARM decompressor, btw), the arm64 kernel is position
> dependent. Fortunately, the small code model is mostly position
> independent by default, but it would be good if we could spot any
> problems at build time.
> 
> > Digging a bit, the stub code itself is safe due to commit
> > f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but that
> 
> libstub is linked into vmlinux so that does not make a different at all

Ok. I assumed that inter-stub references would still be relative, but I
have no idea what the linker would do.

> > We do seem to be ok so far, however. Maybe we just need to keep an eye
> > out.
> >
> 
> I'd much rather restrict the code that goes into the stub somehow than
> deal with any absolute references. Perhaps we could reuse some of the
> section mismatch code in some way to tag certain code as stub-safe and
> do a verification pass on the binary.

It would be great if we could, though I'm not really sure how that would
work.

Imagine stub_foo calls fdt_blah, which calls kern_baz by absolute
address. Normally the latter call is fine, but not when the original
call comes from the stub. I don't think the section mismatch code has
any way to contextualize calls like that, and I don't know what criteria
we could use for annotating code.

Let's not have that delay this patch, however. That's a bigger can of
worms.

Thanks,
Mark.