[12/17] efi: fix efi_pci_io_protocol32 prototype for mixed mode

Message ID 20180504060003.19618-13-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • [01/17] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot
Related show

Commit Message

Ard Biesheuvel May 4, 2018, 5:59 a.m.
Mixed mode allows a kernel built for x86_64 to interact with 32-bit
EFI firmware, but requires us to define all struct definitions carefully
when it comes to pointer sizes. efi_pci_io_protocol32 currently uses a
void* for the 'romimage' field, which will be interpreted as a 64-bit
field on such kernels, potentially resulting in bogus memory references
and subsequent crashes.

Cc: <stable@vger.kernel.org>
Tested-by: Hans de Goede <hdegoede@redhat.com>

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 arch/x86/boot/compressed/eboot.c | 6 ++++--
 include/linux/efi.h              | 8 ++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ingo Molnar May 14, 2018, 6:57 a.m. | #1
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> Mixed mode allows a kernel built for x86_64 to interact with 32-bit

> EFI firmware, but requires us to define all struct definitions carefully

> when it comes to pointer sizes. efi_pci_io_protocol32 currently uses a

> void* for the 'romimage' field, which will be interpreted as a 64-bit

> field on such kernels, potentially resulting in bogus memory references

> and subsequent crashes.


Yeah, so the first confusion I ran into is:

 s/efi_pci_io_protocol32
  /efi_pci_io_protocol_32

Once I found it in the code I made this change:

 s/efi: fix efi_pci_io_protocol32 prototype for mixed mode
  /efi: Fix 'struct efi_pci_io_protocol32' definition for mixed mode

Because we normally use the 'prototype' name for function declarations, not for 
data type definitions. Adding 'struct' and putting it between quotes makes it 
obvious at a glance that we are talking about a structure definition here.

BTW., since it's marked -stable, due to:

 > potentially resulting in bogus memory references

 > and subsequent crashes.


I'm moving it to efi/urgent: the principle here is that if a patch is urgent 
enough for -stable then it should generally not wait for the next merge window.

Also, because this actually fixes a crash, I extended the title to spell this out 
more clearly:

  Subject: efi: Avoid potential crashes, fix the 'struct efi_pci_io_protocol_32' definition for mixed mode

... which also makes it easier for maintainers of older stable kernels to decide 
whether to backport the patch or not.

Anyway, the patch is looking good otherwise, no need to resend.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 47d3efff6805..09f36c0d9d4f 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -163,7 +163,8 @@  __setup_efi_pci32(efi_pci_io_protocol_32 *pci, struct pci_setup_rom **__rom)
 	if (status != EFI_SUCCESS)
 		goto free_struct;
 
-	memcpy(rom->romdata, pci->romimage, pci->romsize);
+	memcpy(rom->romdata, (void *)(unsigned long)pci->romimage,
+	       pci->romsize);
 	return status;
 
 free_struct:
@@ -269,7 +270,8 @@  __setup_efi_pci64(efi_pci_io_protocol_64 *pci, struct pci_setup_rom **__rom)
 	if (status != EFI_SUCCESS)
 		goto free_struct;
 
-	memcpy(rom->romdata, pci->romimage, pci->romsize);
+	memcpy(rom->romdata, (void *)(unsigned long)pci->romimage,
+	       pci->romsize);
 	return status;
 
 free_struct:
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f1b7d68ac460..3016d8c456bc 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -395,8 +395,8 @@  typedef struct {
 	u32 attributes;
 	u32 get_bar_attributes;
 	u32 set_bar_attributes;
-	uint64_t romsize;
-	void *romimage;
+	u64 romsize;
+	u32 romimage;
 } efi_pci_io_protocol_32;
 
 typedef struct {
@@ -415,8 +415,8 @@  typedef struct {
 	u64 attributes;
 	u64 get_bar_attributes;
 	u64 set_bar_attributes;
-	uint64_t romsize;
-	void *romimage;
+	u64 romsize;
+	u64 romimage;
 } efi_pci_io_protocol_64;
 
 typedef struct {