Re: [PATCH] efi: zboot: Ensure zboot PIC

[Pingfan Liu <kernelfans@gmail.com>]

On Mon, Mar 27, 2023 at 6:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 27 Mar 2023 at 11:51, Pingfan Liu <kernelfans@gmail.com> wrote:
> >
> > Using objcopy to reform vmlinuz.efi.elf to vmlinuz.efi will not convey
> > any relocation information. That means vmlinuz.efi is expected to be
> > PIC.
> >
> > At present, vmlinuz.efi is PIC. But it is better to adopt the same
>
> Why is it better?
>

I think except carefully coded with asm language, there is no
guarantee which prevents the compiler from generating position
dependent code.  Or is there any presumption here?

> > solution used by the kernel to resolve the code relocation issue by
> > itself. That is to resolve R_AARCH64_RELATIVE at the runtime.
> >
>
> This breaks other architectures.
>

Oops, could it be done by:
#if defined(_aarch64_)
#define R_ARCH_RELATIVE R_AARCH64_RELATIVE
#else
#define R_ARCH_RELATIVE 0
#endif

Since any r_type equaling 0 can not be ejected into the relocation
section, this macro will make the code dummy in essential.

Thanks,

    Pingfan

> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Huacai Chen <chenhuacai@kernel.org>
> > To: linux-efi@vger.kernel.org
>
>
> > ---
> >  drivers/firmware/efi/libstub/Makefile       |  2 +-
> >  drivers/firmware/efi/libstub/Makefile.zboot |  2 +-
> >  drivers/firmware/efi/libstub/zboot-entry.S  | 35 +++++++++++++++++++++
> >  drivers/firmware/efi/libstub/zboot.c        |  2 +-
> >  drivers/firmware/efi/libstub/zboot.lds      |  6 ++++
> >  5 files changed, 44 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/firmware/efi/libstub/zboot-entry.S
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index 80d85a5169fb..4447395d7218 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -95,7 +95,7 @@ lib-$(CONFIG_LOONGARCH)               += loongarch.o loongarch-stub.o
> >  CFLAGS_arm32-stub.o            := -DTEXT_OFFSET=$(TEXT_OFFSET)
> >
> >  zboot-obj-$(CONFIG_RISCV)      := lib-clz_ctz.o lib-ashldi3.o
> > -lib-$(CONFIG_EFI_ZBOOT)                += zboot.o $(zboot-obj-y)
> > +lib-$(CONFIG_EFI_ZBOOT)                += zboot-entry.o zboot.o $(zboot-obj-y)
> >
> >  extra-y                                := $(lib-y)
> >  lib-y                          := $(patsubst %.o,%.stub.o,$(lib-y))
> > diff --git a/drivers/firmware/efi/libstub/Makefile.zboot b/drivers/firmware/efi/libstub/Makefile.zboot
> > index 43e9a4cab9f5..1ed948cee92f 100644
> > --- a/drivers/firmware/efi/libstub/Makefile.zboot
> > +++ b/drivers/firmware/efi/libstub/Makefile.zboot
> > @@ -36,7 +36,7 @@ $(obj)/zboot-header.o: $(srctree)/drivers/firmware/efi/libstub/zboot-header.S FO
> >
> >  ZBOOT_DEPS := $(obj)/zboot-header.o $(objtree)/drivers/firmware/efi/libstub/lib.a
> >
> > -LDFLAGS_vmlinuz.efi.elf := -T $(srctree)/drivers/firmware/efi/libstub/zboot.lds
> > +LDFLAGS_vmlinuz.efi.elf :=  --no-undefined -X -shared -Bsymbolic -z notext --no-apply-dynamic-relocs -T $(srctree)/drivers/firmware/efi/libstub/zboot.lds
> >  $(obj)/vmlinuz.efi.elf: $(obj)/vmlinuz.o $(ZBOOT_DEPS) FORCE
> >         $(call if_changed,ld)
> >
> > diff --git a/drivers/firmware/efi/libstub/zboot-entry.S b/drivers/firmware/efi/libstub/zboot-entry.S
> > new file mode 100644
> > index 000000000000..072207f2f6ba
> > --- /dev/null
> > +++ b/drivers/firmware/efi/libstub/zboot-entry.S
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <asm/elf.h>
> > +#define ZBOOT_HEADER_BASE 0
> > +
> > +       .text
> > +/*
> > + * x0: efi_handle_t
> > + * x1: efi_system_table_t *
> > + */
> > +       .global efi_zboot_entry
> > +efi_zboot_entry:
> > +       adrp    x2, efi_zboot_header
> > +       add     x2, x2, :lo12:efi_zboot_header
> > +       mov     x3, ZBOOT_HEADER_BASE
> > +       sub     x3, x2, x3              // delta between actual and linked address
> > +       adrp    x4, _rela_start
> > +       add     x4, x4, :lo12:_rela_start
> > +       adrp    x5, _rela_end
> > +       add     x5, x5, :lo12:_rela_end
> > +
> > +0:     cmp     x5, x4
> > +       b.hs    1f
> > +       ldp     x6, x7, [x4], #24
> > +       ldr     x8, [x4, #-8]
> > +       cmp     w7, #R_AARCH64_RELATIVE
> > +       b.ne    0b
> > +       add     x8, x8, x3
> > +       str     x8, [x6, x3]
> > +       b       0b
> > +
> > +1:
> > +       dsb     ishst
> > +       ic      iallu
> > +       b       efi_zboot_main
> > diff --git a/drivers/firmware/efi/libstub/zboot.c b/drivers/firmware/efi/libstub/zboot.c
> > index ba234e062a1a..7aa6b2e6d104 100644
> > --- a/drivers/firmware/efi/libstub/zboot.c
> > +++ b/drivers/firmware/efi/libstub/zboot.c
> > @@ -58,7 +58,7 @@ void __weak efi_cache_sync_image(unsigned long image_base,
> >  }
> >
> >  asmlinkage efi_status_t __efiapi
> > -efi_zboot_entry(efi_handle_t handle, efi_system_table_t *systab)
> > +efi_zboot_main(efi_handle_t handle, efi_system_table_t *systab)
> >  {
> >         unsigned long compressed_size = _gzdata_end - _gzdata_start;
> >         unsigned long image_base, alloc_size, code_size;
> > diff --git a/drivers/firmware/efi/libstub/zboot.lds b/drivers/firmware/efi/libstub/zboot.lds
> > index 93d33f68333b..631942604c3f 100644
> > --- a/drivers/firmware/efi/libstub/zboot.lds
> > +++ b/drivers/firmware/efi/libstub/zboot.lds
> > @@ -12,6 +12,12 @@ SECTIONS
> >                 *(.text* .init.text*)
> >         }
> >
> > +       .rela.dyn : ALIGN(8) {
> > +               __efistub__rela_start = .;
> > +               *(.rela .rela*)
> > +               __efistub__rela_end = .;
> > +       }
> > +
> >         .rodata : ALIGN(8) {
> >                 __efistub__gzdata_start = .;
> >                 *(.gzdata)
> > --
> > 2.31.1
> >