* [PATCH] x86/boot/compressed/64: Fix boot on machines with broken E820 table
@ 2019-08-13 13:16 Kirill A. Shutemov
2019-08-19 14:16 ` [tip:x86/urgent] " tip-bot for Kirill A. Shutemov
0 siblings, 1 reply; 5+ messages in thread
From: Kirill A. Shutemov @ 2019-08-13 13:16 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin
Cc: x86, linux-kernel, Kirill A. Shutemov
BIOS on Samsung 500C Chromebook reports very rudimentary E820 table that
consists of 2 entries:
BIOS-e820: [mem 0x0000000000000000-0x0000000000000fff] usable
BIOS-e820: [mem 0x00000000fffff000-0x00000000ffffffff] reserved
It breaks logic in find_trampoline_placement(): bios_start lands on the
end of the first 4k page and trampoline start gets placed below 0.
Detect underflow and don't touch bios_start for such cases. It makes
kernel ignore E820 table on machines that doesn't have two usable pages
below BIOS_START_MAX.
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: 1b3a62643660 ("x86/boot/compressed/64: Validate trampoline placement against E820")
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203463
---
arch/x86/boot/compressed/pgtable_64.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
index 5f2d03067ae5..2faddeb0398a 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -72,6 +72,8 @@ static unsigned long find_trampoline_placement(void)
/* Find the first usable memory region under bios_start. */
for (i = boot_params->e820_entries - 1; i >= 0; i--) {
+ unsigned long new;
+
entry = &boot_params->e820_table[i];
/* Skip all entries above bios_start. */
@@ -84,15 +86,20 @@ static unsigned long find_trampoline_placement(void)
/* Adjust bios_start to the end of the entry if needed. */
if (bios_start > entry->addr + entry->size)
- bios_start = entry->addr + entry->size;
+ new = entry->addr + entry->size;
/* Keep bios_start page-aligned. */
- bios_start = round_down(bios_start, PAGE_SIZE);
+ new = round_down(new, PAGE_SIZE);
/* Skip the entry if it's too small. */
- if (bios_start - TRAMPOLINE_32BIT_SIZE < entry->addr)
+ if (new - TRAMPOLINE_32BIT_SIZE < entry->addr)
continue;
+ /* Protect against underflow. */
+ if (new - TRAMPOLINE_32BIT_SIZE > bios_start)
+ break;
+
+ bios_start = new;
break;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [tip:x86/urgent] x86/boot/compressed/64: Fix boot on machines with broken E820 table 2019-08-13 13:16 [PATCH] x86/boot/compressed/64: Fix boot on machines with broken E820 table Kirill A. Shutemov @ 2019-08-19 14:16 ` tip-bot for Kirill A. Shutemov 2019-08-26 3:33 ` Gustavo A. R. Silva 0 siblings, 1 reply; 5+ messages in thread From: tip-bot for Kirill A. Shutemov @ 2019-08-19 14:16 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, kirill.shutemov, x86, mingo, linux-kernel, bp, hpa, tglx, kirill Commit-ID: 0a46fff2f9108c2c44218380a43a736cf4612541 Gitweb: https://git.kernel.org/tip/0a46fff2f9108c2c44218380a43a736cf4612541 Author: Kirill A. Shutemov <kirill@shutemov.name> AuthorDate: Tue, 13 Aug 2019 16:16:54 +0300 Committer: Borislav Petkov <bp@suse.de> CommitDate: Mon, 19 Aug 2019 15:59:13 +0200 x86/boot/compressed/64: Fix boot on machines with broken E820 table BIOS on Samsung 500C Chromebook reports very rudimentary E820 table that consists of 2 entries: BIOS-e820: [mem 0x0000000000000000-0x0000000000000fff] usable BIOS-e820: [mem 0x00000000fffff000-0x00000000ffffffff] reserved It breaks logic in find_trampoline_placement(): bios_start lands on the end of the first 4k page and trampoline start gets placed below 0. Detect underflow and don't touch bios_start for such cases. It makes kernel ignore E820 table on machines that doesn't have two usable pages below BIOS_START_MAX. Fixes: 1b3a62643660 ("x86/boot/compressed/64: Validate trampoline placement against E820") Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Signed-off-by: Borislav Petkov <bp@suse.de> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Ingo Molnar <mingo@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: x86-ml <x86@kernel.org> Link: https://bugzilla.kernel.org/show_bug.cgi?id=203463 Link: https://lkml.kernel.org/r/20190813131654.24378-1-kirill.shutemov@linux.intel.com --- arch/x86/boot/compressed/pgtable_64.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c index 5f2d03067ae5..2faddeb0398a 100644 --- a/arch/x86/boot/compressed/pgtable_64.c +++ b/arch/x86/boot/compressed/pgtable_64.c @@ -72,6 +72,8 @@ static unsigned long find_trampoline_placement(void) /* Find the first usable memory region under bios_start. */ for (i = boot_params->e820_entries - 1; i >= 0; i--) { + unsigned long new; + entry = &boot_params->e820_table[i]; /* Skip all entries above bios_start. */ @@ -84,15 +86,20 @@ static unsigned long find_trampoline_placement(void) /* Adjust bios_start to the end of the entry if needed. */ if (bios_start > entry->addr + entry->size) - bios_start = entry->addr + entry->size; + new = entry->addr + entry->size; /* Keep bios_start page-aligned. */ - bios_start = round_down(bios_start, PAGE_SIZE); + new = round_down(new, PAGE_SIZE); /* Skip the entry if it's too small. */ - if (bios_start - TRAMPOLINE_32BIT_SIZE < entry->addr) + if (new - TRAMPOLINE_32BIT_SIZE < entry->addr) continue; + /* Protect against underflow. */ + if (new - TRAMPOLINE_32BIT_SIZE > bios_start) + break; + + bios_start = new; break; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [tip:x86/urgent] x86/boot/compressed/64: Fix boot on machines with broken E820 table 2019-08-19 14:16 ` [tip:x86/urgent] " tip-bot for Kirill A. Shutemov @ 2019-08-26 3:33 ` Gustavo A. R. Silva 2019-08-26 7:15 ` Borislav Petkov 0 siblings, 1 reply; 5+ messages in thread From: Gustavo A. R. Silva @ 2019-08-26 3:33 UTC (permalink / raw) To: tglx, bp, hpa, linux-kernel, mingo, x86, mingo, kirill.shutemov, kirill, linux-tip-commits Hi all, On 8/19/19 9:16 AM, tip-bot for Kirill A. Shutemov wrote: [..] > > diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c > index 5f2d03067ae5..2faddeb0398a 100644 > --- a/arch/x86/boot/compressed/pgtable_64.c > +++ b/arch/x86/boot/compressed/pgtable_64.c > @@ -72,6 +72,8 @@ static unsigned long find_trampoline_placement(void) > > /* Find the first usable memory region under bios_start. */ > for (i = boot_params->e820_entries - 1; i >= 0; i--) { > + unsigned long new; > + > entry = &boot_params->e820_table[i]; > > /* Skip all entries above bios_start. */ > @@ -84,15 +86,20 @@ static unsigned long find_trampoline_placement(void) > > /* Adjust bios_start to the end of the entry if needed. */ > if (bios_start > entry->addr + entry->size) Notice that if this condition happens to be false, we end up with an uninitialized variable *new*. What would be the right value to assign to *new* at declaration under this condition? > - bios_start = entry->addr + entry->size; > + new = entry->addr + entry->size; > > /* Keep bios_start page-aligned. */ > - bios_start = round_down(bios_start, PAGE_SIZE); > + new = round_down(new, PAGE_SIZE); > > /* Skip the entry if it's too small. */ > - if (bios_start - TRAMPOLINE_32BIT_SIZE < entry->addr) > + if (new - TRAMPOLINE_32BIT_SIZE < entry->addr) > continue; > > + /* Protect against underflow. */ > + if (new - TRAMPOLINE_32BIT_SIZE > bios_start) > + break; > + > + bios_start = new; > break; > } > > -- Gustavo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tip:x86/urgent] x86/boot/compressed/64: Fix boot on machines with broken E820 table 2019-08-26 3:33 ` Gustavo A. R. Silva @ 2019-08-26 7:15 ` Borislav Petkov 2019-08-26 13:33 ` Kirill A. Shutemov 0 siblings, 1 reply; 5+ messages in thread From: Borislav Petkov @ 2019-08-26 7:15 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: tglx, hpa, linux-kernel, mingo, x86, mingo, kirill.shutemov, kirill, linux-tip-commits On Sun, Aug 25, 2019 at 10:33:15PM -0500, Gustavo A. R. Silva wrote: > Hi all, > > On 8/19/19 9:16 AM, tip-bot for Kirill A. Shutemov wrote: > [..] > > > > diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c > > index 5f2d03067ae5..2faddeb0398a 100644 > > --- a/arch/x86/boot/compressed/pgtable_64.c > > +++ b/arch/x86/boot/compressed/pgtable_64.c > > @@ -72,6 +72,8 @@ static unsigned long find_trampoline_placement(void) > > > > /* Find the first usable memory region under bios_start. */ > > for (i = boot_params->e820_entries - 1; i >= 0; i--) { > > + unsigned long new; > > + > > entry = &boot_params->e820_table[i]; > > > > /* Skip all entries above bios_start. */ > > @@ -84,15 +86,20 @@ static unsigned long find_trampoline_placement(void) > > > > /* Adjust bios_start to the end of the entry if needed. */ > > if (bios_start > entry->addr + entry->size) > > Notice that if this condition happens to be false, we end up with an > uninitialized variable *new*. Yap, good catch. > What would be the right value to assign to *new* at declaration under > this condition? Looking at the changed flow of the loop, how we use new instead of bios_start and how we assign new back to bios_start, I think we should do: unsigned long new = bios_start; at the beginning... -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tip:x86/urgent] x86/boot/compressed/64: Fix boot on machines with broken E820 table 2019-08-26 7:15 ` Borislav Petkov @ 2019-08-26 13:33 ` Kirill A. Shutemov 0 siblings, 0 replies; 5+ messages in thread From: Kirill A. Shutemov @ 2019-08-26 13:33 UTC (permalink / raw) To: Borislav Petkov Cc: Gustavo A. R. Silva, tglx, hpa, linux-kernel, mingo, x86, mingo, kirill.shutemov, linux-tip-commits On Mon, Aug 26, 2019 at 09:15:39AM +0200, Borislav Petkov wrote: > On Sun, Aug 25, 2019 at 10:33:15PM -0500, Gustavo A. R. Silva wrote: > > Hi all, > > > > On 8/19/19 9:16 AM, tip-bot for Kirill A. Shutemov wrote: > > [..] > > > > > > diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c > > > index 5f2d03067ae5..2faddeb0398a 100644 > > > --- a/arch/x86/boot/compressed/pgtable_64.c > > > +++ b/arch/x86/boot/compressed/pgtable_64.c > > > @@ -72,6 +72,8 @@ static unsigned long find_trampoline_placement(void) > > > > > > /* Find the first usable memory region under bios_start. */ > > > for (i = boot_params->e820_entries - 1; i >= 0; i--) { > > > + unsigned long new; > > > + > > > entry = &boot_params->e820_table[i]; > > > > > > /* Skip all entries above bios_start. */ > > > @@ -84,15 +86,20 @@ static unsigned long find_trampoline_placement(void) > > > > > > /* Adjust bios_start to the end of the entry if needed. */ > > > if (bios_start > entry->addr + entry->size) > > > > Notice that if this condition happens to be false, we end up with an > > uninitialized variable *new*. > > Yap, good catch. :facepalm: > > What would be the right value to assign to *new* at declaration under > > this condition? > > Looking at the changed flow of the loop, how we use new instead of > bios_start and how we assign new back to bios_start, I think we should > do: > > unsigned long new = bios_start; > > at the beginning... Right. What about this: From b613c675e6690ef5608a5abf71d90e15ced31b2b Mon Sep 17 00:00:00 2001 From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> Date: Mon, 26 Aug 2019 16:26:01 +0300 Subject: [PATCH] x86/boot/compressed/64: Fix missining initialization in find_trampoline_placement() Gustavo noticed that 'new' can be left uninitialized if 'bios_start' happens to be less or equal to 'entry->addr + entry->size'. Initialize the variable at the start of the iteration to current value of 'bios_start'. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Reported-by: "Gustavo A. R. Silva" <gustavo@embeddedor.com> Fixes: 0a46fff2f910 ("x86/boot/compressed/64: Fix boot on machines with broken E820 table") --- arch/x86/boot/compressed/pgtable_64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c index 2faddeb0398a..c8862696a47b 100644 --- a/arch/x86/boot/compressed/pgtable_64.c +++ b/arch/x86/boot/compressed/pgtable_64.c @@ -72,7 +72,7 @@ static unsigned long find_trampoline_placement(void) /* Find the first usable memory region under bios_start. */ for (i = boot_params->e820_entries - 1; i >= 0; i--) { - unsigned long new; + unsigned long new = bios_start; entry = &boot_params->e820_table[i]; -- Kirill A. Shutemov ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-26 13:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-13 13:16 [PATCH] x86/boot/compressed/64: Fix boot on machines with broken E820 table Kirill A. Shutemov 2019-08-19 14:16 ` [tip:x86/urgent] " tip-bot for Kirill A. Shutemov 2019-08-26 3:33 ` Gustavo A. R. Silva 2019-08-26 7:15 ` Borislav Petkov 2019-08-26 13:33 ` Kirill A. Shutemov
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).