* [PATCH 4.19 83/86] efi/x86: Align GUIDs to their size in the mixed mode runtime wrapper [not found] <20200310124530.808338541@linuxfoundation.org> @ 2020-03-10 12:45 ` Greg Kroah-Hartman 2020-03-10 12:45 ` [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode Greg Kroah-Hartman 1 sibling, 0 replies; 7+ messages in thread From: Greg Kroah-Hartman @ 2020-03-10 12:45 UTC (permalink / raw) To: linux-kernel Cc: Greg Kroah-Hartman, stable, Hans de Goede, Ard Biesheuvel, Ingo Molnar, linux-efi, Thomas Gleixner 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode [not found] <20200310124530.808338541@linuxfoundation.org> 2020-03-10 12:45 ` [PATCH 4.19 83/86] efi/x86: Align GUIDs to their size in the mixed mode runtime wrapper Greg Kroah-Hartman @ 2020-03-10 12:45 ` Greg Kroah-Hartman 2020-03-11 13:01 ` Pavel Machek 1 sibling, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2020-03-10 12:45 UTC (permalink / raw) To: linux-kernel Cc: Greg Kroah-Hartman, stable, Ard Biesheuvel, Ingo Molnar, linux-efi, Thomas Gleixner 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode 2020-03-10 12:45 ` [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode Greg Kroah-Hartman @ 2020-03-11 13:01 ` Pavel Machek 2020-03-11 13:13 ` Greg Kroah-Hartman 2020-03-12 3:52 ` Arvind Sankar 0 siblings, 2 replies; 7+ messages in thread From: Pavel Machek @ 2020-03-11 13:01 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-kernel, stable, Ard Biesheuvel, Ingo Molnar, linux-efi, Thomas Gleixner [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode 2020-03-11 13:01 ` Pavel Machek @ 2020-03-11 13:13 ` Greg Kroah-Hartman 2020-03-11 13:28 ` Pavel Machek 2020-03-12 3:52 ` Arvind Sankar 1 sibling, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2020-03-11 13:13 UTC (permalink / raw) To: Pavel Machek Cc: linux-kernel, stable, Ard Biesheuvel, Ingo Molnar, linux-efi, Thomas Gleixner 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") ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode 2020-03-11 13:13 ` Greg Kroah-Hartman @ 2020-03-11 13:28 ` Pavel Machek 2020-03-11 13:43 ` Ard Biesheuvel 0 siblings, 1 reply; 7+ messages in thread From: Pavel Machek @ 2020-03-11 13:28 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Pavel Machek, linux-kernel, stable, Ard Biesheuvel, Ingo Molnar, linux-efi, Thomas Gleixner [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode 2020-03-11 13:28 ` Pavel Machek @ 2020-03-11 13:43 ` Ard Biesheuvel 0 siblings, 0 replies; 7+ messages in thread From: Ard Biesheuvel @ 2020-03-11 13:43 UTC (permalink / raw) To: Pavel Machek Cc: Greg Kroah-Hartman, Linux Kernel Mailing List, stable, Ingo Molnar, linux-efi, Thomas Gleixner 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode 2020-03-11 13:01 ` Pavel Machek 2020-03-11 13:13 ` Greg Kroah-Hartman @ 2020-03-12 3:52 ` Arvind Sankar 1 sibling, 0 replies; 7+ messages in thread From: Arvind Sankar @ 2020-03-12 3:52 UTC (permalink / raw) To: Pavel Machek Cc: Greg Kroah-Hartman, linux-kernel, stable, Ard Biesheuvel, Ingo Molnar, linux-efi, Thomas Gleixner 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-12 3:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20200310124530.808338541@linuxfoundation.org> 2020-03-10 12:45 ` [PATCH 4.19 83/86] efi/x86: Align GUIDs to their size in the mixed mode runtime wrapper Greg Kroah-Hartman 2020-03-10 12:45 ` [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode Greg Kroah-Hartman 2020-03-11 13:01 ` Pavel Machek 2020-03-11 13:13 ` Greg Kroah-Hartman 2020-03-11 13:28 ` Pavel Machek 2020-03-11 13:43 ` Ard Biesheuvel 2020-03-12 3:52 ` Arvind Sankar
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).