linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
@ 2021-07-29 13:52 Maurizio Lombardi
  2021-07-29 17:03 ` Konrad Rzeszutek Wilk
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Maurizio Lombardi @ 2021-07-29 13:52 UTC (permalink / raw)
  To: rppt; +Cc: bp, tglx, x86, pjones, konrad, george.kennedy, rafael, linux-kernel

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 <mlombard@redhat.com>
---
 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 <linux/acpi.h>
+#include <linux/types.h>
 
 /*
- * 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


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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-07-29 13:52 [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping Maurizio Lombardi
@ 2021-07-29 17:03 ` Konrad Rzeszutek Wilk
  2021-07-29 19:20   ` Maurizio Lombardi
  2021-07-29 19:31 ` Mike Rapoport
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2021-07-29 17:03 UTC (permalink / raw)
  To: Maurizio Lombardi
  Cc: rppt, bp, tglx, x86, pjones, konrad, george.kennedy, rafael,
	linux-kernel

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.

One part that I think you are saying but just want to double-check is
that the isa_bus_to_virt() virtual addresses it returns - those are
different every boot when KASLR is enabled, right?


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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-07-29 17:03 ` Konrad Rzeszutek Wilk
@ 2021-07-29 19:20   ` Maurizio Lombardi
  0 siblings, 0 replies; 10+ messages in thread
From: Maurizio Lombardi @ 2021-07-29 19:20 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Mike Rapoport, bp, tglx, x86, Peter Jones, konrad,
	George Kennedy, Rafael J. Wysocki, linux-kernel

čt 29. 7. 2021 v 19:03 odesílatel Konrad Rzeszutek Wilk
<konrad@darnok.org> napsal:
> One part that I think you are saying but just want to double-check is
> that the isa_bus_to_virt() virtual addresses it returns - those are
> different every boot when KASLR is enabled, right?
>

Yes.
If KASLR is disabled, the physical memory is mapped at virtual base
address 0xffff888000000000 (on AMD64);
if enabled, KASLR will randomize it.

