From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EFEE4C4338F for ; Thu, 29 Jul 2021 19:31:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CF1A760C40 for ; Thu, 29 Jul 2021 19:31:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231245AbhG2Tbp (ORCPT ); Thu, 29 Jul 2021 15:31:45 -0400 Received: from mail.kernel.org ([198.145.29.99]:35442 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229724AbhG2Tbn (ORCPT ); Thu, 29 Jul 2021 15:31:43 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 89512608FB; Thu, 29 Jul 2021 19:31:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1627587100; bh=IRjaTbat4W9DpEHrlCHgReStseh1FP+5O49lG8yTuKM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=VFTxTjV2sWVbU2lnJ/4Qe+sO79mCKeiIlkq+8eJpgwhR4TwK2eJqzACgJHjfQlsd7 Gtnw5ptHsfTmaZHYCfuPeCSsyzOjeiniqE2+6eLRfa2sNLCjaHvYr/eXygeIa2wNeD AassnoKlDoQNxKy5mbI708/AthsHmYCAskyiKhsCFPxwEAKp1+QCrz+frnfGDuwAl+ 8FCzUzYO0clqU7Urxm0IoKEgfRZVMpIfxUprsNx5JtZ54LKuUO5M42T1T9Kc4mXQkd OJ4iHVoBdpnOeBG2Gjmrh8dCp7oCCLYygSaQKsZ1wP0j7TQWm7enl8JFasg7jTH+8a tUI4VBFvwvbCA== Date: Thu, 29 Jul 2021 22:31:33 +0300 From: Mike Rapoport To: Maurizio Lombardi Cc: bp@alien8.de, tglx@linutronix.de, x86@kernel.org, pjones@redhat.com, konrad@kernel.org, george.kennedy@oracle.com, rafael@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping Message-ID: References: <20210729135250.32212-1-mlombard@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210729135250.32212-1-mlombard@redhat.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 29, 2021 at 03:52:50PM +0200, Maurizio Lombardi wrote: > Starting with commit a799c2bd29d1 > ("x86/setup: Consolidate early memory reservations") > memory reservations have been moved earlier during the boot process, > before the execution of the Kernel Address Space Layout Randomization code. > > setup_arch() calls the iscsi_ibft's find_ibft_region() function > to find and reserve the memory dedicated to the iBFT and this function > also saves a virtual pointer to the iBFT table for later use. > > The problem is that if KALSR is active, the physical memory gets > remapped somewhere else in the virtual address space and the pointer is > no longer valid, this will cause a kernel panic when the iscsi driver tries > to dereference it. > > [ 37.764225] iBFT detected. > [ 37.778877] BUG: unable to handle page fault for address: ffff888000099fd8 > [ 37.816542] #PF: supervisor read access in kernel mode > [ 37.844304] #PF: error_code(0x0000) - not-present page > [ 37.872857] PGD 0 P4D 0 > [ 37.886985] Oops: 0000 [#1] SMP PTI > [ 37.904809] CPU: 46 PID: 1073 Comm: modprobe Tainted: G X --------- --- 5.13.0-0.rc2.19.el9.x86_64 #1 > [ 37.956525] Hardware name: HP ProLiant DL580 G7, BIOS P65 10/01/2013 > [ 37.987170] RIP: 0010:ibft_init+0x3e/0xd42 [iscsi_ibft] > [ 38.012976] Code: 04 25 28 00 00 00 48 89 44 24 08 31 c0 48 83 3d e1 cc 7e d7 00 74 28 48 c7 c7 21 81 1b c0 e8 b3 10 81 d5 48 8b 05 cc cc 7e d7 <0f> b6 70 08 48 63 50 04 40 80 fe 01 75 5e 31 f6 48 01 c2 eb 6e 83 > [ 38.106835] RSP: 0018:ffffb7d288fc3db0 EFLAGS: 00010246 > [ 38.131341] RAX: ffff888000099fd0 RBX: 0000000000000000 RCX: 0000000000000000 > [ 38.167110] RDX: 0000000000000000 RSI: ffff9ba7efb97c80 RDI: ffff9ba7efb97c80 > [ 38.200777] RBP: ffffffffc01c82be R08: 0000000000000000 R09: ffffb7d288fc3bf0 > [ 38.237188] R10: ffffb7d288fc3be8 R11: ffffffff96de70a8 R12: ffff9ba4059d6400 > [ 38.270940] R13: 000055689f1ac050 R14: 000055689df18962 R15: ffffb7d288fc3e78 > [ 38.307167] FS: 00007f9546720b80(0000) GS:ffff9ba7efb80000(0000) knlGS:0000000000000000 > [ 38.351204] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 38.381034] CR2: ffff888000099fd8 CR3: 000000044175e004 CR4: 00000000000206e0 > [ 38.419938] Call Trace: > [ 38.432679] ? ibft_create_kobject+0x1d2/0x1d2 [iscsi_ibft] > [ 38.462584] do_one_initcall+0x44/0x1d0 > [ 38.480856] ? kmem_cache_alloc_trace+0x119/0x220 > [ 38.505554] do_init_module+0x5c/0x270 > [ 38.526578] __do_sys_init_module+0x12e/0x1b0 > [ 38.548699] do_syscall_64+0x40/0x80 > [ 38.565679] entry_SYSCALL_64_after_hwframe+0x44/0xae > > Fix this bug by saving the address of the physical location > of the ibft; later the driver will use isa_bus_to_virt() to get > the correct virtual address. > Simplify the code by renaming find_ibft_region() > to reserve_ibft_region() and remove all the wrappers. > > v2: fix a comment in linux/iscsi_ibft.h > v3: fix the commit message > > Signed-off-by: Maurizio Lombardi Reviewed-by: Mike Rapoport > --- > arch/x86/kernel/setup.c | 10 ------- > drivers/firmware/iscsi_ibft.c | 10 +++++-- > drivers/firmware/iscsi_ibft_find.c | 48 +++++++++++------------------- > include/linux/iscsi_ibft.h | 18 +++++------ > 4 files changed, 32 insertions(+), 54 deletions(-) > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index bff3a784aec5..63b20536c8d2 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -572,16 +572,6 @@ void __init reserve_standard_io_resources(void) > > } > > -static __init void reserve_ibft_region(void) > -{ > - unsigned long addr, size = 0; > - > - addr = find_ibft_region(&size); > - > - if (size) > - memblock_reserve(addr, size); > -} > - > static bool __init snb_gfx_workaround_needed(void) > { > #ifdef CONFIG_PCI > diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c > index 7127a04bca19..612a59e213df 100644 > --- a/drivers/firmware/iscsi_ibft.c > +++ b/drivers/firmware/iscsi_ibft.c > @@ -84,8 +84,10 @@ MODULE_DESCRIPTION("sysfs interface to BIOS iBFT information"); > MODULE_LICENSE("GPL"); > MODULE_VERSION(IBFT_ISCSI_VERSION); > > +static struct acpi_table_ibft *ibft_addr; > + > #ifndef CONFIG_ISCSI_IBFT_FIND > -struct acpi_table_ibft *ibft_addr; > +phys_addr_t ibft_phys_addr; > #endif > > struct ibft_hdr { > @@ -858,11 +860,13 @@ static int __init ibft_init(void) > int rc = 0; > > /* > - As on UEFI systems the setup_arch()/find_ibft_region() > + As on UEFI systems the setup_arch()/reserve_ibft_region() > is called before ACPI tables are parsed and it only does > legacy finding. > */ > - if (!ibft_addr) > + if (ibft_phys_addr) > + ibft_addr = isa_bus_to_virt(ibft_phys_addr); > + else > acpi_find_ibft_region(); > > if (ibft_addr) { > diff --git a/drivers/firmware/iscsi_ibft_find.c b/drivers/firmware/iscsi_ibft_find.c > index 64bb94523281..a0594590847d 100644 > --- a/drivers/firmware/iscsi_ibft_find.c > +++ b/drivers/firmware/iscsi_ibft_find.c > @@ -31,8 +31,8 @@ > /* > * Physical location of iSCSI Boot Format Table. > */ > -struct acpi_table_ibft *ibft_addr; > -EXPORT_SYMBOL_GPL(ibft_addr); > +phys_addr_t ibft_phys_addr; > +EXPORT_SYMBOL_GPL(ibft_phys_addr); > > static const struct { > char *sign; > @@ -47,13 +47,24 @@ static const struct { > #define VGA_MEM 0xA0000 /* VGA buffer */ > #define VGA_SIZE 0x20000 /* 128kB */ > > -static int __init find_ibft_in_mem(void) > +/* > + * Routine used to find and reserve the iSCSI Boot Format Table > + */ > +void __init reserve_ibft_region(void) > { > unsigned long pos; > unsigned int len = 0; > void *virt; > int i; > > + ibft_phys_addr = 0; > + > + /* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will > + * only use ACPI for this > + */ > + if (efi_enabled(EFI_BOOT)) > + return; > + > for (pos = IBFT_START; pos < IBFT_END; pos += 16) { > /* The table can't be inside the VGA BIOS reserved space, > * so skip that area */ > @@ -70,35 +81,12 @@ static int __init find_ibft_in_mem(void) > /* if the length of the table extends past 1M, > * the table cannot be valid. */ > if (pos + len <= (IBFT_END-1)) { > - ibft_addr = (struct acpi_table_ibft *)virt; > - pr_info("iBFT found at 0x%lx.\n", pos); > - goto done; > + ibft_phys_addr = pos; > + memblock_reserve(ibft_phys_addr, PAGE_ALIGN(len)); > + pr_info("iBFT found at 0x%lx.\n", ibft_phys_addr); > + return; > } > } > } > } > -done: > - return len; > -} > -/* > - * Routine used to find the iSCSI Boot Format Table. The logical > - * kernel address is set in the ibft_addr global variable. > - */ > -unsigned long __init find_ibft_region(unsigned long *sizep) > -{ > - ibft_addr = NULL; > - > - /* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will > - * only use ACPI for this */ > - > - if (!efi_enabled(EFI_BOOT)) > - find_ibft_in_mem(); > - > - if (ibft_addr) { > - *sizep = PAGE_ALIGN(ibft_addr->header.length); > - return (u64)virt_to_phys(ibft_addr); > - } > - > - *sizep = 0; > - return 0; > } > diff --git a/include/linux/iscsi_ibft.h b/include/linux/iscsi_ibft.h > index b7b45ca82bea..790e7fcfc1a6 100644 > --- a/include/linux/iscsi_ibft.h > +++ b/include/linux/iscsi_ibft.h > @@ -13,26 +13,22 @@ > #ifndef ISCSI_IBFT_H > #define ISCSI_IBFT_H > > -#include > +#include > > /* > - * Logical location of iSCSI Boot Format Table. > - * If the value is NULL there is no iBFT on the machine. > + * Physical location of iSCSI Boot Format Table. > + * If the value is 0 there is no iBFT on the machine. > */ > -extern struct acpi_table_ibft *ibft_addr; > +extern phys_addr_t ibft_phys_addr; > > /* > * Routine used to find and reserve the iSCSI Boot Format Table. The > - * mapped address is set in the ibft_addr variable. > + * physical address is set in the ibft_phys_addr variable. > */ > #ifdef CONFIG_ISCSI_IBFT_FIND > -unsigned long find_ibft_region(unsigned long *sizep); > +void reserve_ibft_region(void); > #else > -static inline unsigned long find_ibft_region(unsigned long *sizep) > -{ > - *sizep = 0; > - return 0; > -} > +static inline void reserve_ibft_region(void) {} > #endif > > #endif /* ISCSI_IBFT_H */ > -- > 2.27.0 > -- Sincerely yours, Mike.