[PATCH 0/3] efi/x86: Some mixed mode fixes

[PATCH 0/3] efi/x86: Some mixed mode fixes

[Ard Biesheuvel]

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

[PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper

[Ard Biesheuvel]

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

Re: [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper

[Hans de Goede]

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;
>   }
>   
> 

Re: [PATCH 1/3] efi/x86: align GUIDs to their size in the mixed mode runtime wrapper

[Ard Biesheuvel]

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.

[PATCH 2/3] efi/x86: remove support for EFI time and counter services in mixed mode

[Ard Biesheuvel]

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

[PATCH 3/3] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

[Ard Biesheuvel]

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