[PATCH -next] x86/efi_64: fix a user-memory-access in runtime

[PATCH -next] x86/efi_64: fix a user-memory-access in runtime

[Qian Cai]

The commit 698294704573 ("efi/x86: Split SetVirtualAddresMap() wrappers
into 32 and 64 bit versions") introduced a KASAN error during boot,

 BUG: KASAN: user-memory-access in efi_set_virtual_address_map+0x4d3/0x574
 Read of size 8 at addr 00000000788fee50 by task swapper/0/0

 Hardware name: HP ProLiant XL450 Gen9 Server/ProLiant XL450 Gen9
 Server, BIOS U21 05/05/2016
 Call Trace:
  dump_stack+0xa0/0xea
  __kasan_report.cold.8+0xb0/0xc0
  kasan_report+0x12/0x20
  __asan_load8+0x71/0xa0
  efi_set_virtual_address_map+0x4d3/0x574
  efi_enter_virtual_mode+0x5f3/0x64e
  start_kernel+0x53a/0x5dc
  x86_64_start_reservations+0x24/0x26
  x86_64_start_kernel+0xf4/0xfb
  secondary_startup_64+0xb6/0xc0

It points to this line,

status = efi_call(efi.systab->runtime->set_virtual_address_map,

efi.systab->runtime's address is 00000000788fee18 which is an address in
EFI runtime service and does not have a KASAN shadow page. Fix it by
doing a copy_from_user() first instead.

Fixes: 698294704573 ("efi/x86: Split SetVirtualAddresMap() wrappers into 32 and 64 bit versions")
Signed-off-by: Qian Cai <cai@lca.pw>
---
 arch/x86/platform/efi/efi_64.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 515eab388b56..d6712c9cb9d8 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -1023,6 +1023,7 @@ efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
 						u32 descriptor_version,
 						efi_memory_desc_t *virtual_map)
 {
+	efi_runtime_services_t runtime;
 	efi_status_t status;
 	unsigned long flags;
 	pgd_t *save_pgd = NULL;
@@ -1041,13 +1042,15 @@ efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
 		efi_switch_mm(&efi_mm);
 	}
 
+	if (copy_from_user(&runtime, efi.systab->runtime, sizeof(runtime)))
+		return EFI_ABORTED;
+
 	kernel_fpu_begin();
 
 	/* Disable interrupts around EFI calls: */
 	local_irq_save(flags);
-	status = efi_call(efi.systab->runtime->set_virtual_address_map,
-			  memory_map_size, descriptor_size,
-			  descriptor_version, virtual_map);
+	status = efi_call(runtime.set_virtual_address_map, memory_map_size,
+			  descriptor_size, descriptor_version, virtual_map);
 	local_irq_restore(flags);
 
 	kernel_fpu_end();
-- 
2.21.0 (Apple Git-122.2)

Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime

[Ard Biesheuvel]

On Sat, 18 Jan 2020 at 07:30, Qian Cai <cai@lca.pw> wrote:
>
> The commit 698294704573 ("efi/x86: Split SetVirtualAddresMap() wrappers
> into 32 and 64 bit versions") introduced a KASAN error during boot,
>
>  BUG: KASAN: user-memory-access in efi_set_virtual_address_map+0x4d3/0x574
>  Read of size 8 at addr 00000000788fee50 by task swapper/0/0
>
>  Hardware name: HP ProLiant XL450 Gen9 Server/ProLiant XL450 Gen9
>  Server, BIOS U21 05/05/2016
>  Call Trace:
>   dump_stack+0xa0/0xea
>   __kasan_report.cold.8+0xb0/0xc0
>   kasan_report+0x12/0x20
>   __asan_load8+0x71/0xa0
>   efi_set_virtual_address_map+0x4d3/0x574
>   efi_enter_virtual_mode+0x5f3/0x64e
>   start_kernel+0x53a/0x5dc
>   x86_64_start_reservations+0x24/0x26
>   x86_64_start_kernel+0xf4/0xfb
>   secondary_startup_64+0xb6/0xc0
>
> It points to this line,
>
> status = efi_call(efi.systab->runtime->set_virtual_address_map,
>
> efi.systab->runtime's address is 00000000788fee18 which is an address in
> EFI runtime service and does not have a KASAN shadow page. Fix it by
> doing a copy_from_user() first instead.
>

Can't we just use READ_ONCE_NOCHECK() instead?

> Fixes: 698294704573 ("efi/x86: Split SetVirtualAddresMap() wrappers into 32 and 64 bit versions")
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  arch/x86/platform/efi/efi_64.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 515eab388b56..d6712c9cb9d8 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -1023,6 +1023,7 @@ efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
>                                                 u32 descriptor_version,
>                                                 efi_memory_desc_t *virtual_map)
>  {
> +       efi_runtime_services_t runtime;
>         efi_status_t status;
>         unsigned long flags;
>         pgd_t *save_pgd = NULL;
> @@ -1041,13 +1042,15 @@ efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
>                 efi_switch_mm(&efi_mm);
>         }
>
> +       if (copy_from_user(&runtime, efi.systab->runtime, sizeof(runtime)))
> +               return EFI_ABORTED;
> +
>         kernel_fpu_begin();
>
>         /* Disable interrupts around EFI calls: */
>         local_irq_save(flags);
> -       status = efi_call(efi.systab->runtime->set_virtual_address_map,
> -                         memory_map_size, descriptor_size,
> -                         descriptor_version, virtual_map);
> +       status = efi_call(runtime.set_virtual_address_map, memory_map_size,
> +                         descriptor_size, descriptor_version, virtual_map);
>         local_irq_restore(flags);
>
>         kernel_fpu_end();
> --
> 2.21.0 (Apple Git-122.2)
>

Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime

[Qian Cai]

> On Jan 18, 2020, at 3:00 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> Can't we just use READ_ONCE_NOCHECK() instead?

My understanding is that KASAN actually want to make sure there is a no dereference of user memory because it has security implications. Does that make no sense here?

Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime

[Ard Biesheuvel]

On Sat, 18 Jan 2020 at 12:04, Qian Cai <cai@lca.pw> wrote:
>
>
>
> > On Jan 18, 2020, at 3:00 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > Can't we just use READ_ONCE_NOCHECK() instead?
>
> My understanding is that KASAN actually want to make sure there is a no dereference of user memory because it has security implications. Does that make no sense here?

Not really. This code runs extremely early in the boot, with a
temporary 1:1 memory mapping installed so that the EFI firmware can
transition into virtually remapped mode.

Furthermore, the same issue exists for mixed mode, so we'll need to
fix that as well. I'll spin a patch and credit you as the reporter.

Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime

[Dmitry Vyukov]

On Sat, Jan 18, 2020 at 2:35 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> > > On Jan 18, 2020, at 3:00 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > >
> > > Can't we just use READ_ONCE_NOCHECK() instead?
> >
> > My understanding is that KASAN actually want to make sure there is a no dereference of user memory because it has security implications. Does that make no sense here?
>
> Not really. This code runs extremely early in the boot, with a
> temporary 1:1 memory mapping installed so that the EFI firmware can
> transition into virtually remapped mode.
>
> Furthermore, the same issue exists for mixed mode, so we'll need to
> fix that as well. I'll spin a patch and credit you as the reporter.

If this code runs extremely early and uses even completely different
mapping, it may make sense to disable KASAN instrumentation of this
file in Makefile.

Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime

[Ard Biesheuvel]

On Sat, 18 Jan 2020 at 14:37, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Sat, Jan 18, 2020 at 2:35 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > > > On Jan 18, 2020, at 3:00 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > >
> > > > Can't we just use READ_ONCE_NOCHECK() instead?
> > >
> > > My understanding is that KASAN actually want to make sure there is a no dereference of user memory because it has security implications. Does that make no sense here?
> >
> > Not really. This code runs extremely early in the boot, with a
> > temporary 1:1 memory mapping installed so that the EFI firmware can
> > transition into virtually remapped mode.
> >
> > Furthermore, the same issue exists for mixed mode, so we'll need to
> > fix that as well. I'll spin a patch and credit you as the reporter.
>
> If this code runs extremely early and uses even completely different
> mapping, it may make sense to disable KASAN instrumentation of this
> file in Makefile.

The routine in question runs extremely early, but the other code in
the file may be called at any time, so this is probably not the right
choice in this case.

Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime

[kbuild test robot]

Hi Qian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200117]
[cannot apply to efi/next v5.5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Qian-Cai/x86-efi_64-fix-a-user-memory-access-in-runtime/20200118-171142
base:    de970dffa7d19eae1d703c3534825308ef8d5dec
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-153-g47b6dfef-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> arch/x86/platform/efi/efi_64.c:1045:48: sparse: sparse: incorrect type in argument 2 (different address spaces)
>> arch/x86/platform/efi/efi_64.c:1045:48: sparse:    expected void const [noderef] <asn:1> *from
>> arch/x86/platform/efi/efi_64.c:1045:48: sparse:    got union efi_runtime_services_t [usertype] *[usertype] runtime

vim +1045 arch/x86/platform/efi/efi_64.c

  1020	
  1021	efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
  1022							unsigned long descriptor_size,
  1023							u32 descriptor_version,
  1024							efi_memory_desc_t *virtual_map)
  1025	{
  1026		efi_runtime_services_t runtime;
  1027		efi_status_t status;
  1028		unsigned long flags;
  1029		pgd_t *save_pgd = NULL;
  1030	
  1031		if (efi_is_mixed())
  1032			return efi_thunk_set_virtual_address_map(memory_map_size,
  1033								 descriptor_size,
  1034								 descriptor_version,
  1035								 virtual_map);
  1036	
  1037		if (efi_enabled(EFI_OLD_MEMMAP)) {
  1038			save_pgd = efi_old_memmap_phys_prolog();
  1039			if (!save_pgd)
  1040				return EFI_ABORTED;
  1041		} else {
  1042			efi_switch_mm(&efi_mm);
  1043		}
  1044	
> 1045		if (copy_from_user(&runtime, efi.systab->runtime, sizeof(runtime)))

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation