[PATCH 0/3] EFI fixes for -next

[PATCH 0/3] EFI fixes for -next

[Ard Biesheuvel]

A couple of fixes for more unusual configurations that weren't caught in
my testing before. One for KASAN, one for the LLVM linker and one for the
old EFI memory map when running on mixed mode systems.

These apply onto the Git pull request [0[ that I sent out at the beginning
of the week.

[0] https://lore.kernel.org/linux-efi/20200113172245.27925-1-ardb@kernel.org/

Ard Biesheuvel (3):
  efi/x86: avoid KASAN false positives when accessing the 1:1 mapping
  x86/boot/compressed: relax sed symbol type regex for LLVM ld.lld
  efi/x86: disallow efi=old_map in mixed mode

 arch/x86/boot/Makefile         |  2 +-
 arch/x86/platform/efi/efi_64.c | 11 ++++++-----
 arch/x86/platform/uv/bios_uv.c |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH 1/3] efi/x86: avoid KASAN false positives when accessing the 1:1 mapping

[Ard Biesheuvel]

When installing the EFI virtual address map during early boot, we
access the EFI system table to retrieve the 1:1 mapped address of
the SetVirtualAddressMap() EFI runtime service. This memory is not
known to KASAN, so on KASAN enabled builds, this may result in a
splat like

  ==================================================================
  BUG: KASAN: user-memory-access in efi_set_virtual_address_map+0x141/0x354
  Read of size 4 at addr 000000003fbeef38 by task swapper/0/0

  CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.5.0-rc5+ #758
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  Call Trace:
   dump_stack+0x8b/0xbb
   ? efi_set_virtual_address_map+0x141/0x354
   ? efi_set_virtual_address_map+0x141/0x354
   __kasan_report+0x176/0x192
   ? efi_set_virtual_address_map+0x141/0x354
   kasan_report+0xe/0x20
   efi_set_virtual_address_map+0x141/0x354
   ? efi_thunk_runtime_setup+0x148/0x148
   ? __inc_numa_state+0x19/0x90
   ? memcpy+0x34/0x50
   efi_enter_virtual_mode+0x5fd/0x67d
   start_kernel+0x5cd/0x682
   ? mem_encrypt_init+0x6/0x6
   ? x86_family+0x5/0x20
   ? load_ucode_bsp+0x46/0x154
   secondary_startup_64+0xa4/0xb0
  ==================================================================

Since this code runs only a single time during early boot, let's annotate
it as __no_sanitize_address so KASAN disregards it entirely.

Fixes: 698294704573 ("efi/x86: Split SetVirtualAddresMap() wrappers into ...")
Reported-by: Qian Cai <cai@lca.pw>
Cc: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/efi_64.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 8d1869ff1033..e2accfe636bd 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -543,7 +543,7 @@ static DEFINE_SPINLOCK(efi_runtime_lock);
 	__s;								\
 })
 
-static efi_status_t __init
+static efi_status_t __init __no_sanitize_address
 efi_thunk_set_virtual_address_map(unsigned long memory_map_size,
 				  unsigned long descriptor_size,
 				  u32 descriptor_version,
@@ -882,10 +882,11 @@ void __init efi_thunk_runtime_setup(void)
 	efi.query_capsule_caps = efi_thunk_query_capsule_caps;
 }
 
-efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
-						unsigned long descriptor_size,
-						u32 descriptor_version,
-						efi_memory_desc_t *virtual_map)
+efi_status_t __init __no_sanitize_address
+efi_set_virtual_address_map(unsigned long memory_map_size,
+			    unsigned long descriptor_size,
+			    u32 descriptor_version,
+			    efi_memory_desc_t *virtual_map)
 {
 	efi_status_t status;
 	unsigned long flags;
-- 
2.17.1

[PATCH 2/3] x86/boot/compressed: relax sed symbol type regex for LLVM ld.lld

[Ard Biesheuvel]

The final build stage of the x86 kernel captures some symbol
addresses from the decompressor binary and copies them into zoffset.h.
It uses sed with a regular expression that matches the address, symbol
type and symbol name, and mangles the captured addresses and the names
of symbols of interest into #define directives that are added to
zoffset.h

The symbol type is indicated by a single letter, which we match
strictly: only letters in the set 'ABCDGRSTVW' are matched, even
though the actual symbol type is relevant and therefore ignored.

Commit bc7c9d620 ("efi/libstub/x86: Force 'hidden' visibility for
extern declarations") made a change to the way external symbol
references are classified, resulting in 'startup_32' now being
emitted as a hidden symbol. This prevents the use of GOT entries to
refer to this symbol via its absolute address, which recent toolchains
(including Clang based ones) already avoid by default, making this
change a no-op in the majority of cases.

However, as it turns out, the LLVM linker classifies such hidden
symbols as symbols with static linkage in fully linked ELF binaries,
causing tools such as NM to output a lowercase 't' rather than an upper
case 'T' for the type of such symbols. Since our sed expression only
matches upper case letters for the symbol type, the line describing
startup_32 is disregarded, resulting in a build error like the following

  arch/x86/boot/header.S:568:18: error: symbol 'ZO_startup_32' can not be
                                        undefined in a subtraction expression
  init_size: .long (0x00000000008fd000 - ZO_startup_32 +
                    (((0x0000000001f6361c + ((0x0000000001f6361c >> 8) + 65536)
                     - 0x00000000008c32e5) + 4095) & ~4095)) # kernel initialization size

Given that we are only interested in the value of the symbol, let's match
any character in the set 'a-zA-Z' instead.

Tested-by: Nathan Chancellor <natechancellor@gmail.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index 95410d6ee2ff..748b6d28a91d 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -88,7 +88,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
 
 SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
 
-sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [ABCDGRSTVW] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
+sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
 
 quiet_cmd_zoffset = ZOFFSET $@
       cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
-- 
2.17.1

Re: [PATCH 2/3] x86/boot/compressed: relax sed symbol type regex for LLVM ld.lld

[Nick Desaulniers]

On Sat, Jan 18, 2020 at 8:57 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> The final build stage of the x86 kernel captures some symbol
> addresses from the decompressor binary and copies them into zoffset.h.
> It uses sed with a regular expression that matches the address, symbol
> type and symbol name, and mangles the captured addresses and the names
> of symbols of interest into #define directives that are added to
> zoffset.h
>
> The symbol type is indicated by a single letter, which we match
> strictly: only letters in the set 'ABCDGRSTVW' are matched, even
> though the actual symbol type is relevant and therefore ignored.
>
> Commit bc7c9d620 ("efi/libstub/x86: Force 'hidden' visibility for
> extern declarations") made a change to the way external symbol
> references are classified, resulting in 'startup_32' now being
> emitted as a hidden symbol. This prevents the use of GOT entries to
> refer to this symbol via its absolute address, which recent toolchains
> (including Clang based ones) already avoid by default, making this
> change a no-op in the majority of cases.
>
> However, as it turns out, the LLVM linker classifies such hidden
> symbols as symbols with static linkage in fully linked ELF binaries,
> causing tools such as NM to output a lowercase 't' rather than an upper
> case 'T' for the type of such symbols. Since our sed expression only
> matches upper case letters for the symbol type, the line describing
> startup_32 is disregarded, resulting in a build error like the following
>
>   arch/x86/boot/header.S:568:18: error: symbol 'ZO_startup_32' can not be
>                                         undefined in a subtraction expression
>   init_size: .long (0x00000000008fd000 - ZO_startup_32 +
>                     (((0x0000000001f6361c + ((0x0000000001f6361c >> 8) + 65536)
>                      - 0x00000000008c32e5) + 4095) & ~4095)) # kernel initialization size
>
> Given that we are only interested in the value of the symbol, let's match
> any character in the set 'a-zA-Z' instead.
>
> Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>

Thanks for the patch! This fixes a build breakage for us.
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/842

> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/boot/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
> index 95410d6ee2ff..748b6d28a91d 100644
> --- a/arch/x86/boot/Makefile
> +++ b/arch/x86/boot/Makefile
> @@ -88,7 +88,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
>
>  SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
>
> -sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [ABCDGRSTVW] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
> +sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
>
>  quiet_cmd_zoffset = ZOFFSET $@
>        cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
> --
> 2.17.1
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 3/3] efi/x86: disallow efi=old_map in mixed mode

[Ard Biesheuvel]

Before commit ... ("efi/x86: limit EFI old memory map to SGI UV
machines"), enabling the old EFI memory map on mixed mode systems
disabled EFI runtime services altogether. Given that efi=old_map is
a debug feature designed to work around firmware problems related
to EFI runtime services, and disabling them can be achieved more
straightforwardly using 'noefi' or 'efi=noruntime', it makes more
sense to ignore efi=old_map on mixed mode systems.

Currently, we do neither, and try to use the old memory map in
combination with mixed mode routines, which results in crashes,
so let's fix this by making efi=old_map functional on native
systems only.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/uv/bios_uv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index 7c2b8c5d0b49..607f58147311 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -371,7 +371,7 @@ static int __init arch_parse_efi_cmdline(char *str)
 		return -EINVAL;
 	}
 
-	if (parse_option_str(str, "old_map"))
+	if (!efi_is_mixed() && parse_option_str(str, "old_map"))
 		set_bit(EFI_UV1_MEMMAP, &efi.flags);
 
 	return 0;
-- 
2.17.1

Re: [PATCH 0/3] EFI fixes for -next

[Ard Biesheuvel]

On Mon, 20 Jan 2020 at 10:49, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > A couple of fixes for more unusual configurations that weren't caught in
> > my testing before. One for KASAN, one for the LLVM linker and one for the
> > old EFI memory map when running on mixed mode systems.
> >
> > These apply onto the Git pull request [0[ that I sent out at the beginning
> > of the week.
> >
> > [0] https://lore.kernel.org/linux-efi/20200113172245.27925-1-ardb@kernel.org/
> >
> > Ard Biesheuvel (3):
> >   efi/x86: avoid KASAN false positives when accessing the 1:1 mapping
> >   x86/boot/compressed: relax sed symbol type regex for LLVM ld.lld
> >   efi/x86: disallow efi=old_map in mixed mode
> >
> >  arch/x86/boot/Makefile         |  2 +-
> >  arch/x86/platform/efi/efi_64.c | 11 ++++++-----
> >  arch/x86/platform/uv/bios_uv.c |  2 +-
> >  3 files changed, 8 insertions(+), 7 deletions(-)
>
> Just a minor bugreport, in some (weird) config combinations we now get
> this build failure:
>
>   ld: arch/x86/platform/efi/efi_64.o: in function `efi_set_virtual_address_map':
>   efi_64.c:(.init.text+0x1419): undefined reference to `__efi64_thunk'
>   ld: efi_64.c:(.init.text+0x1530): undefined reference to `efi_uv1_memmap_phys_prolog'
>   ld: efi_64.c:(.init.text+0x1706): undefined reference to `efi_uv1_memmap_phys_epilog'
>
> Config attached.
>
> I believe the trigger condition is:
>
>   !CONFIG_X86_UV
>   CONFIG_EFI=y
>

Strange.

Those references to missing symbols are guarded by efi_is_mixed() and
efi_have_uv1_memmap(), both of which are static inline bool()
functions wrapping IS_ENABLED() checks against CONFIG_EFI_MIXED and
CONFIG_X86_UV, respectively. IOW, it is unexpected that the compiler
doesn't const-propagate those expressions and optimize away the
references entirely.

Is there any special debug or diagnostic options enabled?

Re: [PATCH 0/3] EFI fixes for -next

[Ard Biesheuvel]

On Mon, 20 Jan 2020 at 11:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Mon, 20 Jan 2020 at 10:49, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > > A couple of fixes for more unusual configurations that weren't caught in
> > > my testing before. One for KASAN, one for the LLVM linker and one for the
> > > old EFI memory map when running on mixed mode systems.
> > >
> > > These apply onto the Git pull request [0[ that I sent out at the beginning
> > > of the week.
> > >
> > > [0] https://lore.kernel.org/linux-efi/20200113172245.27925-1-ardb@kernel.org/
> > >
> > > Ard Biesheuvel (3):
> > >   efi/x86: avoid KASAN false positives when accessing the 1:1 mapping
> > >   x86/boot/compressed: relax sed symbol type regex for LLVM ld.lld
> > >   efi/x86: disallow efi=old_map in mixed mode
> > >
> > >  arch/x86/boot/Makefile         |  2 +-
> > >  arch/x86/platform/efi/efi_64.c | 11 ++++++-----
> > >  arch/x86/platform/uv/bios_uv.c |  2 +-
> > >  3 files changed, 8 insertions(+), 7 deletions(-)
> >
> > Just a minor bugreport, in some (weird) config combinations we now get
> > this build failure:
> >
> >   ld: arch/x86/platform/efi/efi_64.o: in function `efi_set_virtual_address_map':
> >   efi_64.c:(.init.text+0x1419): undefined reference to `__efi64_thunk'
> >   ld: efi_64.c:(.init.text+0x1530): undefined reference to `efi_uv1_memmap_phys_prolog'
> >   ld: efi_64.c:(.init.text+0x1706): undefined reference to `efi_uv1_memmap_phys_epilog'
> >
> > Config attached.
> >
> > I believe the trigger condition is:
> >
> >   !CONFIG_X86_UV
> >   CONFIG_EFI=y
> >
>
> Strange.
>
> Those references to missing symbols are guarded by efi_is_mixed() and
> efi_have_uv1_memmap(), both of which are static inline bool()
> functions wrapping IS_ENABLED() checks against CONFIG_EFI_MIXED and
> CONFIG_X86_UV, respectively. IOW, it is unexpected that the compiler
> doesn't const-propagate those expressions and optimize away the
> references entirely.
>

I think we should simply add the following - this should also cover
the remaining mixed mode KASAN issue that I mentioned.

diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 7ec3a8b31f8b..84b09c230cbd 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 OBJECT_FILES_NON_STANDARD_efi_thunk_$(BITS).o := y
+KASAN_SANITIZE := n
+GCOV_PROFILE := n

 obj-$(CONFIG_EFI)              += quirks.o efi.o efi_$(BITS).o
efi_stub_$(BITS).o
 obj-$(CONFIG_EFI_MIXED)                += efi_thunk_$(BITS).o