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 <[email protected]>
---
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)
On Sat, 18 Jan 2020 at 07:30, Qian Cai <[email protected]> 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 <[email protected]>
> ---
> 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)
>
> On Jan 18, 2020, at 3:00 AM, Ard Biesheuvel <[email protected]> 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?
On Sat, 18 Jan 2020 at 12:04, Qian Cai <[email protected]> wrote:
>
>
>
> > On Jan 18, 2020, at 3:00 AM, Ard Biesheuvel <[email protected]> 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.
On Sat, Jan 18, 2020 at 2:35 PM Ard Biesheuvel
<[email protected]> wrote:
> > > On Jan 18, 2020, at 3:00 AM, Ard Biesheuvel <[email protected]> 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.
On Sat, 18 Jan 2020 at 14:37, Dmitry Vyukov <[email protected]> wrote:
>
> On Sat, Jan 18, 2020 at 2:35 PM Ard Biesheuvel
> <[email protected]> wrote:
> > > > On Jan 18, 2020, at 3:00 AM, Ard Biesheuvel <[email protected]> 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.
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 <[email protected]>
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/[email protected] Intel Corporation