Maurizio


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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-07-29 13:52 [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping Maurizio Lombardi
  2021-07-29 17:03 ` Konrad Rzeszutek Wilk
@ 2021-07-29 19:31 ` Mike Rapoport
  2021-08-01  2:44   ` Konrad Rzeszutek Wilk
  2021-08-10 17:00 ` Christoph Hellwig
  2021-09-01 16:47 ` Guenter Roeck
  3 siblings, 1 reply; 10+ messages in thread
From: Mike Rapoport @ 2021-07-29 19:31 UTC (permalink / raw)
  To: Maurizio Lombardi
  Cc: bp, tglx, x86, pjones, konrad, george.kennedy, rafael, linux-kernel

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 <mlombard@redhat.com>

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  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 <linux/acpi.h>
> +#include <linux/types.h>
>  
>  /*
> - * 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.

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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-07-29 19:31 ` Mike Rapoport
@ 2021-08-01  2:44   ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2021-08-01  2:44 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Maurizio Lombardi, bp, tglx, x86, pjones, konrad, george.kennedy,
	rafael, linux-kernel

> > Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> 
> Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

Stuck it in linux-next and modified the commit message a bit. Thanks folks!

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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-07-29 13:52 [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping Maurizio Lombardi
  2021-07-29 17:03 ` Konrad Rzeszutek Wilk
  2021-07-29 19:31 ` Mike Rapoport
@ 2021-08-10 17:00 ` Christoph Hellwig
  2021-08-10 17:55   ` Konrad Rzeszutek Wilk
  2021-09-01 16:47 ` Guenter Roeck
  3 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2021-08-10 17:00 UTC (permalink / raw)
  To: Maurizio Lombardi
  Cc: rppt, bp, tglx, x86, pjones, konrad, george.kennedy, rafael,
	linux-kernel

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

That sound rather broken.  Why not save the physical address in
find_ibft_region and then later ioremap that when a virtual address is
needed like all other code accessing magic I/O memory?

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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-08-10 17:00 ` Christoph Hellwig
@ 2021-08-10 17:55   ` Konrad Rzeszutek Wilk
  2021-08-11  6:57     ` Christoph Hellwig
  0 siblings, 1 reply; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2021-08-10 17:55 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Maurizio Lombardi, rppt, bp, tglx, x86, pjones, konrad,
	george.kennedy, rafael, linux-kernel

On Tue, Aug 10, 2021 at 06:00:24PM +0100, Christoph Hellwig wrote:
> > 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.
> 
> That sound rather broken.  Why not save the physical address in
> find_ibft_region and then later ioremap that when a virtual address is
> needed like all other code accessing magic I/O memory?

That is kind of what he does. The physical address is saved as a global
static variable and also the physical address is memreserved. Then
later on the physical address is used to create the virtual address.

Or are you thinking of making the find_ibft_region reserve the physical
address, and _cache_ the physical address so there is no global
variable ?

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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-08-10 17:55   ` Konrad Rzeszutek Wilk
@ 2021-08-11  6:57     ` Christoph Hellwig
  2021-08-11  7:20       ` Mike Rapoport
  0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2021-08-11  6:57 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Christoph Hellwig, Maurizio Lombardi, rppt, bp, tglx, x86,
	pjones, konrad, george.kennedy, rafael, linux-kernel

On Tue, Aug 10, 2021 at 01:55:11PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 10, 2021 at 06:00:24PM +0100, Christoph Hellwig wrote:
> > > 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.
> > 
> > That sound rather broken.  Why not save the physical address in
> > find_ibft_region and then later ioremap that when a virtual address is
> > needed like all other code accessing magic I/O memory?
> 
> That is kind of what he does. The physical address is saved as a global
> static variable and also the physical address is memreserved. Then
> later on the physical address is used to create the virtual address.

Except that it uses isa_bus_to_virt, which is really broken.

> Or are you thinking of making the find_ibft_region reserve the physical
> address, and _cache_ the physical address so there is no global
> variable ?

No.  Just switch to ioremap/early_ioremap insted of this isa_bus_to_virt
mess.

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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-08-11  6:57     ` Christoph Hellwig
@ 2021-08-11  7:20       ` Mike Rapoport
  0 siblings, 0 replies; 10+ messages in thread
From: Mike Rapoport @ 2021-08-11  7:20 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Konrad Rzeszutek Wilk, Maurizio Lombardi, bp, tglx, x86, pjones,
	konrad, george.kennedy, rafael, linux-kernel

On Wed, Aug 11, 2021 at 07:57:25AM +0100, Christoph Hellwig wrote:
> On Tue, Aug 10, 2021 at 01:55:11PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Aug 10, 2021 at 06:00:24PM +0100, Christoph Hellwig wrote:
> > > > 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.
> > > 
> > > That sound rather broken.  Why not save the physical address in
> > > find_ibft_region and then later ioremap that when a virtual address is
> > > needed like all other code accessing magic I/O memory?
> > 
> > That is kind of what he does. The physical address is saved as a global
> > static variable and also the physical address is memreserved. Then
> > later on the physical address is used to create the virtual address.
> 
> Except that it uses isa_bus_to_virt, which is really broken.
> 
> > Or are you thinking of making the find_ibft_region reserve the physical
> > address, and _cache_ the physical address so there is no global
> > variable ?
> 
> No.  Just switch to ioremap/early_ioremap insted of this isa_bus_to_virt
> mess.

Why ioremap? This is not IO memory, plain phys_to_virt should be fine here.

-- 
Sincerely yours,
Mike.

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

* Re: [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping
  2021-07-29 13:52 [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping Maurizio Lombardi
                   ` (2 preceding siblings ...)
  2021-08-10 17:00 ` Christoph Hellwig
@ 2021-09-01 16:47 ` Guenter Roeck
  3 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2021-09-01 16:47 UTC (permalink / raw)
  To: Maurizio Lombardi
  Cc: rppt, bp, tglx, x86, pjones, konrad, george.kennedy, rafael,
	linux-kernel

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 <mlombard@redhat.com>

In addition to the x86 build failures, arm64:allmodconfig fails to build as
well.

Building arm64:allmodconfig ... failed
--------------
Error log:
drivers/firmware/iscsi_ibft.c: In function 'ibft_init':
drivers/firmware/iscsi_ibft.c:868:29: error: implicit declaration of function 'isa_bus_to_virt' [-Werror=implicit-function-declaration]
  868 |                 ibft_addr = isa_bus_to_virt(ibft_phys_addr);
      |                             ^~~~~~~~~~~~~~~

ISCSI_IBFT now depends on ISA thanks to this patch.
If that is intentional, it should be declared in Kconfig.

Guenter

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

end of thread, other threads:[~2021-09-01 16:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 13:52 [PATCH V3] iscsi_ibft: fix crash due to KASLR physical memory remapping Maurizio Lombardi
2021-07-29 17:03 ` Konrad Rzeszutek Wilk
2021-07-29 19:20   ` Maurizio Lombardi
2021-07-29 19:31 ` Mike Rapoport
2021-08-01  2:44   ` Konrad Rzeszutek Wilk
2021-08-10 17:00 ` Christoph Hellwig
2021-08-10 17:55   ` Konrad Rzeszutek Wilk
2021-08-11  6:57     ` Christoph Hellwig
2021-08-11  7:20       ` Mike Rapoport
2021-09-01 16:47 ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).