Linux-EFI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] efi/x86: Some mixed mode fixes
@ 2020-02-13 10:20 Ard Biesheuvel
  2020-02-13 10:21 ` [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper Ard Biesheuvel
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-02-13 10:20 UTC (permalink / raw)
  To: linux-efi; +Cc: Ard Biesheuvel, Hans de Goede, Arvind Sankar, Andy Lutomirski

Hans reports that v5.6-rc1 triggers a WARN() when booting in mixed mode,
which appears to be due to the fact that a GUID argument allocated on
the vmap'ed stack and passed by reference to the EFI runtime services
is no longer aligned to its size, which is what the mixed mode runtime
wrappers expect.

Let's fix this in a way that doesn't burden the caller, and copy the
GUID into a suitably aligned buffer in the runtime wrappers. (#1)

Then, patches #2 and #3 are additional cleanup and correctness fixes
for the mixed mode runtime wrappers.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Andy Lutomirski <luto@amacapital.net>

Ard Biesheuvel (3):
  efi/x86: align GUIDs to their size in the mixed mode runtime wrapper
  efi/x86: remove support for EFI time and counter services in mixed
    mode
  efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

 arch/x86/platform/efi/efi_64.c | 151 +++++++-------------
 1 file changed, 52 insertions(+), 99 deletions(-)

-- 
2.17.1


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

* [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper
  2020-02-13 10:20 [PATCH 0/3] efi/x86: Some mixed mode fixes Ard Biesheuvel
@ 2020-02-13 10:21 ` Ard Biesheuvel
  2020-02-14 12:55   ` Hans de Goede
  2020-02-13 10:21 ` [PATCH 2/3] efi/x86: remove support for EFI time and counter services in mixed mode Ard Biesheuvel
  2020-02-13 10:21 ` [PATCH 3/3] efi/x86: Handle by-ref arguments covering multiple pages " Ard Biesheuvel
  2 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2020-02-13 10:21 UTC (permalink / raw)
  To: linux-efi; +Cc: Ard Biesheuvel, Hans de Goede, Arvind Sankar, Andy Lutomirski

The mixed mode runtime wrappers are fragile when it comes to how the
memory referred to by its pointer arguments are laid out in memory,
due to the fact that it translates these addresses to physical addresses
that the runtime services can dereference when running in 1:1 mode.

As a quick (i.e., backportable) fix, copy GUID pointer arguments to
the local stack into a buffer that is naturally aligned to its size,
so that is guaranteed to cover only one physical page.

Note that on x86, we cannot rely on the stack pointer being aligned
the way the compiler expects, so we need to allocate an 8-byte aligned
buffer of sufficient size, and copy the GUID into that buffer at an
offset that is aligned to 16 bytes.

Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/efi_64.c | 25 ++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index fa8506e76bbe..543edfdcd1b9 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -658,6 +658,8 @@ static efi_status_t
 efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor,
 		       u32 *attr, unsigned long *data_size, void *data)
 {
+	u8 buf[24] __aligned(8);
+	efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd));
 	efi_status_t status;
 	u32 phys_name, phys_vendor, phys_attr;
 	u32 phys_data_size, phys_data;
@@ -665,8 +667,10 @@ efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor,
 
 	spin_lock_irqsave(&efi_runtime_lock, flags);
 
+	*vnd = *vendor;
+
 	phys_data_size = virt_to_phys_or_null(data_size);
-	phys_vendor = virt_to_phys_or_null(vendor);
+	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
 	phys_attr = virt_to_phys_or_null(attr);
 	phys_data = virt_to_phys_or_null_size(data, *data_size);
@@ -683,14 +687,18 @@ static efi_status_t
 efi_thunk_set_variable(efi_char16_t *name, efi_guid_t *vendor,
 		       u32 attr, unsigned long data_size, void *data)
 {
+	u8 buf[24] __aligned(8);
+	efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd));
 	u32 phys_name, phys_vendor, phys_data;
 	efi_status_t status;
 	unsigned long flags;
 
 	spin_lock_irqsave(&efi_runtime_lock, flags);
 
+	*vnd = *vendor;
+
 	phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
-	phys_vendor = virt_to_phys_or_null(vendor);
+	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_data = virt_to_phys_or_null_size(data, data_size);
 
 	/* If data_size is > sizeof(u32) we've got problems */
@@ -707,6 +715,8 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 				   u32 attr, unsigned long data_size,
 				   void *data)
 {
+	u8 buf[24] __aligned(8);
+	efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd));
 	u32 phys_name, phys_vendor, phys_data;
 	efi_status_t status;
 	unsigned long flags;
@@ -714,8 +724,10 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
 		return EFI_NOT_READY;
 
+	*vnd = *vendor;
+
 	phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
-	phys_vendor = virt_to_phys_or_null(vendor);
+	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_data = virt_to_phys_or_null_size(data, data_size);
 
 	/* If data_size is > sizeof(u32) we've got problems */
@@ -732,14 +744,18 @@ efi_thunk_get_next_variable(unsigned long *name_size,
 			    efi_char16_t *name,
 			    efi_guid_t *vendor)
 {
+	u8 buf[24] __aligned(8);
+	efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd));
 	efi_status_t status;
 	u32 phys_name_size, phys_name, phys_vendor;
 	unsigned long flags;
 
 	spin_lock_irqsave(&efi_runtime_lock, flags);
 
+	*vnd = *vendor;
+
 	phys_name_size = virt_to_phys_or_null(name_size);
-	phys_vendor = virt_to_phys_or_null(vendor);
+	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_name = virt_to_phys_or_null_size(name, *name_size);
 
 	status = efi_thunk(get_next_variable, phys_name_size,
@@ -747,6 +763,7 @@ efi_thunk_get_next_variable(unsigned long *name_size,
 
 	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 
+	*vendor = *vnd;
 	return status;
 }
 
-- 
2.17.1


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

* [PATCH 2/3] efi/x86: remove support for EFI time and counter services in mixed mode
  2020-02-13 10:20 [PATCH 0/3] efi/x86: Some mixed mode fixes Ard Biesheuvel
  2020-02-13 10:21 ` [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper Ard Biesheuvel
@ 2020-02-13 10:21 ` Ard Biesheuvel
  2020-02-13 10:21 ` [PATCH 3/3] efi/x86: Handle by-ref arguments covering multiple pages " Ard Biesheuvel
  2 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-02-13 10:21 UTC (permalink / raw)
  To: linux-efi; +Cc: Ard Biesheuvel, Hans de Goede, Arvind Sankar, Andy Lutomirski

Mixed mode calls at runtime are rather tricky with vmap'ed stack, as
we can no longer assume that data passed in by the callers of the
EFI runtime wrapper routines is contiguous in physical memory.

We need to fix this, but before we do, let's drop the implementations
of routines that we know are never used on x86, i.e., the RTC related
ones. Given that UEFI rev2.8 permits any runtime service to return
EFI_UNSUPPORTED at runtime, let's return that instead.

As get_next_high_mono_count() is never used at all, even on other
architectures, let's make that return EFI_UNSUPPORTED as well.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/efi_64.c | 81 ++------------------
 1 file changed, 5 insertions(+), 76 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 543edfdcd1b9..ae398587f264 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -568,85 +568,25 @@ efi_thunk_set_virtual_address_map(unsigned long memory_map_size,
 
 static efi_status_t efi_thunk_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
-	efi_status_t status;
-	u32 phys_tm, phys_tc;
-	unsigned long flags;
-
-	spin_lock(&rtc_lock);
-	spin_lock_irqsave(&efi_runtime_lock, flags);
-
-	phys_tm = virt_to_phys_or_null(tm);
-	phys_tc = virt_to_phys_or_null(tc);
-
-	status = efi_thunk(get_time, phys_tm, phys_tc);
-
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
-	spin_unlock(&rtc_lock);
-
-	return status;
+	return EFI_UNSUPPORTED;
 }
 
 static efi_status_t efi_thunk_set_time(efi_time_t *tm)
 {
-	efi_status_t status;
-	u32 phys_tm;
-	unsigned long flags;
-
-	spin_lock(&rtc_lock);
-	spin_lock_irqsave(&efi_runtime_lock, flags);
-
-	phys_tm = virt_to_phys_or_null(tm);
-
-	status = efi_thunk(set_time, phys_tm);
-
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
-	spin_unlock(&rtc_lock);
-
-	return status;
+	return EFI_UNSUPPORTED;
 }
 
 static efi_status_t
 efi_thunk_get_wakeup_time(efi_bool_t *enabled, efi_bool_t *pending,
 			  efi_time_t *tm)
 {
-	efi_status_t status;
-	u32 phys_enabled, phys_pending, phys_tm;
-	unsigned long flags;
-
-	spin_lock(&rtc_lock);
-	spin_lock_irqsave(&efi_runtime_lock, flags);
-
-	phys_enabled = virt_to_phys_or_null(enabled);
-	phys_pending = virt_to_phys_or_null(pending);
-	phys_tm = virt_to_phys_or_null(tm);
-
-	status = efi_thunk(get_wakeup_time, phys_enabled,
-			     phys_pending, phys_tm);
-
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
-	spin_unlock(&rtc_lock);
-
-	return status;
+	return EFI_UNSUPPORTED;
 }
 
 static efi_status_t
 efi_thunk_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 {
-	efi_status_t status;
-	u32 phys_tm;
-	unsigned long flags;
-
-	spin_lock(&rtc_lock);
-	spin_lock_irqsave(&efi_runtime_lock, flags);
-
-	phys_tm = virt_to_phys_or_null(tm);
-
-	status = efi_thunk(set_wakeup_time, enabled, phys_tm);
-
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
-	spin_unlock(&rtc_lock);
-
-	return status;
+	return EFI_UNSUPPORTED;
 }
 
 static unsigned long efi_name_size(efi_char16_t *name)
@@ -770,18 +710,7 @@ efi_thunk_get_next_variable(unsigned long *name_size,
 static efi_status_t
 efi_thunk_get_next_high_mono_count(u32 *count)
 {
-	efi_status_t status;
-	u32 phys_count;
-	unsigned long flags;
-
-	spin_lock_irqsave(&efi_runtime_lock, flags);
-
-	phys_count = virt_to_phys_or_null(count);
-	status = efi_thunk(get_next_high_mono_count, phys_count);
-
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
-
-	return status;
+	return EFI_UNSUPPORTED;
 }
 
 static void
-- 
2.17.1


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

* [PATCH 3/3] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode
  2020-02-13 10:20 [PATCH 0/3] efi/x86: Some mixed mode fixes Ard Biesheuvel
  2020-02-13 10:21 ` [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper Ard Biesheuvel
  2020-02-13 10:21 ` [PATCH 2/3] efi/x86: remove support for EFI time and counter services in mixed mode Ard Biesheuvel
@ 2020-02-13 10:21 ` " Ard Biesheuvel
  2 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-02-13 10:21 UTC (permalink / raw)
  To: linux-efi; +Cc: Ard Biesheuvel, Hans de Goede, Arvind Sankar, Andy Lutomirski

We currently apply some stringent checks on the variable name and
buffer arguments passed to the EFI runtime services, and WARN()
if they are allocated in the vmalloc space and are not aligned to
their size - which must be a power of two - since in that case, it
is not guaranteed that they will not cross a page boundary.

However, we then simply proceed with the call, which could cause data
corruption if the next physical page belongs to a mapping that is
entirely unrelated.

Given that with vmap'ed stacks, this condition is much more likely
to trigger, let's relax the condition a bit, but fail the runtime
service call if it triggers.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/efi_64.c | 45 +++++++++++---------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index ae398587f264..d19a2edd63cb 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -180,7 +180,7 @@ void efi_sync_low_kernel_mappings(void)
 static inline phys_addr_t
 virt_to_phys_or_null_size(void *va, unsigned long size)
 {
-	bool bad_size;
+	phys_addr_t pa;
 
 	if (!va)
 		return 0;
@@ -188,16 +188,13 @@ virt_to_phys_or_null_size(void *va, unsigned long size)
 	if (virt_addr_valid(va))
 		return virt_to_phys(va);
 
-	/*
-	 * A fully aligned variable on the stack is guaranteed not to
-	 * cross a page bounary. Try to catch strings on the stack by
-	 * checking that 'size' is a power of two.
-	 */
-	bad_size = size > PAGE_SIZE || !is_power_of_2(size);
+	pa = slow_virt_to_phys(va);
 
-	WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size);
+	/* check if the object crosses a page boundary */
+	if (WARN_ON((pa ^ (pa + size - 1)) & PAGE_MASK))
+		return 0;
 
-	return slow_virt_to_phys(va);
+	return pa;
 }
 
 #define virt_to_phys_or_null(addr)				\
@@ -615,8 +612,11 @@ efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor,
 	phys_attr = virt_to_phys_or_null(attr);
 	phys_data = virt_to_phys_or_null_size(data, *data_size);
 
-	status = efi_thunk(get_variable, phys_name, phys_vendor,
-			   phys_attr, phys_data_size, phys_data);
+	if (!phys_name || (data && !phys_data))
+		status = EFI_INVALID_PARAMETER;
+	else
+		status = efi_thunk(get_variable, phys_name, phys_vendor,
+				   phys_attr, phys_data_size, phys_data);
 
 	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 
@@ -641,9 +641,11 @@ efi_thunk_set_variable(efi_char16_t *name, efi_guid_t *vendor,
 	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_data = virt_to_phys_or_null_size(data, data_size);
 
-	/* If data_size is > sizeof(u32) we've got problems */
-	status = efi_thunk(set_variable, phys_name, phys_vendor,
-			   attr, data_size, phys_data);
+	if (!phys_name || !phys_data)
+		status = EFI_INVALID_PARAMETER;
+	else
+		status = efi_thunk(set_variable, phys_name, phys_vendor,
+				   attr, data_size, phys_data);
 
 	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 
@@ -670,9 +672,11 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_data = virt_to_phys_or_null_size(data, data_size);
 
-	/* If data_size is > sizeof(u32) we've got problems */
-	status = efi_thunk(set_variable, phys_name, phys_vendor,
-			   attr, data_size, phys_data);
+	if (!phys_name || !phys_data)
+		status = EFI_INVALID_PARAMETER;
+	else
+		status = efi_thunk(set_variable, phys_name, phys_vendor,
+				   attr, data_size, phys_data);
 
 	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 
@@ -698,8 +702,11 @@ efi_thunk_get_next_variable(unsigned long *name_size,
 	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_name = virt_to_phys_or_null_size(name, *name_size);
 
-	status = efi_thunk(get_next_variable, phys_name_size,
-			   phys_name, phys_vendor);
+	if (!phys_name)
+		status = EFI_INVALID_PARAMETER;
+	else
+		status = efi_thunk(get_next_variable, phys_name_size,
+				   phys_name, phys_vendor);
 
 	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 
-- 
2.17.1


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

* Re: [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper
  2020-02-13 10:21 ` [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper Ard Biesheuvel
@ 2020-02-14 12:55   ` Hans de Goede
  2020-02-14 13:35     ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2020-02-14 12:55 UTC (permalink / raw)
  To: Ard Biesheuvel, linux-efi; +Cc: Arvind Sankar, Andy Lutomirski

Hi,

On 2/13/20 11:21 AM, Ard Biesheuvel wrote:
> The mixed mode runtime wrappers are fragile when it comes to how the
> memory referred to by its pointer arguments are laid out in memory,
> due to the fact that it translates these addresses to physical addresses
> that the runtime services can dereference when running in 1:1 mode.
> 
> As a quick (i.e., backportable) fix, copy GUID pointer arguments to
> the local stack into a buffer that is naturally aligned to its size,
> so that is guaranteed to cover only one physical page.
> 
> Note that on x86, we cannot rely on the stack pointer being aligned
> the way the compiler expects, so we need to allocate an 8-byte aligned
> buffer of sufficient size, and copy the GUID into that buffer at an
> offset that is aligned to 16 bytes.
> 
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

I can confirm that this fixes the WARN_ON triggering I was seeing,
thanks:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>   arch/x86/platform/efi/efi_64.c | 25 ++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index fa8506e76bbe..543edfdcd1b9 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -658,6 +658,8 @@ static efi_status_t
>   efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor,
>   		       u32 *attr, unsigned long *data_size, void *data)
>   {
> +	u8 buf[24] __aligned(8);
> +	efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd));
>   	efi_status_t status;
>   	u32 phys_name, phys_vendor, phys_attr;
>   	u32 phys_data_size, phys_data;
> @@ -665,8 +667,10 @@ efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor,
>   
>   	spin_lock_irqsave(&efi_runtime_lock, flags);
>   
> +	*vnd = *vendor;
> +
>   	phys_data_size = virt_to_phys_or_null(data_size);
> -	phys_vendor = virt_to_phys_or_null(vendor);
> +	phys_vendor = virt_to_phys_or_null(vnd);
>   	phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
>   	phys_attr = virt_to_phys_or_null(attr);
>   	phys_data = virt_to_phys_or_null_size(data, *data_size);
> @@ -683,14 +687,18 @@ static efi_status_t
>   efi_thunk_set_variable(efi_char16_t *name, efi_guid_t *vendor,
>   		       u32 attr, unsigned long data_size, void *data)
>   {
> +	u8 buf[24] __aligned(8);
> +	efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd));
>   	u32 phys_name, phys_vendor, phys_data;
>   	efi_status_t status;
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&efi_runtime_lock, flags);
>   
> +	*vnd = *vendor;
> +
>   	phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
> -	phys_vendor = virt_to_phys_or_null(vendor);
> +	phys_vendor = virt_to_phys_or_null(vnd);
>   	phys_data = virt_to_phys_or_null_size(data, data_size);
>   
>   	/* If data_size is > sizeof(u32) we've got problems */
> @@ -707,6 +715,8 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
>   				   u32 attr, unsigned long data_size,
>   				   void *data)
>   {
> +	u8 buf[24] __aligned(8);
> +	efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd));
>   	u32 phys_name, phys_vendor, phys_data;
>   	efi_status_t status;
>   	unsigned long flags;
> @@ -714,8 +724,10 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
>   	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
>   		return EFI_NOT_READY;
>   
> +	*vnd = *vendor;
> +
>   	phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
> -	phys_vendor = virt_to_phys_or_null(vendor);
> +	phys_vendor = virt_to_phys_or_null(vnd);
>   	phys_data = virt_to_phys_or_null_size(data, data_size);
>   
>   	/* If data_size is > sizeof(u32) we've got problems */
> @@ -732,14 +744,18 @@ efi_thunk_get_next_variable(unsigned long *name_size,
>   			    efi_char16_t *name,
>   			    efi_guid_t *vendor)
>   {
> +	u8 buf[24] __aligned(8);
> +	efi_guid_t *vnd = PTR_ALIGN((efi_guid_t *)buf, sizeof(*vnd));
>   	efi_status_t status;
>   	u32 phys_name_size, phys_name, phys_vendor;
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&efi_runtime_lock, flags);
>   
> +	*vnd = *vendor;
> +
>   	phys_name_size = virt_to_phys_or_null(name_size);
> -	phys_vendor = virt_to_phys_or_null(vendor);
> +	phys_vendor = virt_to_phys_or_null(vnd);
>   	phys_name = virt_to_phys_or_null_size(name, *name_size);
>   
>   	status = efi_thunk(get_next_variable, phys_name_size,
> @@ -747,6 +763,7 @@ efi_thunk_get_next_variable(unsigned long *name_size,
>   
>   	spin_unlock_irqrestore(&efi_runtime_lock, flags);
>   
> +	*vendor = *vnd;
>   	return status;
>   }
>   
> 


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

* Re: [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper
  2020-02-14 12:55   ` Hans de Goede
@ 2020-02-14 13:35     ` Ard Biesheuvel
  0 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-02-14 13:35 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-efi, Arvind Sankar, Andy Lutomirski

On Fri, 14 Feb 2020 at 13:55, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 2/13/20 11:21 AM, Ard Biesheuvel wrote:
> > The mixed mode runtime wrappers are fragile when it comes to how the
> > memory referred to by its pointer arguments are laid out in memory,
> > due to the fact that it translates these addresses to physical addresses
> > that the runtime services can dereference when running in 1:1 mode.
> >
> > As a quick (i.e., backportable) fix, copy GUID pointer arguments to
> > the local stack into a buffer that is naturally aligned to its size,
> > so that is guaranteed to cover only one physical page.
> >
> > Note that on x86, we cannot rely on the stack pointer being aligned
> > the way the compiler expects, so we need to allocate an 8-byte aligned
> > buffer of sufficient size, and copy the GUID into that buffer at an
> > offset that is aligned to 16 bytes.
> >
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> I can confirm that this fixes the WARN_ON triggering I was seeing,
> thanks:
>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
>

Thanks Hans

I'll let these soak a bit in efi/urgent before I send them to tip.

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

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 10:20 [PATCH 0/3] efi/x86: Some mixed mode fixes Ard Biesheuvel
2020-02-13 10:21 ` [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper Ard Biesheuvel
2020-02-14 12:55   ` Hans de Goede
2020-02-14 13:35     ` Ard Biesheuvel
2020-02-13 10:21 ` [PATCH 2/3] efi/x86: remove support for EFI time and counter services in mixed mode Ard Biesheuvel
2020-02-13 10:21 ` [PATCH 3/3] efi/x86: Handle by-ref arguments covering multiple pages " Ard Biesheuvel

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