Linux-EFI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH -next] x86/efi_64: fix a user-memory-access in runtime
@ 2020-01-18  6:30 Qian Cai
  2020-01-18  8:00 ` Ard Biesheuvel
  2020-01-27 23:29 ` kbuild test robot
  0 siblings, 2 replies; 7+ messages in thread
From: Qian Cai @ 2020-01-18  6:30 UTC (permalink / raw)
  To: ardb; +Cc: mingo, kasan-dev, linux-efi, linux-kernel, 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)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime
  2020-01-18  6:30 [PATCH -next] x86/efi_64: fix a user-memory-access in runtime Qian Cai
@ 2020-01-18  8:00 ` Ard Biesheuvel
  2020-01-18 11:04   ` Qian Cai
  2020-01-27 23:29 ` kbuild test robot
  1 sibling, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2020-01-18  8:00 UTC (permalink / raw)
  To: Qian Cai
  Cc: Ard Biesheuvel, Ingo Molnar, kasan-dev, linux-efi,
	Linux Kernel Mailing List

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)
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime
  2020-01-18  8:00 ` Ard Biesheuvel
@ 2020-01-18 11:04   ` Qian Cai
  2020-01-18 13:34     ` Ard Biesheuvel
  0 siblings, 1 reply; 7+ messages in thread
From: Qian Cai @ 2020-01-18 11:04 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Ard Biesheuvel, Ingo Molnar, kasan-dev, linux-efi,
	Linux Kernel Mailing List



> 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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime
  2020-01-18 11:04   ` Qian Cai
@ 2020-01-18 13:34     ` Ard Biesheuvel
  2020-01-18 13:37       ` Dmitry Vyukov
  0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2020-01-18 13:34 UTC (permalink / raw)
  To: Qian Cai
  Cc: Ard Biesheuvel, Ingo Molnar, kasan-dev, linux-efi,
	Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime
  2020-01-18 13:34     ` Ard Biesheuvel
@ 2020-01-18 13:37       ` Dmitry Vyukov
  2020-01-18 13:41         ` Ard Biesheuvel
  0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Vyukov @ 2020-01-18 13:37 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Qian Cai, Ard Biesheuvel, Ingo Molnar, kasan-dev, linux-efi,
	Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime
  2020-01-18 13:37       ` Dmitry Vyukov
@ 2020-01-18 13:41         ` Ard Biesheuvel
  0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2020-01-18 13:41 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Qian Cai, Ard Biesheuvel, Ingo Molnar, kasan-dev, linux-efi,
	Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH -next] x86/efi_64: fix a user-memory-access in runtime
  2020-01-18  6:30 [PATCH -next] x86/efi_64: fix a user-memory-access in runtime Qian Cai
  2020-01-18  8:00 ` Ard Biesheuvel
@ 2020-01-27 23:29 ` kbuild test robot
  1 sibling, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2020-01-27 23:29 UTC (permalink / raw)
  To: Qian Cai
  Cc: kbuild-all, ardb, mingo, kasan-dev, linux-efi, linux-kernel, Qian Cai

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-18  6:30 [PATCH -next] x86/efi_64: fix a user-memory-access in runtime Qian Cai
2020-01-18  8:00 ` Ard Biesheuvel
2020-01-18 11:04   ` Qian Cai
2020-01-18 13:34     ` Ard Biesheuvel
2020-01-18 13:37       ` Dmitry Vyukov
2020-01-18 13:41         ` Ard Biesheuvel
2020-01-27 23:29 ` kbuild test robot

Linux-EFI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-efi/0 linux-efi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-efi linux-efi/ https://lore.kernel.org/linux-efi \
		linux-efi@vger.kernel.org
	public-inbox-index linux-efi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-efi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git