[PATCH 4.19 83/86] efi/x86: Align GUIDs to their size in the mixed mode runtime wrapper

[PATCH 4.19 83/86] efi/x86: Align GUIDs to their size in the mixed mode runtime wrapper

[Greg Kroah-Hartman]

From: Ard Biesheuvel <ardb@kernel.org>

commit 63056e8b5ebf41d52170e9f5ba1fc83d1855278c upstream.

Hans reports that his mixed mode systems running v5.6-rc1 kernels hit
the WARN_ON() in virt_to_phys_or_null_size(), caused by the fact that
efi_guid_t objects on the vmap'ed stack happen to be misaligned with
respect to their sizes. 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 it 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.

Fixes: f6697df36bdf0bf7 ("x86/efi: Prevent mixed mode boot corruption with CONFIG_VMAP_STACK=y")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Cc: linux-efi@vger.kernel.org
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20200221084849.26878-2-ardb@kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 arch/x86/platform/efi/efi_64.c |   25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -790,6 +790,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;
@@ -797,8 +799,10 @@ efi_thunk_get_variable(efi_char16_t *nam
 
 	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);
@@ -815,14 +819,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 */
@@ -839,6 +847,8 @@ efi_thunk_set_variable_nonblocking(efi_c
 				   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;
@@ -846,8 +856,10 @@ efi_thunk_set_variable_nonblocking(efi_c
 	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 */
@@ -864,14 +876,18 @@ efi_thunk_get_next_variable(unsigned lon
 			    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,
@@ -879,6 +895,7 @@ efi_thunk_get_next_variable(unsigned lon
 
 	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 
+	*vendor = *vnd;
 	return status;
 }
 

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

[Greg Kroah-Hartman]

From: Ard Biesheuvel <ardb@kernel.org>

commit 8319e9d5ad98ffccd19f35664382c73cea216193 upstream.

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. Since
vmalloc'ed pages (including the vmap'ed stack) are not contiguous in the
physical address space, this scheme only works if the referenced memory
objects do not cross page boundaries.

Currently, the mixed mode runtime service wrappers require that all by-ref
arguments that live in the vmalloc space have a size that is a power of 2,
and are aligned to that same value. While this is a sensible way to
construct an object that is guaranteed not to cross a page boundary, it is
overly strict when it comes to checking whether a given object violates
this requirement, as we can simply take the physical address of the first
and the last byte, and verify that they point into the same physical page.

When this check fails, we emit a WARN(), but 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 does trigger.

Fixes: f6697df36bdf0bf7 ("x86/efi: Prevent mixed mode boot corruption with CONFIG_VMAP_STACK=y")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: linux-efi@vger.kernel.org
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20200221084849.26878-4-ardb@kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 arch/x86/platform/efi/efi_64.c |   45 +++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -313,7 +313,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;
@@ -321,16 +321,13 @@ virt_to_phys_or_null_size(void *va, unsi
 	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)				\
@@ -807,8 +804,11 @@ efi_thunk_get_variable(efi_char16_t *nam
 	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);
 
@@ -833,9 +833,11 @@ efi_thunk_set_variable(efi_char16_t *nam
 	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);
 
@@ -862,9 +864,11 @@ efi_thunk_set_variable_nonblocking(efi_c
 	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);
 
@@ -890,8 +894,11 @@ efi_thunk_get_next_variable(unsigned lon
 	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);
 

Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]

Hi!

> Currently, the mixed mode runtime service wrappers require that all by-ref
> arguments that live in the vmalloc space have a size that is a power of 2,
> and are aligned to that same value. While this is a sensible way to
> construct an object that is guaranteed not to cross a page boundary, it is
> overly strict when it comes to checking whether a given object violates
> this requirement, as we can simply take the physical address of the first
> and the last byte, and verify that they point into the same physical
> page.

Dunno. If start passing buffers that _sometime_ cross page boundaries,
we'll get hard to debug failures. Maybe original code is better
buecause it catches problems earlier?

Furthermore, all existing code should pass aligned, 2^n size buffers,
so we should not need this in stable?

> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -321,16 +321,13 @@ virt_to_phys_or_null_size(void *va, unsi
>  	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;

We don't really need to do this computation on pa, it would work on va
as well, right? It does not matter much, but old code worked that way.

Plus, strictly speaking, pa + size can overflow for huge sizes, and
test will return false negatives.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

[Greg Kroah-Hartman]

On Wed, Mar 11, 2020 at 02:01:07PM +0100, Pavel Machek wrote:
> Hi!
> 
> > Currently, the mixed mode runtime service wrappers require that all by-ref
> > arguments that live in the vmalloc space have a size that is a power of 2,
> > and are aligned to that same value. While this is a sensible way to
> > construct an object that is guaranteed not to cross a page boundary, it is
> > overly strict when it comes to checking whether a given object violates
> > this requirement, as we can simply take the physical address of the first
> > and the last byte, and verify that they point into the same physical
> > page.
> 
> Dunno. If start passing buffers that _sometime_ cross page boundaries,
> we'll get hard to debug failures. Maybe original code is better
> buecause it catches problems earlier?
> 
> Furthermore, all existing code should pass aligned, 2^n size buffers,
> so we should not need this in stable?

For some crazy reason you cut out the reason I applied this patch to the
stable tree.  From the changelog text:
	Fixes: f6697df36bdf0bf7 ("x86/efi: Prevent mixed mode boot corruption with CONFIG_VMAP_STACK=y")

Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1825 bytes --]

On Wed 2020-03-11 14:13:11, Greg Kroah-Hartman wrote:
> On Wed, Mar 11, 2020 at 02:01:07PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > Currently, the mixed mode runtime service wrappers require that all by-ref
> > > arguments that live in the vmalloc space have a size that is a power of 2,
> > > and are aligned to that same value. While this is a sensible way to
> > > construct an object that is guaranteed not to cross a page boundary, it is
> > > overly strict when it comes to checking whether a given object violates
> > > this requirement, as we can simply take the physical address of the first
> > > and the last byte, and verify that they point into the same physical
> > > page.
> > 
> > Dunno. If start passing buffers that _sometime_ cross page boundaries,
> > we'll get hard to debug failures. Maybe original code is better
> > buecause it catches problems earlier?
> > 
> > Furthermore, all existing code should pass aligned, 2^n size buffers,
> > so we should not need this in stable?
> 
> For some crazy reason you cut out the reason I applied this patch to the
> stable tree.  From the changelog text:
> 	Fixes: f6697df36bdf0bf7 ("x86/efi: Prevent mixed mode boot
>corruption with CONFIG_VMAP_STACK=y")

I did not notice that, but reviewing f669 does not really help. If
there is some known code that passes unaligned (but guaranteed
not-to-cross-page) buffers here, then yes, but is it? Having
not-page-crossing guarantees is kind of hard without alignment.

People seem to be adding Fixes: tags even if it is not a bugfix, just
as reminder that this has relation to some other commit...

Best regards,
								Pavel


-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

[Ard Biesheuvel]

On Wed, 11 Mar 2020 at 09:28, Pavel Machek <pavel@denx.de> wrote:
>
> On Wed 2020-03-11 14:13:11, Greg Kroah-Hartman wrote:
> > On Wed, Mar 11, 2020 at 02:01:07PM +0100, Pavel Machek wrote:
> > > Hi!
> > >
> > > > Currently, the mixed mode runtime service wrappers require that all by-ref
> > > > arguments that live in the vmalloc space have a size that is a power of 2,
> > > > and are aligned to that same value. While this is a sensible way to
> > > > construct an object that is guaranteed not to cross a page boundary, it is
> > > > overly strict when it comes to checking whether a given object violates
> > > > this requirement, as we can simply take the physical address of the first
> > > > and the last byte, and verify that they point into the same physical
> > > > page.
> > >
> > > Dunno. If start passing buffers that _sometime_ cross page boundaries,
> > > we'll get hard to debug failures. Maybe original code is better
> > > buecause it catches problems earlier?
> > >
> > > Furthermore, all existing code should pass aligned, 2^n size buffers,
> > > so we should not need this in stable?
> >
> > For some crazy reason you cut out the reason I applied this patch to the
> > stable tree.  From the changelog text:
> >       Fixes: f6697df36bdf0bf7 ("x86/efi: Prevent mixed mode boot
> >corruption with CONFIG_VMAP_STACK=y")
>
> I did not notice that, but reviewing f669 does not really help. If
> there is some known code that passes unaligned (but guaranteed
> not-to-cross-page) buffers here, then yes, but is it? Having
> not-page-crossing guarantees is kind of hard without alignment.
>
> People seem to be adding Fixes: tags even if it is not a bugfix, just
> as reminder that this has relation to some other commit...
>

If you read the commit log until the end, you will find a paragraph
that describes how the old code warns about, but then ignores the
error condition, and proceeds in a way that may cause data corruption.
Also, on x86, the stack alignment is only 8 bytes, so with the
introduction of vmapped stacks, most guarantees about alignment of
objects in the vmalloc space went out the window, potentially
triggering this condition in unanticipated ways.

It is not very constructive to comment on 'what people seem to be
doing' if you have no clue what the context of the change is. Opinions
are welcome but informed ones are preferred.


>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

[Arvind Sankar]

On Wed, Mar 11, 2020 at 02:01:07PM +0100, Pavel Machek wrote:
> We don't really need to do this computation on pa, it would work on va
> as well, right? It does not matter much, but old code worked that way.
> 
> Plus, strictly speaking, pa + size can overflow for huge sizes, and
> test will return false negatives.

This is 64-bit code, overflow would need pa + size to be bigger than
2^64, and even then a false negative would need size to be around 2^64.