x86/boot/64: clang: use fixup_pointer() to access next_early_pgt
PATCH 1/1 x86/boot/64: clang: use fixup_pointer() to access next_early_pgt
__startup_64() is normally using fixup_pointer() to access globals in a position-independent fashion. However |next_early_pgt| was accessed directly, which wasn't guaranteed to work.
Luckily GCC was generating a R_X86_64_PC32 PC-relative relocation for |next_early_pgt|, but Clang emitted a R_X86_64_32S, which led to accessing invalid memory and rebooting the kernel.
--- arch/x86/kernel/head64.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
arch/x86/kernel/head64.c +4 -3
@@ -53,6 +53,7 @@ void __head __startup_64(unsigned long physaddr)pudval_t *pud;pmdval_t *pmd, pmd_entry;int i; +unsigned int *next_pgt_ptr;/* Is the address too large? */ if (physaddr >>MAX_PHYSMEM_BITS )
@@ -91,9 +92,9 @@ void __head __startup_64(unsigned long physaddr)* creates a bunch of nonsense entries but that is fine -- * it avoids problems around wraparound. */ - - pud =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr); - pmd =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr); + next_pgt_ptr =fixup_pointer (&next_early_pgt, physaddr); + pud =fixup_pointer (early_dynamic_pgts[(*next_pgt_ptr)++], physaddr); + pmd =fixup_pointer (early_dynamic_pgts[(*next_pgt_ptr)++], physaddr);if (IS_ENABLED (CONFIG_X86_5LEVEL )) { p4d =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr);
Signature · 1 line
2.14.1.480.gb18f417b89-goog
Quoted 45 lines · Alexander Potapenko
__startup_64()is normally usingfixup_pointer()to access globals in a position-independent fashion. However |next_early_pgt| was accessed directly, which wasn't guaranteed to work.Luckily GCC was generating a
R_X86_64_PC32PC-relative relocation for |next_early_pgt|, but Clang emitted aR_X86_64_32S, which led to accessing invalid memory and rebooting the kernel.Cc Kirill A. Shutemov <[email protected]>Cc Dmitry Vyukov <[email protected]>Cc Michael Davidson <[email protected]>Fixes c88d71508e36 ("x86/boot/64: Rewrite startup_64() in C")Signed-off-by Alexander Potapenko <[email protected]>--- arch/x86/kernel/head64.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)arch/x86/kernel/head64.c +4
-4@@ -53,6 +53,7 @@ void __head __startup_64(unsigned long physaddr)pudval_t *pud;pmdval_t *pmd, pmd_entry;int i; +unsigned int *next_pgt_ptr;/* Is the address too large? */ if (physaddr >>MAX_PHYSMEM_BITS )@@ -91,9 +92,9 @@ void __head __startup_64(unsigned long physaddr)* creates a bunch of nonsense entries but that is fine -- * it avoids problems around wraparound. */ - - pud =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr); - pmd =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr); + next_pgt_ptr =fixup_pointer (&next_early_pgt, physaddr); + pud =fixup_pointer (early_dynamic_pgts[(*next_pgt_ptr)++], physaddr); + pmd =fixup_pointer (early_dynamic_pgts[(*next_pgt_ptr)++], physaddr);if (IS_ENABLED (CONFIG_X86_5LEVEL )) { p4d =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr); --2.14.1.480.gb18f417b89-goog
Signature · 10 lines
Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
Quoted 7 lines · Alexander Potapenko
__startup_64()is normally usingfixup_pointer()to access globals in a position-independent fashion. However |next_early_pgt| was accessed directly, which wasn't guaranteed to work.Luckily GCC was generating a
R_X86_64_PC32PC-relative relocation for |next_early_pgt|, but Clang emitted aR_X86_64_32S, which led to accessing invalid memory and rebooting the kernel.
Thanks for tracking this down.
Quoted 4 lines
Cc Kirill A. Shutemov <[email protected]>Cc Dmitry Vyukov <[email protected]>Cc Michael Davidson <[email protected]>Fixes c88d71508e36 ("x86/boot/64: Rewrite startup_64() in C")
Signature · 1 line
Kirill A. Shutemov
tip-bot for Alexander Potapenko
merge bot[tip:x86/urgent] x86/boot/64/clang: Use fixup_pointer() to access 'next_early_pgt'
In reply to Alexander Potapenkox86/boot/64/clang: Use fixup_pointer() to access 'next_early_pgt'
Commit-ID: 187e91fe5e915f4b7f39b824aa422493463e443d Gitweb: http://git.kernel.org/tip/187e91fe5e915f4b7f39b824aa422493463e443d Author: Alexander Potapenko <[email protected]> AuthorDate: Wed, 16 Aug 2017 21:08:08 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 17 Aug 2017 09:53:00 +0200
x86/boot/64/clang: Use fixup_pointer() to access 'next_early_pgt'
__startup_64() is normally using fixup_pointer() to access globals in a position-independent fashion. However 'next_early_pgt' was accessed directly, which wasn't guaranteed to work.
Luckily GCC was generating a R_X86_64_PC32 PC-relative relocation for 'next_early_pgt', but Clang emitted a R_X86_64_32S, which led to accessing invalid memory and rebooting the kernel.
--- arch/x86/kernel/head64.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
arch/x86/kernel/head64.c +4 -3
@@ -53,6 +53,7 @@ void __head __startup_64(unsigned long physaddr)pudval_t *pud;pmdval_t *pmd, pmd_entry;int i; +unsigned int *next_pgt_ptr;/* Is the address too large? */ if (physaddr >>MAX_PHYSMEM_BITS )
@@ -91,9 +92,9 @@ void __head __startup_64(unsigned long physaddr)* creates a bunch of nonsense entries but that is fine -- * it avoids problems around wraparound. */ - - pud =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr); - pmd =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr); + next_pgt_ptr =fixup_pointer (&next_early_pgt, physaddr); + pud =fixup_pointer (early_dynamic_pgts[(*next_pgt_ptr)++], physaddr); + pmd =fixup_pointer (early_dynamic_pgts[(*next_pgt_ptr)++], physaddr);if (IS_ENABLED (CONFIG_X86_5LEVEL )) { p4d =fixup_pointer (early_dynamic_pgts[next_early_pgt++], physaddr);