All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
@ 2021-01-07 22:34 Arnd Bergmann
  2021-01-07 22:42 ` Nathan Chancellor
                   ` (4 more replies)
  0 siblings, 5 replies; 36+ messages in thread
From: Arnd Bergmann @ 2021-01-07 22:34 UTC (permalink / raw)
  To: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, Nathan Chancellor, Nick Desaulniers
  Cc: Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, linux-kernel, clang-built-linux

From: Arnd Bergmann <arnd@arndb.de>

When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():

x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'

Use the same method as in commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT
and PTRS_PER_P4D variable") and change it to MAYBE_BUILD_BUG_ON(),
so it only triggers for constant input.

Link: https://github.com/ClangBuiltLinux/linux/issues/256
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/x86/platform/efi/efi_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index e1e8d4e3a213..62bb1616b4a5 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -137,8 +137,8 @@ void efi_sync_low_kernel_mappings(void)
 	 * As with PGDs, we share all P4D entries apart from the one entry
 	 * that covers the EFI runtime mapping space.
 	 */
-	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
-	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
+	MAYBE_BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
+	MAYBE_BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
 
 	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
 	pgd_k = pgd_offset_k(EFI_VA_END);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-07 22:34 [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Arnd Bergmann
@ 2021-01-07 22:42 ` Nathan Chancellor
  2021-01-13 17:51 ` Ard Biesheuvel
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 36+ messages in thread
From: Nathan Chancellor @ 2021-01-07 22:42 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, Nick Desaulniers, Arnd Bergmann, Darren Hart,
	Andy Shevchenko, H. Peter Anvin, linux-efi, platform-driver-x86,
	linux-kernel, clang-built-linux

On Thu, Jan 07, 2021 at 11:34:15PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():
> 
> x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
> 
> Use the same method as in commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT
> and PTRS_PER_P4D variable") and change it to MAYBE_BUILD_BUG_ON(),
> so it only triggers for constant input.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/256
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>

> ---
>  arch/x86/platform/efi/efi_64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..62bb1616b4a5 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -137,8 +137,8 @@ void efi_sync_low_kernel_mappings(void)
>  	 * As with PGDs, we share all P4D entries apart from the one entry
>  	 * that covers the EFI runtime mapping space.
>  	 */
> -	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> +	MAYBE_BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> +	MAYBE_BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
>  
>  	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>  	pgd_k = pgd_offset_k(EFI_VA_END);
> -- 
> 2.29.2
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-07 22:34 [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Arnd Bergmann
  2021-01-07 22:42 ` Nathan Chancellor
@ 2021-01-13 17:51 ` Ard Biesheuvel
  2021-01-15 18:23 ` Borislav Petkov
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 36+ messages in thread
From: Ard Biesheuvel @ 2021-01-13 17:51 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML,
	Nathan Chancellor, Nick Desaulniers, Arnd Bergmann, Darren Hart,
	Andy Shevchenko, H. Peter Anvin, linux-efi, platform-driver-x86,
	Linux Kernel Mailing List, clang-built-linux

On Thu, 7 Jan 2021 at 23:34, Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():
>
> x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
>
> Use the same method as in commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT
> and PTRS_PER_P4D variable") and change it to MAYBE_BUILD_BUG_ON(),
> so it only triggers for constant input.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/256
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

This can go via the x86 tree directly, IMO

> ---
>  arch/x86/platform/efi/efi_64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..62bb1616b4a5 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -137,8 +137,8 @@ void efi_sync_low_kernel_mappings(void)
>          * As with PGDs, we share all P4D entries apart from the one entry
>          * that covers the EFI runtime mapping space.
>          */
> -       BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> +       MAYBE_BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> +       MAYBE_BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
>
>         pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>         pgd_k = pgd_offset_k(EFI_VA_END);
> --
> 2.29.2
>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-07 22:34 [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Arnd Bergmann
  2021-01-07 22:42 ` Nathan Chancellor
  2021-01-13 17:51 ` Ard Biesheuvel
@ 2021-01-15 18:23 ` Borislav Petkov
  2021-01-15 18:32   ` Nathan Chancellor
  2021-01-15 19:07 ` Arvind Sankar
  2021-02-06 12:56 ` [tip: x86/urgent] x86/efi: Remove EFI PGD build time checks tip-bot2 for Borislav Petkov
  4 siblings, 1 reply; 36+ messages in thread
From: Borislav Petkov @ 2021-01-15 18:23 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, x86,
	Nathan Chancellor, Nick Desaulniers, Arnd Bergmann, Darren Hart,
	Andy Shevchenko, H. Peter Anvin, linux-efi, platform-driver-x86,
	linux-kernel, clang-built-linux

On Thu, Jan 07, 2021 at 11:34:15PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():

I have CONFIG_X86_5LEVEL=y, CONFIG_EFI=y and am using Debian clang
version 10.0.1-8+b1 but my .config builds just fine.

How do you trigger this?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 18:23 ` Borislav Petkov
@ 2021-01-15 18:32   ` Nathan Chancellor
  2021-01-15 19:07     ` Borislav Petkov
  0 siblings, 1 reply; 36+ messages in thread
From: Nathan Chancellor @ 2021-01-15 18:32 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Arnd Bergmann, Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, x86,
	Nick Desaulniers, Arnd Bergmann, Darren Hart, Andy Shevchenko,
	H. Peter Anvin, linux-efi, platform-driver-x86, linux-kernel,
	clang-built-linux

On Fri, Jan 15, 2021 at 07:23:00PM +0100, Borislav Petkov wrote:
> On Thu, Jan 07, 2021 at 11:34:15PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():
> 
> I have CONFIG_X86_5LEVEL=y, CONFIG_EFI=y and am using Debian clang
> version 10.0.1-8+b1 but my .config builds just fine.
> 
> How do you trigger this?

I triggered it with CONFIG_UBSAN=y + CONFIG_UBSAN_UNSIGNED_OVERFLOW=y
(it can be exposed with an allyesconfig/allmodconfig on mainline
currently).

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 18:32   ` Nathan Chancellor
@ 2021-01-15 19:07     ` Borislav Petkov
  2021-01-15 19:11       ` Arvind Sankar
  0 siblings, 1 reply; 36+ messages in thread
From: Borislav Petkov @ 2021-01-15 19:07 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Arnd Bergmann, Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, x86,
	Nick Desaulniers, Arnd Bergmann, Darren Hart, Andy Shevchenko,
	H. Peter Anvin, linux-efi, platform-driver-x86, linux-kernel,
	clang-built-linux

On Fri, Jan 15, 2021 at 11:32:03AM -0700, Nathan Chancellor wrote:
> I triggered it with CONFIG_UBSAN=y + CONFIG_UBSAN_UNSIGNED_OVERFLOW=y
> (it can be exposed with an allyesconfig/allmodconfig on mainline
> currently).

Yah, I can trigger with that, thanks.

But I'll be damned, check this out:

clang preprocesses to this:

 do { extern void __compiletime_assert_332(void) ; if (!(!(p4d_index((-68 * ((1UL) << 30))) != p4d_index((0xffffffffff000000UL))))) __compiletime_assert_332(); } while (0);

The resulting asm is:

.LBB1_32:
        movabsq $-73014444032, %r13     # imm = 0xFFFFFFEF00000000
        testb   $1, %al
        jne     .LBB1_33
.LBB1_34:
        xorl    %r14d, %ebx
        testl   $33554431, %ebx         # imm = 0x1FFFFFF
        je      .LBB1_36
# %bb.35:
        callq   __compiletime_assert_332

so the undefined symbol is there, leading to:

ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
/home/boris/kernel/linux/arch/x86/platform/efi/efi_64.c:140: undefined reference to `__compiletime_assert_332'

Now look at gcc:

It preprocesses to:

 do { extern void __compiletime_assert_332(void) __attribute__((__error__("BUILD_BUG_ON failed: " "p4d_index(EFI_VA_END) != p4d_index(MODULES_END)"))); if (!(!(p4d_index((-68 * ((1UL) << 30))) != p4d_index((0xffffffffff000000UL))))) __compiletime_assert_332(); } while (0);


Resulting asm:

$ grep __compiletime_assert_332  arch/x86/platform/efi/efi_64.s
$

That thing has been optimized away!

Which means, those build assertions are gone on gcc and they don't catch
diddly squat. I sure hope I'm missing something here...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-07 22:34 [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Arnd Bergmann
                   ` (2 preceding siblings ...)
  2021-01-15 18:23 ` Borislav Petkov
@ 2021-01-15 19:07 ` Arvind Sankar
  2021-01-15 20:27   ` Arvind Sankar
  2021-01-20 11:06   ` Kirill A. Shutemov
  2021-02-06 12:56 ` [tip: x86/urgent] x86/efi: Remove EFI PGD build time checks tip-bot2 for Borislav Petkov
  4 siblings, 2 replies; 36+ messages in thread
From: Arvind Sankar @ 2021-01-15 19:07 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, Nathan Chancellor, Nick Desaulniers, Arnd Bergmann,
	Darren Hart, Andy Shevchenko, H. Peter Anvin, linux-efi,
	platform-driver-x86, linux-kernel, clang-built-linux

On Thu, Jan 07, 2021 at 11:34:15PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():
> 
> x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
> 
> Use the same method as in commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT
> and PTRS_PER_P4D variable") and change it to MAYBE_BUILD_BUG_ON(),
> so it only triggers for constant input.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/256
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/x86/platform/efi/efi_64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..62bb1616b4a5 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -137,8 +137,8 @@ void efi_sync_low_kernel_mappings(void)
>  	 * As with PGDs, we share all P4D entries apart from the one entry
>  	 * that covers the EFI runtime mapping space.
>  	 */
> -	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> +	MAYBE_BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> +	MAYBE_BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
>  
>  	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>  	pgd_k = pgd_offset_k(EFI_VA_END);
> -- 
> 2.29.2
> 

I think this needs more explanation as to why clang is triggering this.
The issue mentions clang not inline p4d_index(), and I guess not
performing inter-procedural analysis either?

For the second assertion there, everything is always constant AFAICT:
EFI_VA_START, EFI_VA_END and P4D_MASK are all constants regardless of
CONFIG_5LEVEL.

For the first assertion, it isn't technically constant, but if
p4d_index() gets inlined, the compiler should be able to see that the
two are always equal, even though ptrs_per_p4d is not constant:
	EFI_VA_END >> 39 == MODULES_END >> 39
so the masking with ptrs_per_p4d-1 doesn't matter for the comparison.

As a matter of fact, it seems like the four assertions could be combined
into:
	BUILD_BUG_ON((EFI_VA_END & P4D_MASK) != (MODULES_END & P4D_MASK));
	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
instead of separately asserting they're the same PGD entry and the same
P4D entry.

Thanks.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 19:07     ` Borislav Petkov
@ 2021-01-15 19:11       ` Arvind Sankar
  2021-01-15 19:18         ` Borislav Petkov
  0 siblings, 1 reply; 36+ messages in thread
From: Arvind Sankar @ 2021-01-15 19:11 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Nathan Chancellor, Arnd Bergmann, Ard Biesheuvel,
	Thomas Gleixner, Ingo Molnar, x86, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, linux-kernel, clang-built-linux

On Fri, Jan 15, 2021 at 08:07:29PM +0100, Borislav Petkov wrote:
> On Fri, Jan 15, 2021 at 11:32:03AM -0700, Nathan Chancellor wrote:
> > I triggered it with CONFIG_UBSAN=y + CONFIG_UBSAN_UNSIGNED_OVERFLOW=y
> > (it can be exposed with an allyesconfig/allmodconfig on mainline
> > currently).
> 
> Yah, I can trigger with that, thanks.
> 
> But I'll be damned, check this out:
> 
> clang preprocesses to this:
> 
>  do { extern void __compiletime_assert_332(void) ; if (!(!(p4d_index((-68 * ((1UL) << 30))) != p4d_index((0xffffffffff000000UL))))) __compiletime_assert_332(); } while (0);
> 
> The resulting asm is:
> 
> .LBB1_32:
>         movabsq $-73014444032, %r13     # imm = 0xFFFFFFEF00000000
>         testb   $1, %al
>         jne     .LBB1_33
> .LBB1_34:
>         xorl    %r14d, %ebx
>         testl   $33554431, %ebx         # imm = 0x1FFFFFF
>         je      .LBB1_36
> # %bb.35:
>         callq   __compiletime_assert_332
> 
> so the undefined symbol is there, leading to:
> 
> ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> /home/boris/kernel/linux/arch/x86/platform/efi/efi_64.c:140: undefined reference to `__compiletime_assert_332'
> 
> Now look at gcc:
> 
> It preprocesses to:
> 
>  do { extern void __compiletime_assert_332(void) __attribute__((__error__("BUILD_BUG_ON failed: " "p4d_index(EFI_VA_END) != p4d_index(MODULES_END)"))); if (!(!(p4d_index((-68 * ((1UL) << 30))) != p4d_index((0xffffffffff000000UL))))) __compiletime_assert_332(); } while (0);
> 
> 
> Resulting asm:
> 
> $ grep __compiletime_assert_332  arch/x86/platform/efi/efi_64.s
> $
> 
> That thing has been optimized away!
> 
> Which means, those build assertions are gone on gcc and they don't catch
> diddly squat. I sure hope I'm missing something here...

That's how build-time assertions work: they are _supposed_ to be
optimized away completely when the assertion is true. If they're
_not_ optimized away, the build will fail.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 19:11       ` Arvind Sankar
@ 2021-01-15 19:18         ` Borislav Petkov
  2021-01-15 19:54           ` Arnd Bergmann
  0 siblings, 1 reply; 36+ messages in thread
From: Borislav Petkov @ 2021-01-15 19:18 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Nathan Chancellor, Arnd Bergmann, Ard Biesheuvel,
	Thomas Gleixner, Ingo Molnar, x86, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, linux-kernel, clang-built-linux

On Fri, Jan 15, 2021 at 02:11:25PM -0500, Arvind Sankar wrote:
> That's how build-time assertions work: they are _supposed_ to be
> optimized away completely when the assertion is true. If they're
> _not_ optimized away, the build will fail.

Yah, that I know, thanks.

If gcc really inlines p4d_index() and does a lot more aggressive
optimization to determine that the condition is false and thus optimize
everything away (and clang doesn't), then that would explain the
observation.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 19:18         ` Borislav Petkov
@ 2021-01-15 19:54           ` Arnd Bergmann
  2021-01-15 20:12             ` Arvind Sankar
  0 siblings, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2021-01-15 19:54 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Arvind Sankar, Nathan Chancellor, Ard Biesheuvel,
	Thomas Gleixner, Ingo Molnar, the arch/x86 maintainers,
	Nick Desaulniers, Arnd Bergmann, Darren Hart, Andy Shevchenko,
	H. Peter Anvin, linux-efi, Platform Driver, linux-kernel,
	clang-built-linux

On Fri, Jan 15, 2021 at 8:18 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Jan 15, 2021 at 02:11:25PM -0500, Arvind Sankar wrote:
> > That's how build-time assertions work: they are _supposed_ to be
> > optimized away completely when the assertion is true. If they're
> > _not_ optimized away, the build will fail.
>
> Yah, that I know, thanks.
>
> If gcc really inlines p4d_index() and does a lot more aggressive
> optimization to determine that the condition is false and thus optimize
> everything away (and clang doesn't), then that would explain the
> observation.

One difference is that gcc does not have
-fsanitize=unsigned-integer-overflow at all, and I don't see the
assertion without that on clang either, so it's possible that clang
behaves as designed here.

The description is:
    -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
     the result of an unsigned integer computation cannot be represented in
     its type. Unlike signed integer overflow, this is not undefined behavior,
     but it is often unintentional. This sanitizer does not check for
lossy implicit
     conversions performed before such a computation (see
    -fsanitize=implicit-conversion).

The "-68 * ((1UL) << 30" computation does overflow an unsigned long
as intended, right? Maybe this is enough for the ubsan code in clang to
just disable some of the optimization steps that the assertion relies on.

        Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 19:54           ` Arnd Bergmann
@ 2021-01-15 20:12             ` Arvind Sankar
  2021-01-15 20:32               ` Arvind Sankar
  0 siblings, 1 reply; 36+ messages in thread
From: Arvind Sankar @ 2021-01-15 20:12 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Borislav Petkov, Arvind Sankar, Nathan Chancellor,
	Ard Biesheuvel, Thomas Gleixner, Ingo Molnar,
	the arch/x86 maintainers, Nick Desaulniers, Arnd Bergmann,
	Darren Hart, Andy Shevchenko, H. Peter Anvin, linux-efi,
	Platform Driver, linux-kernel, clang-built-linux

On Fri, Jan 15, 2021 at 08:54:18PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 15, 2021 at 8:18 PM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Fri, Jan 15, 2021 at 02:11:25PM -0500, Arvind Sankar wrote:
> > > That's how build-time assertions work: they are _supposed_ to be
> > > optimized away completely when the assertion is true. If they're
> > > _not_ optimized away, the build will fail.
> >
> > Yah, that I know, thanks.
> >
> > If gcc really inlines p4d_index() and does a lot more aggressive
> > optimization to determine that the condition is false and thus optimize
> > everything away (and clang doesn't), then that would explain the
> > observation.
> 
> One difference is that gcc does not have
> -fsanitize=unsigned-integer-overflow at all, and I don't see the
> assertion without that on clang either, so it's possible that clang
> behaves as designed here.
> 
> The description is:
>     -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
>      the result of an unsigned integer computation cannot be represented in
>      its type. Unlike signed integer overflow, this is not undefined behavior,
>      but it is often unintentional. This sanitizer does not check for
> lossy implicit
>      conversions performed before such a computation (see
>     -fsanitize=implicit-conversion).
> 
> The "-68 * ((1UL) << 30" computation does overflow an unsigned long
> as intended, right? Maybe this is enough for the ubsan code in clang to
> just disable some of the optimization steps that the assertion relies on.
> 
>         Arnd

That does seem to be an overflow, but that happens at compile-time.
Maybe
	AC(-68, UL) << 30
would be a better definition to avoid overflow.

The real issue might be (ptrs_per_p4d - 1) which can overflow at
run-time, and maybe the added ubsan code makes p4d_index() not worth
inlining according to clang?

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 19:07 ` Arvind Sankar
@ 2021-01-15 20:27   ` Arvind Sankar
  2021-01-16 16:34     ` Ard Biesheuvel
  2021-01-20 11:06   ` Kirill A. Shutemov
  1 sibling, 1 reply; 36+ messages in thread
From: Arvind Sankar @ 2021-01-15 20:27 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Arnd Bergmann, Ard Biesheuvel, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, linux-kernel, clang-built-linux

On Fri, Jan 15, 2021 at 02:07:51PM -0500, Arvind Sankar wrote:
> On Thu, Jan 07, 2021 at 11:34:15PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():
> > 
> > x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> > efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
> > 
> > Use the same method as in commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT
> > and PTRS_PER_P4D variable") and change it to MAYBE_BUILD_BUG_ON(),
> > so it only triggers for constant input.
> > 
> > Link: https://github.com/ClangBuiltLinux/linux/issues/256
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  arch/x86/platform/efi/efi_64.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index e1e8d4e3a213..62bb1616b4a5 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -137,8 +137,8 @@ void efi_sync_low_kernel_mappings(void)
> >  	 * As with PGDs, we share all P4D entries apart from the one entry
> >  	 * that covers the EFI runtime mapping space.
> >  	 */
> > -	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > -	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > +	MAYBE_BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > +	MAYBE_BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> >  
> >  	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
> >  	pgd_k = pgd_offset_k(EFI_VA_END);
> > -- 
> > 2.29.2
> > 
> 
> I think this needs more explanation as to why clang is triggering this.
> The issue mentions clang not inline p4d_index(), and I guess not
> performing inter-procedural analysis either?
> 
> For the second assertion there, everything is always constant AFAICT:
> EFI_VA_START, EFI_VA_END and P4D_MASK are all constants regardless of
> CONFIG_5LEVEL.
> 
> For the first assertion, it isn't technically constant, but if
> p4d_index() gets inlined, the compiler should be able to see that the
> two are always equal, even though ptrs_per_p4d is not constant:
> 	EFI_VA_END >> 39 == MODULES_END >> 39
> so the masking with ptrs_per_p4d-1 doesn't matter for the comparison.
> 
> As a matter of fact, it seems like the four assertions could be combined
> into:
> 	BUILD_BUG_ON((EFI_VA_END & P4D_MASK) != (MODULES_END & P4D_MASK));
> 	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> instead of separately asserting they're the same PGD entry and the same
> P4D entry.
> 
> Thanks.

I actually don't quite get the MODULES_END check -- Ard, do you know
what that's for?

What we really should be checking is that EFI_VA_START is in the top-most
PGD entry and the top-most P4D entry, since we only copy PGD/P4D entries
before EFI_VA_END, but not after EFI_VA_START. So the checks should
really be
	BUILD_BUG_ON(((EFI_VA_START - 1) & P4D_MASK) != (-1ul & P4D_MASK));
	BUILD_BUG_ON(((EFI_VA_START - 1) & P4D_MASK) != (EFI_VA_END & P4D_MASK));
imo. I guess that's what using MODULES_END is effectively checking, but
it would be clearer to check it directly.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 20:12             ` Arvind Sankar
@ 2021-01-15 20:32               ` Arvind Sankar
  0 siblings, 0 replies; 36+ messages in thread
From: Arvind Sankar @ 2021-01-15 20:32 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Arnd Bergmann, Borislav Petkov, Nathan Chancellor,
	Ard Biesheuvel, Thomas Gleixner, Ingo Molnar,
	the arch/x86 maintainers, Nick Desaulniers, Arnd Bergmann,
	Darren Hart, Andy Shevchenko, H. Peter Anvin, linux-efi,
	Platform Driver, linux-kernel, clang-built-linux

On Fri, Jan 15, 2021 at 03:12:25PM -0500, Arvind Sankar wrote:
> On Fri, Jan 15, 2021 at 08:54:18PM +0100, Arnd Bergmann wrote:
> > On Fri, Jan 15, 2021 at 8:18 PM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > On Fri, Jan 15, 2021 at 02:11:25PM -0500, Arvind Sankar wrote:
> > > > That's how build-time assertions work: they are _supposed_ to be
> > > > optimized away completely when the assertion is true. If they're
> > > > _not_ optimized away, the build will fail.
> > >
> > > Yah, that I know, thanks.
> > >
> > > If gcc really inlines p4d_index() and does a lot more aggressive
> > > optimization to determine that the condition is false and thus optimize
> > > everything away (and clang doesn't), then that would explain the
> > > observation.
> > 
> > One difference is that gcc does not have
> > -fsanitize=unsigned-integer-overflow at all, and I don't see the
> > assertion without that on clang either, so it's possible that clang
> > behaves as designed here.
> > 
> > The description is:
> >     -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
> >      the result of an unsigned integer computation cannot be represented in
> >      its type. Unlike signed integer overflow, this is not undefined behavior,
> >      but it is often unintentional. This sanitizer does not check for
> > lossy implicit
> >      conversions performed before such a computation (see
> >     -fsanitize=implicit-conversion).
> > 
> > The "-68 * ((1UL) << 30" computation does overflow an unsigned long
> > as intended, right? Maybe this is enough for the ubsan code in clang to
> > just disable some of the optimization steps that the assertion relies on.
> > 
> >         Arnd
> 
> That does seem to be an overflow, but that happens at compile-time.
> Maybe
> 	AC(-68, UL) << 30
> would be a better definition to avoid overflow.

Eh, that's an overflow too, isn't it :( Is this option really useful
with kernel code -- this sort of thing is probably done all over the
place?

> 
> The real issue might be (ptrs_per_p4d - 1) which can overflow at
> run-time, and maybe the added ubsan code makes p4d_index() not worth
> inlining according to clang?

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 20:27   ` Arvind Sankar
@ 2021-01-16 16:34     ` Ard Biesheuvel
  2021-01-18 20:24       ` Borislav Petkov
  0 siblings, 1 reply; 36+ messages in thread
From: Ard Biesheuvel @ 2021-01-16 16:34 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Arnd Bergmann, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, Nathan Chancellor, Nick Desaulniers, Arnd Bergmann,
	Darren Hart, Andy Shevchenko, H. Peter Anvin, linux-efi,
	platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux

On Fri, 15 Jan 2021 at 21:27, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Fri, Jan 15, 2021 at 02:07:51PM -0500, Arvind Sankar wrote:
> > On Thu, Jan 07, 2021 at 11:34:15PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():
> > >
> > > x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> > > efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
> > >
> > > Use the same method as in commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT
> > > and PTRS_PER_P4D variable") and change it to MAYBE_BUILD_BUG_ON(),
> > > so it only triggers for constant input.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/256
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > >  arch/x86/platform/efi/efi_64.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > index e1e8d4e3a213..62bb1616b4a5 100644
> > > --- a/arch/x86/platform/efi/efi_64.c
> > > +++ b/arch/x86/platform/efi/efi_64.c
> > > @@ -137,8 +137,8 @@ void efi_sync_low_kernel_mappings(void)
> > >      * As with PGDs, we share all P4D entries apart from the one entry
> > >      * that covers the EFI runtime mapping space.
> > >      */
> > > -   BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > > -   BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > > +   MAYBE_BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > > +   MAYBE_BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > >
> > >     pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
> > >     pgd_k = pgd_offset_k(EFI_VA_END);
> > > --
> > > 2.29.2
> > >
> >
> > I think this needs more explanation as to why clang is triggering this.
> > The issue mentions clang not inline p4d_index(), and I guess not
> > performing inter-procedural analysis either?
> >
> > For the second assertion there, everything is always constant AFAICT:
> > EFI_VA_START, EFI_VA_END and P4D_MASK are all constants regardless of
> > CONFIG_5LEVEL.
> >
> > For the first assertion, it isn't technically constant, but if
> > p4d_index() gets inlined, the compiler should be able to see that the
> > two are always equal, even though ptrs_per_p4d is not constant:
> >       EFI_VA_END >> 39 == MODULES_END >> 39
> > so the masking with ptrs_per_p4d-1 doesn't matter for the comparison.
> >
> > As a matter of fact, it seems like the four assertions could be combined
> > into:
> >       BUILD_BUG_ON((EFI_VA_END & P4D_MASK) != (MODULES_END & P4D_MASK));
> >       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > instead of separately asserting they're the same PGD entry and the same
> > P4D entry.
> >
> > Thanks.
>
> I actually don't quite get the MODULES_END check -- Ard, do you know
> what that's for?
>

Maybe Boris remembers? He wrote the original code for the 'new' EFI
page table layout.


> What we really should be checking is that EFI_VA_START is in the top-most
> PGD entry and the top-most P4D entry, since we only copy PGD/P4D entries
> before EFI_VA_END, but not after EFI_VA_START. So the checks should
> really be
>         BUILD_BUG_ON(((EFI_VA_START - 1) & P4D_MASK) != (-1ul & P4D_MASK));
>         BUILD_BUG_ON(((EFI_VA_START - 1) & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> imo. I guess that's what using MODULES_END is effectively checking, but
> it would be clearer to check it directly.

This obviously needs a comment, but checking that everything lives in
the top 512 GB of the kernel VA space seems sufficient to me,

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-16 16:34     ` Ard Biesheuvel
@ 2021-01-18 20:24       ` Borislav Petkov
  2021-01-18 21:42         ` Arvind Sankar
  0 siblings, 1 reply; 36+ messages in thread
From: Borislav Petkov @ 2021-01-18 20:24 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Arvind Sankar, Arnd Bergmann, Thomas Gleixner, Ingo Molnar,
	X86 ML, Nathan Chancellor, Nick Desaulniers, Arnd Bergmann,
	Darren Hart, Andy Shevchenko, H. Peter Anvin, linux-efi,
	platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Sat, Jan 16, 2021 at 05:34:27PM +0100, Ard Biesheuvel wrote:
> On Fri, 15 Jan 2021 at 21:27, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Fri, Jan 15, 2021 at 02:07:51PM -0500, Arvind Sankar wrote:
> > > On Thu, Jan 07, 2021 at 11:34:15PM +0100, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > >
> > > > When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():
> > > >
> > > > x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> > > > efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
> > > >
> > > > Use the same method as in commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT
> > > > and PTRS_PER_P4D variable") and change it to MAYBE_BUILD_BUG_ON(),
> > > > so it only triggers for constant input.
> > > >
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/256
> > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > > ---
> > > >  arch/x86/platform/efi/efi_64.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > > index e1e8d4e3a213..62bb1616b4a5 100644
> > > > --- a/arch/x86/platform/efi/efi_64.c
> > > > +++ b/arch/x86/platform/efi/efi_64.c
> > > > @@ -137,8 +137,8 @@ void efi_sync_low_kernel_mappings(void)
> > > >      * As with PGDs, we share all P4D entries apart from the one entry
> > > >      * that covers the EFI runtime mapping space.
> > > >      */
> > > > -   BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > > > -   BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > > > +   MAYBE_BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > > > +   MAYBE_BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > > >
> > > >     pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
> > > >     pgd_k = pgd_offset_k(EFI_VA_END);
> > > > --
> > > > 2.29.2
> > > >
> > >
> > > I think this needs more explanation as to why clang is triggering this.
> > > The issue mentions clang not inline p4d_index(), and I guess not
> > > performing inter-procedural analysis either?
> > >
> > > For the second assertion there, everything is always constant AFAICT:
> > > EFI_VA_START, EFI_VA_END and P4D_MASK are all constants regardless of
> > > CONFIG_5LEVEL.
> > >
> > > For the first assertion, it isn't technically constant, but if
> > > p4d_index() gets inlined, the compiler should be able to see that the
> > > two are always equal, even though ptrs_per_p4d is not constant:
> > >       EFI_VA_END >> 39 == MODULES_END >> 39
> > > so the masking with ptrs_per_p4d-1 doesn't matter for the comparison.
> > >
> > > As a matter of fact, it seems like the four assertions could be combined
> > > into:
> > >       BUILD_BUG_ON((EFI_VA_END & P4D_MASK) != (MODULES_END & P4D_MASK));
> > >       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > > instead of separately asserting they're the same PGD entry and the same
> > > P4D entry.
> > >
> > > Thanks.
> >
> > I actually don't quite get the MODULES_END check -- Ard, do you know
> > what that's for?
> >
> 
> Maybe Boris remembers? He wrote the original code for the 'new' EFI
> page table layout.

That was added by Kirill for 5-level pgtables:

  e981316f5604 ("x86/efi: Add 5-level paging support")

 Documentation/x86/x86_64/mm.rst should explain the pagetable layout:

   ffffff8000000000 | -512    GB | ffffffeeffffffff |  444 GB | ... unused hole
   ffffffef00000000 |  -68    GB | fffffffeffffffff |   64 GB | EFI region mapping space
   ffffffff00000000 |   -4    GB | ffffffff7fffffff |    2 GB | ... unused hole
   ffffffff80000000 |   -2    GB | ffffffff9fffffff |  512 MB | kernel text mapping, mapped to physical address 0
   ffffffff80000000 |-2048    MB |                  |         |
   ffffffffa0000000 |-1536    MB | fffffffffeffffff | 1520 MB | module mapping space
   ffffffffff000000 |  -16    MB |                  |         |
      FIXADDR_START | ~-11    MB | ffffffffff5fffff | ~0.5 MB | kernel-internal fixmap range, variable size and offset

That thing which starts at -512 GB above is the last PGD on the
pagetable. In it, between -4G and -68G there are 64G which are the EFI
region mapping space for runtime services.

Frankly I'm not sure what this thing is testing because the EFI VA range
is hardcoded and I can't imagine it being somewhere else *except* in the
last PGD.

Lemme add Kirill for clarification.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-18 20:24       ` Borislav Petkov
@ 2021-01-18 21:42         ` Arvind Sankar
  2021-01-20  9:33           ` Ard Biesheuvel
  2021-01-20 11:26           ` Kirill A. Shutemov
  0 siblings, 2 replies; 36+ messages in thread
From: Arvind Sankar @ 2021-01-18 21:42 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ard Biesheuvel, Arvind Sankar, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Mon, Jan 18, 2021 at 09:24:09PM +0100, Borislav Petkov wrote:
> > > > As a matter of fact, it seems like the four assertions could be combined
> > > > into:
> > > >       BUILD_BUG_ON((EFI_VA_END & P4D_MASK) != (MODULES_END & P4D_MASK));
> > > >       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > > > instead of separately asserting they're the same PGD entry and the same
> > > > P4D entry.
> > > >
> > > > Thanks.
> > >
> > > I actually don't quite get the MODULES_END check -- Ard, do you know
> > > what that's for?
> > >
> > 
> > Maybe Boris remembers? He wrote the original code for the 'new' EFI
> > page table layout.
> 
> That was added by Kirill for 5-level pgtables:
> 
>   e981316f5604 ("x86/efi: Add 5-level paging support")

That just duplicates the existing pgd_index() check for the p4d_index()
as well. It looks like the original commit adding
efi_sync_low_kernel_mappings() used to copy upto the PGD entry including
MODULES_END:
  d2f7cbe7b26a7 ("x86/efi: Runtime services virtual mapping")
and then Matt changed that when creating efi_mm:
  67a9108ed4313 ("x86/efi: Build our own page table structures")
to use EFI_VA_END instead but have a check that EFI_VA_END is in the
same entry as MODULES_END.

AFAICT, MODULES_END is only relevant as being something that happens to
be in the top 512GiB, and -1ul would be clearer.

> 
>  Documentation/x86/x86_64/mm.rst should explain the pagetable layout:
> 
>    ffffff8000000000 | -512    GB | ffffffeeffffffff |  444 GB | ... unused hole
>    ffffffef00000000 |  -68    GB | fffffffeffffffff |   64 GB | EFI region mapping space
>    ffffffff00000000 |   -4    GB | ffffffff7fffffff |    2 GB | ... unused hole
>    ffffffff80000000 |   -2    GB | ffffffff9fffffff |  512 MB | kernel text mapping, mapped to physical address 0
>    ffffffff80000000 |-2048    MB |                  |         |
>    ffffffffa0000000 |-1536    MB | fffffffffeffffff | 1520 MB | module mapping space
>    ffffffffff000000 |  -16    MB |                  |         |
>       FIXADDR_START | ~-11    MB | ffffffffff5fffff | ~0.5 MB | kernel-internal fixmap range, variable size and offset
> 
> That thing which starts at -512 GB above is the last PGD on the
> pagetable. In it, between -4G and -68G there are 64G which are the EFI
> region mapping space for runtime services.
> 
> Frankly I'm not sure what this thing is testing because the EFI VA range
> is hardcoded and I can't imagine it being somewhere else *except* in the
> last PGD.

It's just so that someone doesn't just change the #define's for
EFI_VA_END/START and think that it will work, I guess.

Another reasonable option, for example, would be to reserve an entire
PGD entry, allowing everything but the PGD level to be shared, and
adding the EFI PGD to the pgd_list and getting rid of
efi_sync_low_kernel_mappings() altogether. There aren't that many PGD
entries still unused though, so this is probably not worth it.

> 
> Lemme add Kirill for clarification.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-18 21:42         ` Arvind Sankar
@ 2021-01-20  9:33           ` Ard Biesheuvel
  2021-01-20 11:44             ` Borislav Petkov
  2021-02-03 18:51             ` Nathan Chancellor
  2021-01-20 11:26           ` Kirill A. Shutemov
  1 sibling, 2 replies; 36+ messages in thread
From: Ard Biesheuvel @ 2021-01-20  9:33 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Borislav Petkov, Arnd Bergmann, Thomas Gleixner, Ingo Molnar,
	X86 ML, Nathan Chancellor, Nick Desaulniers, Arnd Bergmann,
	Darren Hart, Andy Shevchenko, H. Peter Anvin, linux-efi,
	platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Mon, 18 Jan 2021 at 22:42, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Jan 18, 2021 at 09:24:09PM +0100, Borislav Petkov wrote:
> > > > > As a matter of fact, it seems like the four assertions could be combined
> > > > > into:
> > > > >       BUILD_BUG_ON((EFI_VA_END & P4D_MASK) != (MODULES_END & P4D_MASK));
> > > > >       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > > > > instead of separately asserting they're the same PGD entry and the same
> > > > > P4D entry.
> > > > >
> > > > > Thanks.
> > > >
> > > > I actually don't quite get the MODULES_END check -- Ard, do you know
> > > > what that's for?
> > > >
> > >
> > > Maybe Boris remembers? He wrote the original code for the 'new' EFI
> > > page table layout.
> >
> > That was added by Kirill for 5-level pgtables:
> >
> >   e981316f5604 ("x86/efi: Add 5-level paging support")
>
> That just duplicates the existing pgd_index() check for the p4d_index()
> as well. It looks like the original commit adding
> efi_sync_low_kernel_mappings() used to copy upto the PGD entry including
> MODULES_END:
>   d2f7cbe7b26a7 ("x86/efi: Runtime services virtual mapping")
> and then Matt changed that when creating efi_mm:
>   67a9108ed4313 ("x86/efi: Build our own page table structures")
> to use EFI_VA_END instead but have a check that EFI_VA_END is in the
> same entry as MODULES_END.
>
> AFAICT, MODULES_END is only relevant as being something that happens to
> be in the top 512GiB, and -1ul would be clearer.
>
> >
> >  Documentation/x86/x86_64/mm.rst should explain the pagetable layout:
> >
> >    ffffff8000000000 | -512    GB | ffffffeeffffffff |  444 GB | ... unused hole
> >    ffffffef00000000 |  -68    GB | fffffffeffffffff |   64 GB | EFI region mapping space
> >    ffffffff00000000 |   -4    GB | ffffffff7fffffff |    2 GB | ... unused hole
> >    ffffffff80000000 |   -2    GB | ffffffff9fffffff |  512 MB | kernel text mapping, mapped to physical address 0
> >    ffffffff80000000 |-2048    MB |                  |         |
> >    ffffffffa0000000 |-1536    MB | fffffffffeffffff | 1520 MB | module mapping space
> >    ffffffffff000000 |  -16    MB |                  |         |
> >       FIXADDR_START | ~-11    MB | ffffffffff5fffff | ~0.5 MB | kernel-internal fixmap range, variable size and offset
> >
> > That thing which starts at -512 GB above is the last PGD on the
> > pagetable. In it, between -4G and -68G there are 64G which are the EFI
> > region mapping space for runtime services.
> >
> > Frankly I'm not sure what this thing is testing because the EFI VA range
> > is hardcoded and I can't imagine it being somewhere else *except* in the
> > last PGD.
>
> It's just so that someone doesn't just change the #define's for
> EFI_VA_END/START and think that it will work, I guess.
>
> Another reasonable option, for example, would be to reserve an entire
> PGD entry, allowing everything but the PGD level to be shared, and
> adding the EFI PGD to the pgd_list and getting rid of
> efi_sync_low_kernel_mappings() altogether. There aren't that many PGD
> entries still unused though, so this is probably not worth it.
>

The churn doesn't seem to be worth it, tbh.

So could we get rid of the complexity here, and only build_bug() on
the start address of the EFI region being outside the topmost p4d?
That should make the PGD test redundant as well.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-15 19:07 ` Arvind Sankar
  2021-01-15 20:27   ` Arvind Sankar
@ 2021-01-20 11:06   ` Kirill A. Shutemov
  1 sibling, 0 replies; 36+ messages in thread
From: Kirill A. Shutemov @ 2021-01-20 11:06 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Arnd Bergmann, Ard Biesheuvel, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, linux-kernel, clang-built-linux

On Fri, Jan 15, 2021 at 02:07:51PM -0500, Arvind Sankar wrote:
> On Thu, Jan 07, 2021 at 11:34:15PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > When 5-level page tables are enabled, clang triggers a BUILD_BUG_ON():
> > 
> > x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> > efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
> > 
> > Use the same method as in commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT
> > and PTRS_PER_P4D variable") and change it to MAYBE_BUILD_BUG_ON(),
> > so it only triggers for constant input.
> > 
> > Link: https://github.com/ClangBuiltLinux/linux/issues/256
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  arch/x86/platform/efi/efi_64.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index e1e8d4e3a213..62bb1616b4a5 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -137,8 +137,8 @@ void efi_sync_low_kernel_mappings(void)
> >  	 * As with PGDs, we share all P4D entries apart from the one entry
> >  	 * that covers the EFI runtime mapping space.
> >  	 */
> > -	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > -	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > +	MAYBE_BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > +	MAYBE_BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> >  
> >  	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
> >  	pgd_k = pgd_offset_k(EFI_VA_END);
> > -- 
> > 2.29.2
> > 
> 
> I think this needs more explanation as to why clang is triggering this.
> The issue mentions clang not inline p4d_index(), and I guess not
> performing inter-procedural analysis either?
> 
> For the second assertion there, everything is always constant AFAICT:
> EFI_VA_START, EFI_VA_END and P4D_MASK are all constants regardless of
> CONFIG_5LEVEL.

Back in the days BUILD_BUG_ON() false-triggered on GCC-4.8 as well. 

-- 
 Kirill A. Shutemov

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-18 21:42         ` Arvind Sankar
  2021-01-20  9:33           ` Ard Biesheuvel
@ 2021-01-20 11:26           ` Kirill A. Shutemov
  1 sibling, 0 replies; 36+ messages in thread
From: Kirill A. Shutemov @ 2021-01-20 11:26 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Borislav Petkov, Ard Biesheuvel, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Mon, Jan 18, 2021 at 04:42:20PM -0500, Arvind Sankar wrote:
> AFAICT, MODULES_END is only relevant as being something that happens to
> be in the top 512GiB, and -1ul would be clearer.

I think you are right. But -1UL is not very self-descriptive. :/

-- 
 Kirill A. Shutemov

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-20  9:33           ` Ard Biesheuvel
@ 2021-01-20 11:44             ` Borislav Petkov
  2021-02-03 18:51             ` Nathan Chancellor
  1 sibling, 0 replies; 36+ messages in thread
From: Borislav Petkov @ 2021-01-20 11:44 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Arvind Sankar, Arnd Bergmann, Thomas Gleixner, Ingo Molnar,
	X86 ML, Nathan Chancellor, Nick Desaulniers, Arnd Bergmann,
	Darren Hart, Andy Shevchenko, H. Peter Anvin, linux-efi,
	platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Wed, Jan 20, 2021 at 10:33:43AM +0100, Ard Biesheuvel wrote:
> The churn doesn't seem to be worth it, tbh.
> 
> So could we get rid of the complexity here, and only build_bug() on
> the start address of the EFI region being outside the topmost p4d?
> That should make the PGD test redundant as well.

Yah, makes sense to me.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-01-20  9:33           ` Ard Biesheuvel
  2021-01-20 11:44             ` Borislav Petkov
@ 2021-02-03 18:51             ` Nathan Chancellor
  2021-02-03 20:29               ` Ard Biesheuvel
  2021-02-05 10:34               ` [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Borislav Petkov
  1 sibling, 2 replies; 36+ messages in thread
From: Nathan Chancellor @ 2021-02-03 18:51 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Arvind Sankar, Borislav Petkov, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Wed, Jan 20, 2021 at 10:33:43AM +0100, Ard Biesheuvel wrote:
> On Mon, 18 Jan 2021 at 22:42, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Mon, Jan 18, 2021 at 09:24:09PM +0100, Borislav Petkov wrote:
> > > > > > As a matter of fact, it seems like the four assertions could be combined
> > > > > > into:
> > > > > >       BUILD_BUG_ON((EFI_VA_END & P4D_MASK) != (MODULES_END & P4D_MASK));
> > > > > >       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > > > > > instead of separately asserting they're the same PGD entry and the same
> > > > > > P4D entry.
> > > > > >
> > > > > > Thanks.
> > > > >
> > > > > I actually don't quite get the MODULES_END check -- Ard, do you know
> > > > > what that's for?
> > > > >
> > > >
> > > > Maybe Boris remembers? He wrote the original code for the 'new' EFI
> > > > page table layout.
> > >
> > > That was added by Kirill for 5-level pgtables:
> > >
> > >   e981316f5604 ("x86/efi: Add 5-level paging support")
> >
> > That just duplicates the existing pgd_index() check for the p4d_index()
> > as well. It looks like the original commit adding
> > efi_sync_low_kernel_mappings() used to copy upto the PGD entry including
> > MODULES_END:
> >   d2f7cbe7b26a7 ("x86/efi: Runtime services virtual mapping")
> > and then Matt changed that when creating efi_mm:
> >   67a9108ed4313 ("x86/efi: Build our own page table structures")
> > to use EFI_VA_END instead but have a check that EFI_VA_END is in the
> > same entry as MODULES_END.
> >
> > AFAICT, MODULES_END is only relevant as being something that happens to
> > be in the top 512GiB, and -1ul would be clearer.
> >
> > >
> > >  Documentation/x86/x86_64/mm.rst should explain the pagetable layout:
> > >
> > >    ffffff8000000000 | -512    GB | ffffffeeffffffff |  444 GB | ... unused hole
> > >    ffffffef00000000 |  -68    GB | fffffffeffffffff |   64 GB | EFI region mapping space
> > >    ffffffff00000000 |   -4    GB | ffffffff7fffffff |    2 GB | ... unused hole
> > >    ffffffff80000000 |   -2    GB | ffffffff9fffffff |  512 MB | kernel text mapping, mapped to physical address 0
> > >    ffffffff80000000 |-2048    MB |                  |         |
> > >    ffffffffa0000000 |-1536    MB | fffffffffeffffff | 1520 MB | module mapping space
> > >    ffffffffff000000 |  -16    MB |                  |         |
> > >       FIXADDR_START | ~-11    MB | ffffffffff5fffff | ~0.5 MB | kernel-internal fixmap range, variable size and offset
> > >
> > > That thing which starts at -512 GB above is the last PGD on the
> > > pagetable. In it, between -4G and -68G there are 64G which are the EFI
> > > region mapping space for runtime services.
> > >
> > > Frankly I'm not sure what this thing is testing because the EFI VA range
> > > is hardcoded and I can't imagine it being somewhere else *except* in the
> > > last PGD.
> >
> > It's just so that someone doesn't just change the #define's for
> > EFI_VA_END/START and think that it will work, I guess.
> >
> > Another reasonable option, for example, would be to reserve an entire
> > PGD entry, allowing everything but the PGD level to be shared, and
> > adding the EFI PGD to the pgd_list and getting rid of
> > efi_sync_low_kernel_mappings() altogether. There aren't that many PGD
> > entries still unused though, so this is probably not worth it.
> >
> 
> The churn doesn't seem to be worth it, tbh.
> 
> So could we get rid of the complexity here, and only build_bug() on
> the start address of the EFI region being outside the topmost p4d?
> That should make the PGD test redundant as well.

Was there ever a resolution to this conversation or a patch sent? I am
still seeing the build failure that Arnd initially sent the patch for.
x86_64 all{mod,yes}config with clang are going to ship broken in 5.11.

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-03 18:51             ` Nathan Chancellor
@ 2021-02-03 20:29               ` Ard Biesheuvel
  2021-02-04 10:51                 ` Borislav Petkov
  2021-02-05 10:34               ` [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Borislav Petkov
  1 sibling, 1 reply; 36+ messages in thread
From: Ard Biesheuvel @ 2021-02-03 20:29 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Arvind Sankar, Borislav Petkov, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Wed, 3 Feb 2021 at 19:51, Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Wed, Jan 20, 2021 at 10:33:43AM +0100, Ard Biesheuvel wrote:
> > On Mon, 18 Jan 2021 at 22:42, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Mon, Jan 18, 2021 at 09:24:09PM +0100, Borislav Petkov wrote:
> > > > > > > As a matter of fact, it seems like the four assertions could be combined
> > > > > > > into:
> > > > > > >       BUILD_BUG_ON((EFI_VA_END & P4D_MASK) != (MODULES_END & P4D_MASK));
> > > > > > >       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> > > > > > > instead of separately asserting they're the same PGD entry and the same
> > > > > > > P4D entry.
> > > > > > >
> > > > > > > Thanks.
> > > > > >
> > > > > > I actually don't quite get the MODULES_END check -- Ard, do you know
> > > > > > what that's for?
> > > > > >
> > > > >
> > > > > Maybe Boris remembers? He wrote the original code for the 'new' EFI
> > > > > page table layout.
> > > >
> > > > That was added by Kirill for 5-level pgtables:
> > > >
> > > >   e981316f5604 ("x86/efi: Add 5-level paging support")
> > >
> > > That just duplicates the existing pgd_index() check for the p4d_index()
> > > as well. It looks like the original commit adding
> > > efi_sync_low_kernel_mappings() used to copy upto the PGD entry including
> > > MODULES_END:
> > >   d2f7cbe7b26a7 ("x86/efi: Runtime services virtual mapping")
> > > and then Matt changed that when creating efi_mm:
> > >   67a9108ed4313 ("x86/efi: Build our own page table structures")
> > > to use EFI_VA_END instead but have a check that EFI_VA_END is in the
> > > same entry as MODULES_END.
> > >
> > > AFAICT, MODULES_END is only relevant as being something that happens to
> > > be in the top 512GiB, and -1ul would be clearer.
> > >
> > > >
> > > >  Documentation/x86/x86_64/mm.rst should explain the pagetable layout:
> > > >
> > > >    ffffff8000000000 | -512    GB | ffffffeeffffffff |  444 GB | ... unused hole
> > > >    ffffffef00000000 |  -68    GB | fffffffeffffffff |   64 GB | EFI region mapping space
> > > >    ffffffff00000000 |   -4    GB | ffffffff7fffffff |    2 GB | ... unused hole
> > > >    ffffffff80000000 |   -2    GB | ffffffff9fffffff |  512 MB | kernel text mapping, mapped to physical address 0
> > > >    ffffffff80000000 |-2048    MB |                  |         |
> > > >    ffffffffa0000000 |-1536    MB | fffffffffeffffff | 1520 MB | module mapping space
> > > >    ffffffffff000000 |  -16    MB |                  |         |
> > > >       FIXADDR_START | ~-11    MB | ffffffffff5fffff | ~0.5 MB | kernel-internal fixmap range, variable size and offset
> > > >
> > > > That thing which starts at -512 GB above is the last PGD on the
> > > > pagetable. In it, between -4G and -68G there are 64G which are the EFI
> > > > region mapping space for runtime services.
> > > >
> > > > Frankly I'm not sure what this thing is testing because the EFI VA range
> > > > is hardcoded and I can't imagine it being somewhere else *except* in the
> > > > last PGD.
> > >
> > > It's just so that someone doesn't just change the #define's for
> > > EFI_VA_END/START and think that it will work, I guess.
> > >
> > > Another reasonable option, for example, would be to reserve an entire
> > > PGD entry, allowing everything but the PGD level to be shared, and
> > > adding the EFI PGD to the pgd_list and getting rid of
> > > efi_sync_low_kernel_mappings() altogether. There aren't that many PGD
> > > entries still unused though, so this is probably not worth it.
> > >
> >
> > The churn doesn't seem to be worth it, tbh.
> >
> > So could we get rid of the complexity here, and only build_bug() on
> > the start address of the EFI region being outside the topmost p4d?
> > That should make the PGD test redundant as well.
>
> Was there ever a resolution to this conversation or a patch sent? I am
> still seeing the build failure that Arnd initially sent the patch for.
> x86_64 all{mod,yes}config with clang are going to ship broken in 5.11.
>

I think we have agreement on the approach but it is unclear who is
going to write the patch.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-03 20:29               ` Ard Biesheuvel
@ 2021-02-04 10:51                 ` Borislav Petkov
  2021-02-04 10:59                   ` Ard Biesheuvel
                                     ` (2 more replies)
  0 siblings, 3 replies; 36+ messages in thread
From: Borislav Petkov @ 2021-02-04 10:51 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Nathan Chancellor, Arvind Sankar, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Wed, Feb 03, 2021 at 09:29:18PM +0100, Ard Biesheuvel wrote:
> I think we have agreement on the approach but it is unclear who is
> going to write the patch.

How's that below?

And frankly, I'd even vote for removing those assertions altogether. If
somehow the EFI pgd lands somewhere else, the kernel will crash'n'burn
spectacularly and quickly so it's not like we won't catch it...

---
diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
index 91ac10654570..b6be19c09841 100644
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -156,8 +156,8 @@ extern unsigned int ptrs_per_p4d;
 #define CPU_ENTRY_AREA_PGD	_AC(-4, UL)
 #define CPU_ENTRY_AREA_BASE	(CPU_ENTRY_AREA_PGD << P4D_SHIFT)
 
-#define EFI_VA_START		( -4 * (_AC(1, UL) << 30))
-#define EFI_VA_END		(-68 * (_AC(1, UL) << 30))
+#define EFI_VA_START		( -4UL * (_AC(1, UL) << 30))
+#define EFI_VA_END		(-68UL * (_AC(1, UL) << 30))
 
 #define EARLY_DYNAMIC_PAGE_TABLES	64
 
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index e1e8d4e3a213..56fdc0bbb554 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -123,9 +123,7 @@ void efi_sync_low_kernel_mappings(void)
 	 * only span a single PGD entry and that the entry also maps
 	 * other important kernel regions.
 	 */
-	MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
-	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
-			(EFI_VA_END & PGDIR_MASK));
+	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) != PGDIR_MASK);
 
 	pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
 	pgd_k = pgd_offset_k(PAGE_OFFSET);
@@ -137,8 +135,7 @@ void efi_sync_low_kernel_mappings(void)
 	 * As with PGDs, we share all P4D entries apart from the one entry
 	 * that covers the EFI runtime mapping space.
 	 */
-	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
-	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
+	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != P4D_MASK);
 
 	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
 	pgd_k = pgd_offset_k(EFI_VA_END);


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-04 10:51                 ` Borislav Petkov
@ 2021-02-04 10:59                   ` Ard Biesheuvel
  2021-02-04 19:16                   ` Nathan Chancellor
  2021-02-04 21:43                   ` Arvind Sankar
  2 siblings, 0 replies; 36+ messages in thread
From: Ard Biesheuvel @ 2021-02-04 10:59 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Nathan Chancellor, Arvind Sankar, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Thu, 4 Feb 2021 at 11:52, Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Feb 03, 2021 at 09:29:18PM +0100, Ard Biesheuvel wrote:
> > I think we have agreement on the approach but it is unclear who is
> > going to write the patch.
>
> How's that below?
>
> And frankly, I'd even vote for removing those assertions altogether. If
> somehow the EFI pgd lands somewhere else, the kernel will crash'n'burn
> spectacularly and quickly so it's not like we won't catch it...
>

+1 for just removing them

> ---
> diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
> index 91ac10654570..b6be19c09841 100644
> --- a/arch/x86/include/asm/pgtable_64_types.h
> +++ b/arch/x86/include/asm/pgtable_64_types.h
> @@ -156,8 +156,8 @@ extern unsigned int ptrs_per_p4d;
>  #define CPU_ENTRY_AREA_PGD     _AC(-4, UL)
>  #define CPU_ENTRY_AREA_BASE    (CPU_ENTRY_AREA_PGD << P4D_SHIFT)
>
> -#define EFI_VA_START           ( -4 * (_AC(1, UL) << 30))
> -#define EFI_VA_END             (-68 * (_AC(1, UL) << 30))
> +#define EFI_VA_START           ( -4UL * (_AC(1, UL) << 30))
> +#define EFI_VA_END             (-68UL * (_AC(1, UL) << 30))
>
>  #define EARLY_DYNAMIC_PAGE_TABLES      64
>
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..56fdc0bbb554 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -123,9 +123,7 @@ void efi_sync_low_kernel_mappings(void)
>          * only span a single PGD entry and that the entry also maps
>          * other important kernel regions.
>          */
> -       MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
> -       MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> -                       (EFI_VA_END & PGDIR_MASK));
> +       MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) != PGDIR_MASK);
>
>         pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
>         pgd_k = pgd_offset_k(PAGE_OFFSET);
> @@ -137,8 +135,7 @@ void efi_sync_low_kernel_mappings(void)
>          * As with PGDs, we share all P4D entries apart from the one entry
>          * that covers the EFI runtime mapping space.
>          */
> -       BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> +       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != P4D_MASK);
>
>         pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>         pgd_k = pgd_offset_k(EFI_VA_END);
>
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-04 10:51                 ` Borislav Petkov
  2021-02-04 10:59                   ` Ard Biesheuvel
@ 2021-02-04 19:16                   ` Nathan Chancellor
  2021-02-04 21:43                   ` Arvind Sankar
  2 siblings, 0 replies; 36+ messages in thread
From: Nathan Chancellor @ 2021-02-04 19:16 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ard Biesheuvel, Arvind Sankar, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Thu, Feb 04, 2021 at 11:51:55AM +0100, Borislav Petkov wrote:
> On Wed, Feb 03, 2021 at 09:29:18PM +0100, Ard Biesheuvel wrote:
> > I think we have agreement on the approach but it is unclear who is
> > going to write the patch.
> 
> How's that below?
> 
> And frankly, I'd even vote for removing those assertions altogether. If
> somehow the EFI pgd lands somewhere else, the kernel will crash'n'burn
> spectacularly and quickly so it's not like we won't catch it...
> 
> ---

This resolves the issue initially reported in this thread. Obviously
removing the assertions will do that as well. Feel free to carry
forward

Tested-by: Nathan Chancellor <nathan@kernel.org>

on a patch if you send it out.

> diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
> index 91ac10654570..b6be19c09841 100644
> --- a/arch/x86/include/asm/pgtable_64_types.h
> +++ b/arch/x86/include/asm/pgtable_64_types.h
> @@ -156,8 +156,8 @@ extern unsigned int ptrs_per_p4d;
>  #define CPU_ENTRY_AREA_PGD	_AC(-4, UL)
>  #define CPU_ENTRY_AREA_BASE	(CPU_ENTRY_AREA_PGD << P4D_SHIFT)
>  
> -#define EFI_VA_START		( -4 * (_AC(1, UL) << 30))
> -#define EFI_VA_END		(-68 * (_AC(1, UL) << 30))
> +#define EFI_VA_START		( -4UL * (_AC(1, UL) << 30))
> +#define EFI_VA_END		(-68UL * (_AC(1, UL) << 30))
>  
>  #define EARLY_DYNAMIC_PAGE_TABLES	64
>  
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..56fdc0bbb554 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -123,9 +123,7 @@ void efi_sync_low_kernel_mappings(void)
>  	 * only span a single PGD entry and that the entry also maps
>  	 * other important kernel regions.
>  	 */
> -	MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
> -	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> -			(EFI_VA_END & PGDIR_MASK));
> +	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) != PGDIR_MASK);
>  
>  	pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
>  	pgd_k = pgd_offset_k(PAGE_OFFSET);
> @@ -137,8 +135,7 @@ void efi_sync_low_kernel_mappings(void)
>  	 * As with PGDs, we share all P4D entries apart from the one entry
>  	 * that covers the EFI runtime mapping space.
>  	 */
> -	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> +	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != P4D_MASK);
>  
>  	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>  	pgd_k = pgd_offset_k(EFI_VA_END);
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-04 10:51                 ` Borislav Petkov
  2021-02-04 10:59                   ` Ard Biesheuvel
  2021-02-04 19:16                   ` Nathan Chancellor
@ 2021-02-04 21:43                   ` Arvind Sankar
  2021-02-04 22:13                     ` Borislav Petkov
  2 siblings, 1 reply; 36+ messages in thread
From: Arvind Sankar @ 2021-02-04 21:43 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ard Biesheuvel, Nathan Chancellor, Arnd Bergmann,
	Thomas Gleixner, Ingo Molnar, X86 ML, Nathan Chancellor,
	Nick Desaulniers, Arnd Bergmann, Darren Hart, Andy Shevchenko,
	H. Peter Anvin, linux-efi, platform-driver-x86,
	Linux Kernel Mailing List, clang-built-linux, Kirill A. Shutemov

On Thu, Feb 04, 2021 at 11:51:55AM +0100, Borislav Petkov wrote:
> On Wed, Feb 03, 2021 at 09:29:18PM +0100, Ard Biesheuvel wrote:
> > I think we have agreement on the approach but it is unclear who is
> > going to write the patch.
> 
> How's that below?
> 
> And frankly, I'd even vote for removing those assertions altogether. If
> somehow the EFI pgd lands somewhere else, the kernel will crash'n'burn
> spectacularly and quickly so it's not like we won't catch it...

Removing altogether should be fine, but see below if we don't.

> 
> ---
> diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
> index 91ac10654570..b6be19c09841 100644
> --- a/arch/x86/include/asm/pgtable_64_types.h
> +++ b/arch/x86/include/asm/pgtable_64_types.h
> @@ -156,8 +156,8 @@ extern unsigned int ptrs_per_p4d;
>  #define CPU_ENTRY_AREA_PGD	_AC(-4, UL)
>  #define CPU_ENTRY_AREA_BASE	(CPU_ENTRY_AREA_PGD << P4D_SHIFT)
>  
> -#define EFI_VA_START		( -4 * (_AC(1, UL) << 30))
> -#define EFI_VA_END		(-68 * (_AC(1, UL) << 30))
> +#define EFI_VA_START		( -4UL * (_AC(1, UL) << 30))
> +#define EFI_VA_END		(-68UL * (_AC(1, UL) << 30))

This doesn't have any effect right? And the reason for the _AC() stuff
in there is to allow the #define to be used in assembler -- this
particular one isn't, but it makes no sense to use the UL suffix as well
as _AC() in the same macro.

>  
>  #define EARLY_DYNAMIC_PAGE_TABLES	64
>  
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..56fdc0bbb554 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -123,9 +123,7 @@ void efi_sync_low_kernel_mappings(void)
>  	 * only span a single PGD entry and that the entry also maps
>  	 * other important kernel regions.
>  	 */
> -	MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
> -	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> -			(EFI_VA_END & PGDIR_MASK));
> +	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) != PGDIR_MASK);

This check is superfluous. Just do the P4D one.

>  
>  	pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
>  	pgd_k = pgd_offset_k(PAGE_OFFSET);
> @@ -137,8 +135,7 @@ void efi_sync_low_kernel_mappings(void)
>  	 * As with PGDs, we share all P4D entries apart from the one entry
>  	 * that covers the EFI runtime mapping space.
>  	 */
> -	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> +	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != P4D_MASK);

This should check EFI_VA_END instead of EFI_VA_START, and maybe throw in
a BUG_ON if EFI_VA_END >= EFI_VA_START.

>  
>  	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>  	pgd_k = pgd_offset_k(EFI_VA_END);
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-04 21:43                   ` Arvind Sankar
@ 2021-02-04 22:13                     ` Borislav Petkov
  2021-02-05  0:08                       ` Arvind Sankar
  0 siblings, 1 reply; 36+ messages in thread
From: Borislav Petkov @ 2021-02-04 22:13 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: Ard Biesheuvel, Nathan Chancellor, Arnd Bergmann,
	Thomas Gleixner, Ingo Molnar, X86 ML, Nathan Chancellor,
	Nick Desaulniers, Arnd Bergmann, Darren Hart, Andy Shevchenko,
	H. Peter Anvin, linux-efi, platform-driver-x86,
	Linux Kernel Mailing List, clang-built-linux, Kirill A. Shutemov

On Thu, Feb 04, 2021 at 04:43:58PM -0500, Arvind Sankar wrote:
> This should check EFI_VA_END instead of EFI_VA_START, and maybe throw in
> a BUG_ON if EFI_VA_END >= EFI_VA_START.

No need:

        if (efi_va < EFI_VA_END) {
                pr_warn(FW_WARN "VA address range overflow!\n");
                return;
        }

We already check we're not going over at map time. And our runtime
services range is hardcoded. And we're switching to that PGD on each
runtime services call.

So I don't see the point for keeping any of the assertions.

Unless you have other valid arguments for keeping them...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-04 22:13                     ` Borislav Petkov
@ 2021-02-05  0:08                       ` Arvind Sankar
  2021-02-05 11:39                         ` [PATCH] x86/efi: Remove EFI PGD build time checks Borislav Petkov
  0 siblings, 1 reply; 36+ messages in thread
From: Arvind Sankar @ 2021-02-05  0:08 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ard Biesheuvel, Nathan Chancellor, Arnd Bergmann,
	Thomas Gleixner, Ingo Molnar, X86 ML, Nathan Chancellor,
	Nick Desaulniers, Arnd Bergmann, Darren Hart, Andy Shevchenko,
	H. Peter Anvin, linux-efi, platform-driver-x86,
	Linux Kernel Mailing List, clang-built-linux, Kirill A. Shutemov

On Thu, Feb 04, 2021 at 11:13:18PM +0100, Borislav Petkov wrote:
> On Thu, Feb 04, 2021 at 04:43:58PM -0500, Arvind Sankar wrote:
> > This should check EFI_VA_END instead of EFI_VA_START, and maybe throw in
> > a BUG_ON if EFI_VA_END >= EFI_VA_START.
> 
> No need:
> 
>         if (efi_va < EFI_VA_END) {
>                 pr_warn(FW_WARN "VA address range overflow!\n");
>                 return;
>         }
> 
> We already check we're not going over at map time. And our runtime
> services range is hardcoded. And we're switching to that PGD on each
> runtime services call.
> 
> So I don't see the point for keeping any of the assertions.
> 
> Unless you have other valid arguments for keeping them...
> 

No, I don't have any objections to removing them altogether. All the
comments other than the one about changing the #define's only apply if
it's decided to keep them.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-03 18:51             ` Nathan Chancellor
  2021-02-03 20:29               ` Ard Biesheuvel
@ 2021-02-05 10:34               ` Borislav Petkov
  2021-02-05 18:27                 ` Nick Desaulniers
  1 sibling, 1 reply; 36+ messages in thread
From: Borislav Petkov @ 2021-02-05 10:34 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Ard Biesheuvel, Arvind Sankar, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Wed, Feb 03, 2021 at 11:51:48AM -0700, Nathan Chancellor wrote:
> x86_64 all{mod,yes}config with clang are going to ship broken in 5.11.

Dunno, it is still broken here even with those build assertions removed. And it
ain't even an all{mod,yes}config - just my machine's config with

CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y
CONFIG_UBSAN=y
# CONFIG_UBSAN_TRAP is not set
CONFIG_CC_HAS_UBSAN_BOUNDS=y
CONFIG_CC_HAS_UBSAN_ARRAY_BOUNDS=y
CONFIG_UBSAN_BOUNDS=y
CONFIG_UBSAN_ARRAY_BOUNDS=y
CONFIG_UBSAN_SHIFT=y
CONFIG_UBSAN_DIV_ZERO=y
CONFIG_UBSAN_SIGNED_OVERFLOW=y
CONFIG_UBSAN_UNSIGNED_OVERFLOW=y
CONFIG_UBSAN_OBJECT_SIZE=y
CONFIG_UBSAN_BOOL=y
CONFIG_UBSAN_ENUM=y
CONFIG_UBSAN_ALIGNMENT=y
CONFIG_UBSAN_SANITIZE_ALL=y
# CONFIG_TEST_UBSAN is not set

and clang-10:

lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x253: call to __ubsan_handle_add_overflow() with UACCESS enabled
lib/strnlen_user.o: warning: objtool: strnlen_user()+0x244: call to __ubsan_handle_add_overflow() with UACCESS enabled
ld: init/main.o: in function `kmalloc':
/home/boris/kernel/linux/./include/linux/slab.h:557: undefined reference to `__ubsan_handle_alignment_assumption'
ld: init/initramfs.o: in function `kmalloc':
/home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
ld: init/initramfs.o: in function `kmalloc_large':
/home/boris/kernel/linux/./include/linux/slab.h:481: undefined reference to `__ubsan_handle_alignment_assumption'
ld: init/initramfs.o: in function `kmalloc':
/home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
ld: /home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
ld: init/initramfs.o:/home/boris/kernel/linux/./include/linux/slab.h:552: more undefined references to `__ubsan_handle_alignment_assumption' follow
ld: mm/mremap.o: in function `get_extent':
/home/boris/kernel/linux/mm/mremap.c:355: undefined reference to `__compiletime_assert_327'
ld: mm/rmap.o: in function `anon_vma_chain_alloc':
/home/boris/kernel/linux/mm/rmap.c:136: undefined reference to `__ubsan_handle_alignment_assumption'
ld: mm/rmap.o: in function `anon_vma_alloc':
/home/boris/kernel/linux/mm/rmap.c:89: undefined reference to `__ubsan_handle_alignment_assumption'
ld: mm/rmap.o: in function `anon_vma_chain_alloc':
/home/boris/kernel/linux/mm/rmap.c:136: undefined reference to `__ubsan_handle_alignment_assumption'
ld: /home/boris/kernel/linux/mm/rmap.c:136: undefined reference to `__ubsan_handle_alignment_assumption'
ld: /home/boris/kernel/linux/mm/rmap.c:136: undefined reference to `__ubsan_handle_alignment_assumption'
ld: mm/vmalloc.o:/home/boris/kernel/linux/mm/vmalloc.c:1213: more undefined references to `__ubsan_handle_alignment_assumption' follow
make: *** [Makefile:1164: vmlinux] Error 1

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH] x86/efi: Remove EFI PGD build time checks
  2021-02-05  0:08                       ` Arvind Sankar
@ 2021-02-05 11:39                         ` Borislav Petkov
  2021-02-05 11:57                           ` Ard Biesheuvel
                                             ` (2 more replies)
  0 siblings, 3 replies; 36+ messages in thread
From: Borislav Petkov @ 2021-02-05 11:39 UTC (permalink / raw)
  To: Arvind Sankar, Ard Biesheuvel, Nathan Chancellor
  Cc: Arnd Bergmann, Thomas Gleixner, Ingo Molnar, X86 ML,
	Nathan Chancellor, Nick Desaulniers, Arnd Bergmann, Darren Hart,
	Andy Shevchenko, H. Peter Anvin, linux-efi, platform-driver-x86,
	Linux Kernel Mailing List, clang-built-linux, Kirill A. Shutemov

From: Borislav Petkov <bp@suse.de>

With CONFIG_X86_5LEVEL, CONFIG_UBSAN and CONFIG_UBSAN_UNSIGNED_OVERFLOW
enabled, clang fails the build with

  x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
  efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'

which happens due to -fsanitize=unsigned-integer-overflow being enabled:

  -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
  the result of an unsigned integer computation cannot be represented
  in its type. Unlike signed integer overflow, this is not undefined
  behavior, but it is often unintentional. This sanitizer does not check
  for lossy implicit conversions performed before such a computation
  (see -fsanitize=implicit-conversion).

and that fires when the (intentional) EFI_VA_START/END defines overflow
an unsigned long, leading to the assertion expressions not getting
optimized away (on GCC they do)...

However, those checks are superfluous: the runtime services mapping
code already makes sure the ranges don't overshoot EFI_VA_END as the
EFI mapping range is hardcoded. On each runtime services call, it is
switched to the EFI-specific PGD and even if mappings manage to escape
that last PGD, this won't remain unnoticed for long.

So rip them out.

See https://github.com/ClangBuiltLinux/linux/issues/256 for more info.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Link: http://lkml.kernel.org/r/20210107223424.4135538-1-arnd@kernel.org
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/platform/efi/efi_64.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index e1e8d4e3a213..8efd003540ca 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -115,31 +115,12 @@ void efi_sync_low_kernel_mappings(void)
 	pud_t *pud_k, *pud_efi;
 	pgd_t *efi_pgd = efi_mm.pgd;
 
-	/*
-	 * We can share all PGD entries apart from the one entry that
-	 * covers the EFI runtime mapping space.
-	 *
-	 * Make sure the EFI runtime region mappings are guaranteed to
-	 * only span a single PGD entry and that the entry also maps
-	 * other important kernel regions.
-	 */
-	MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
-	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
-			(EFI_VA_END & PGDIR_MASK));
-
 	pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
 	pgd_k = pgd_offset_k(PAGE_OFFSET);
 
 	num_entries = pgd_index(EFI_VA_END) - pgd_index(PAGE_OFFSET);
 	memcpy(pgd_efi, pgd_k, sizeof(pgd_t) * num_entries);
 
-	/*
-	 * As with PGDs, we share all P4D entries apart from the one entry
-	 * that covers the EFI runtime mapping space.
-	 */
-	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
-	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
-
 	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
 	pgd_k = pgd_offset_k(EFI_VA_END);
 	p4d_efi = p4d_offset(pgd_efi, 0);
-- 
2.29.2

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86/efi: Remove EFI PGD build time checks
  2021-02-05 11:39                         ` [PATCH] x86/efi: Remove EFI PGD build time checks Borislav Petkov
@ 2021-02-05 11:57                           ` Ard Biesheuvel
  2021-02-05 18:14                           ` Nick Desaulniers
  2021-02-05 18:56                           ` Nathan Chancellor
  2 siblings, 0 replies; 36+ messages in thread
From: Ard Biesheuvel @ 2021-02-05 11:57 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Arvind Sankar, Nathan Chancellor, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Fri, 5 Feb 2021 at 12:39, Borislav Petkov <bp@alien8.de> wrote:
>
> From: Borislav Petkov <bp@suse.de>
>
> With CONFIG_X86_5LEVEL, CONFIG_UBSAN and CONFIG_UBSAN_UNSIGNED_OVERFLOW
> enabled, clang fails the build with
>
>   x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
>   efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
>
> which happens due to -fsanitize=unsigned-integer-overflow being enabled:
>
>   -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
>   the result of an unsigned integer computation cannot be represented
>   in its type. Unlike signed integer overflow, this is not undefined
>   behavior, but it is often unintentional. This sanitizer does not check
>   for lossy implicit conversions performed before such a computation
>   (see -fsanitize=implicit-conversion).
>
> and that fires when the (intentional) EFI_VA_START/END defines overflow
> an unsigned long, leading to the assertion expressions not getting
> optimized away (on GCC they do)...
>
> However, those checks are superfluous: the runtime services mapping
> code already makes sure the ranges don't overshoot EFI_VA_END as the
> EFI mapping range is hardcoded. On each runtime services call, it is
> switched to the EFI-specific PGD and even if mappings manage to escape
> that last PGD, this won't remain unnoticed for long.
>
> So rip them out.
>
> See https://github.com/ClangBuiltLinux/linux/issues/256 for more info.
>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Link: http://lkml.kernel.org/r/20210107223424.4135538-1-arnd@kernel.org
> Signed-off-by: Borislav Petkov <bp@suse.de>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  arch/x86/platform/efi/efi_64.c | 19 -------------------
>  1 file changed, 19 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..8efd003540ca 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -115,31 +115,12 @@ void efi_sync_low_kernel_mappings(void)
>         pud_t *pud_k, *pud_efi;
>         pgd_t *efi_pgd = efi_mm.pgd;
>
> -       /*
> -        * We can share all PGD entries apart from the one entry that
> -        * covers the EFI runtime mapping space.
> -        *
> -        * Make sure the EFI runtime region mappings are guaranteed to
> -        * only span a single PGD entry and that the entry also maps
> -        * other important kernel regions.
> -        */
> -       MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
> -       MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> -                       (EFI_VA_END & PGDIR_MASK));
> -
>         pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
>         pgd_k = pgd_offset_k(PAGE_OFFSET);
>
>         num_entries = pgd_index(EFI_VA_END) - pgd_index(PAGE_OFFSET);
>         memcpy(pgd_efi, pgd_k, sizeof(pgd_t) * num_entries);
>
> -       /*
> -        * As with PGDs, we share all P4D entries apart from the one entry
> -        * that covers the EFI runtime mapping space.
> -        */
> -       BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> -
>         pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>         pgd_k = pgd_offset_k(EFI_VA_END);
>         p4d_efi = p4d_offset(pgd_efi, 0);
> --
> 2.29.2
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86/efi: Remove EFI PGD build time checks
  2021-02-05 11:39                         ` [PATCH] x86/efi: Remove EFI PGD build time checks Borislav Petkov
  2021-02-05 11:57                           ` Ard Biesheuvel
@ 2021-02-05 18:14                           ` Nick Desaulniers
  2021-02-05 18:56                           ` Nathan Chancellor
  2 siblings, 0 replies; 36+ messages in thread
From: Nick Desaulniers @ 2021-02-05 18:14 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Arvind Sankar, Ard Biesheuvel, Nathan Chancellor, Arnd Bergmann,
	Thomas Gleixner, Ingo Molnar, X86 ML, Nathan Chancellor,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Fri, Feb 5, 2021 at 3:39 AM Borislav Petkov <bp@alien8.de> wrote:
>
> From: Borislav Petkov <bp@suse.de>
>
> With CONFIG_X86_5LEVEL, CONFIG_UBSAN and CONFIG_UBSAN_UNSIGNED_OVERFLOW
> enabled, clang fails the build with
>
>   x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
>   efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
>
> which happens due to -fsanitize=unsigned-integer-overflow being enabled:
>
>   -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
>   the result of an unsigned integer computation cannot be represented
>   in its type. Unlike signed integer overflow, this is not undefined
>   behavior, but it is often unintentional. This sanitizer does not check
>   for lossy implicit conversions performed before such a computation
>   (see -fsanitize=implicit-conversion).
>
> and that fires when the (intentional) EFI_VA_START/END defines overflow
> an unsigned long, leading to the assertion expressions not getting
> optimized away (on GCC they do)...
>
> However, those checks are superfluous: the runtime services mapping
> code already makes sure the ranges don't overshoot EFI_VA_END as the
> EFI mapping range is hardcoded. On each runtime services call, it is
> switched to the EFI-specific PGD and even if mappings manage to escape
> that last PGD, this won't remain unnoticed for long.
>
> So rip them out.
>
> See https://github.com/ClangBuiltLinux/linux/issues/256 for more info.
>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Link: http://lkml.kernel.org/r/20210107223424.4135538-1-arnd@kernel.org
> Signed-off-by: Borislav Petkov <bp@suse.de>

Thanks, this fixes the failed assertion for me.

Tested-by: Nick Desaulniers <ndesaulniers@google.com>

(https://lore.kernel.org/lkml/20201230154104.522605-1-arnd@kernel.org/
is needed to finish a build of that configuration; going to chase that
next)

(consider applying Arvind's+Ard's suggested by tag)

> ---
>  arch/x86/platform/efi/efi_64.c | 19 -------------------
>  1 file changed, 19 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..8efd003540ca 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -115,31 +115,12 @@ void efi_sync_low_kernel_mappings(void)
>         pud_t *pud_k, *pud_efi;
>         pgd_t *efi_pgd = efi_mm.pgd;
>
> -       /*
> -        * We can share all PGD entries apart from the one entry that
> -        * covers the EFI runtime mapping space.
> -        *
> -        * Make sure the EFI runtime region mappings are guaranteed to
> -        * only span a single PGD entry and that the entry also maps
> -        * other important kernel regions.
> -        */
> -       MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
> -       MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> -                       (EFI_VA_END & PGDIR_MASK));
> -
>         pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
>         pgd_k = pgd_offset_k(PAGE_OFFSET);
>
>         num_entries = pgd_index(EFI_VA_END) - pgd_index(PAGE_OFFSET);
>         memcpy(pgd_efi, pgd_k, sizeof(pgd_t) * num_entries);
>
> -       /*
> -        * As with PGDs, we share all P4D entries apart from the one entry
> -        * that covers the EFI runtime mapping space.
> -        */
> -       BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -       BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> -
>         pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>         pgd_k = pgd_offset_k(EFI_VA_END);
>         p4d_efi = p4d_offset(pgd_efi, 0);
> --
> 2.29.2
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-05 10:34               ` [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Borislav Petkov
@ 2021-02-05 18:27                 ` Nick Desaulniers
  2021-02-05 18:31                   ` Nathan Chancellor
  0 siblings, 1 reply; 36+ messages in thread
From: Nick Desaulniers @ 2021-02-05 18:27 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Nathan Chancellor, Ard Biesheuvel, Arvind Sankar, Arnd Bergmann,
	Thomas Gleixner, Ingo Molnar, X86 ML, Nathan Chancellor,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov, Kees Cook

On Fri, Feb 5, 2021 at 2:35 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Feb 03, 2021 at 11:51:48AM -0700, Nathan Chancellor wrote:
> > x86_64 all{mod,yes}config with clang are going to ship broken in 5.11.
>
> Dunno, it is still broken here even with those build assertions removed. And it
> ain't even an all{mod,yes}config - just my machine's config with
>
> CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y
> CONFIG_UBSAN=y
> # CONFIG_UBSAN_TRAP is not set
> CONFIG_CC_HAS_UBSAN_BOUNDS=y
> CONFIG_CC_HAS_UBSAN_ARRAY_BOUNDS=y
> CONFIG_UBSAN_BOUNDS=y
> CONFIG_UBSAN_ARRAY_BOUNDS=y
> CONFIG_UBSAN_SHIFT=y
> CONFIG_UBSAN_DIV_ZERO=y
> CONFIG_UBSAN_SIGNED_OVERFLOW=y
> CONFIG_UBSAN_UNSIGNED_OVERFLOW=y
> CONFIG_UBSAN_OBJECT_SIZE=y
> CONFIG_UBSAN_BOOL=y
> CONFIG_UBSAN_ENUM=y
> CONFIG_UBSAN_ALIGNMENT=y
> CONFIG_UBSAN_SANITIZE_ALL=y
> # CONFIG_TEST_UBSAN is not set
>
> and clang-10:
>
> lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x253: call to __ubsan_handle_add_overflow() with UACCESS enabled
> lib/strnlen_user.o: warning: objtool: strnlen_user()+0x244: call to __ubsan_handle_add_overflow() with UACCESS enabled
> ld: init/main.o: in function `kmalloc':
> /home/boris/kernel/linux/./include/linux/slab.h:557: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: init/initramfs.o: in function `kmalloc':
> /home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: init/initramfs.o: in function `kmalloc_large':
> /home/boris/kernel/linux/./include/linux/slab.h:481: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: init/initramfs.o: in function `kmalloc':
> /home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: /home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: init/initramfs.o:/home/boris/kernel/linux/./include/linux/slab.h:552: more undefined references to `__ubsan_handle_alignment_assumption' follow
> ld: mm/mremap.o: in function `get_extent':
> /home/boris/kernel/linux/mm/mremap.c:355: undefined reference to `__compiletime_assert_327'

^ this one is https://lore.kernel.org/lkml/20201230154104.522605-1-arnd@kernel.org/.
Trying to get the last of these tracked down.  I think there were some
changes to UBSAN configs that weren't tested with clang before merged.

> ld: mm/rmap.o: in function `anon_vma_chain_alloc':
> /home/boris/kernel/linux/mm/rmap.c:136: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: mm/rmap.o: in function `anon_vma_alloc':
> /home/boris/kernel/linux/mm/rmap.c:89: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: mm/rmap.o: in function `anon_vma_chain_alloc':
> /home/boris/kernel/linux/mm/rmap.c:136: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: /home/boris/kernel/linux/mm/rmap.c:136: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: /home/boris/kernel/linux/mm/rmap.c:136: undefined reference to `__ubsan_handle_alignment_assumption'
> ld: mm/vmalloc.o:/home/boris/kernel/linux/mm/vmalloc.c:1213: more undefined references to `__ubsan_handle_alignment_assumption' follow
> make: *** [Makefile:1164: vmlinux] Error 1
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index
  2021-02-05 18:27                 ` Nick Desaulniers
@ 2021-02-05 18:31                   ` Nathan Chancellor
  0 siblings, 0 replies; 36+ messages in thread
From: Nathan Chancellor @ 2021-02-05 18:31 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Borislav Petkov, Ard Biesheuvel, Arvind Sankar, Arnd Bergmann,
	Thomas Gleixner, Ingo Molnar, X86 ML, Nathan Chancellor,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov, Kees Cook

On Fri, Feb 05, 2021 at 10:27:54AM -0800, Nick Desaulniers wrote:
> On Fri, Feb 5, 2021 at 2:35 AM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Wed, Feb 03, 2021 at 11:51:48AM -0700, Nathan Chancellor wrote:
> > > x86_64 all{mod,yes}config with clang are going to ship broken in 5.11.
> >
> > Dunno, it is still broken here even with those build assertions removed. And it
> > ain't even an all{mod,yes}config - just my machine's config with
> >
> > CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y
> > CONFIG_UBSAN=y
> > # CONFIG_UBSAN_TRAP is not set
> > CONFIG_CC_HAS_UBSAN_BOUNDS=y
> > CONFIG_CC_HAS_UBSAN_ARRAY_BOUNDS=y
> > CONFIG_UBSAN_BOUNDS=y
> > CONFIG_UBSAN_ARRAY_BOUNDS=y
> > CONFIG_UBSAN_SHIFT=y
> > CONFIG_UBSAN_DIV_ZERO=y
> > CONFIG_UBSAN_SIGNED_OVERFLOW=y
> > CONFIG_UBSAN_UNSIGNED_OVERFLOW=y
> > CONFIG_UBSAN_OBJECT_SIZE=y
> > CONFIG_UBSAN_BOOL=y
> > CONFIG_UBSAN_ENUM=y
> > CONFIG_UBSAN_ALIGNMENT=y
> > CONFIG_UBSAN_SANITIZE_ALL=y
> > # CONFIG_TEST_UBSAN is not set
> >
> > and clang-10:
> >
> > lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x253: call to __ubsan_handle_add_overflow() with UACCESS enabled
> > lib/strnlen_user.o: warning: objtool: strnlen_user()+0x244: call to __ubsan_handle_add_overflow() with UACCESS enabled
> > ld: init/main.o: in function `kmalloc':
> > /home/boris/kernel/linux/./include/linux/slab.h:557: undefined reference to `__ubsan_handle_alignment_assumption'
> > ld: init/initramfs.o: in function `kmalloc':
> > /home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
> > ld: init/initramfs.o: in function `kmalloc_large':
> > /home/boris/kernel/linux/./include/linux/slab.h:481: undefined reference to `__ubsan_handle_alignment_assumption'
> > ld: init/initramfs.o: in function `kmalloc':
> > /home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
> > ld: /home/boris/kernel/linux/./include/linux/slab.h:552: undefined reference to `__ubsan_handle_alignment_assumption'
> > ld: init/initramfs.o:/home/boris/kernel/linux/./include/linux/slab.h:552: more undefined references to `__ubsan_handle_alignment_assumption' follow
> > ld: mm/mremap.o: in function `get_extent':
> > /home/boris/kernel/linux/mm/mremap.c:355: undefined reference to `__compiletime_assert_327'
> 
> ^ this one is https://lore.kernel.org/lkml/20201230154104.522605-1-arnd@kernel.org/.
> Trying to get the last of these tracked down.  I think there were some
> changes to UBSAN configs that weren't tested with clang before merged.

The rest of these should be resolved by
https://lore.kernel.org/r/20210205023257.NJnJdyyZk%25akpm@linux-foundation.org/,
which is currently on its way to Linus.

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH] x86/efi: Remove EFI PGD build time checks
  2021-02-05 11:39                         ` [PATCH] x86/efi: Remove EFI PGD build time checks Borislav Petkov
  2021-02-05 11:57                           ` Ard Biesheuvel
  2021-02-05 18:14                           ` Nick Desaulniers
@ 2021-02-05 18:56                           ` Nathan Chancellor
  2 siblings, 0 replies; 36+ messages in thread
From: Nathan Chancellor @ 2021-02-05 18:56 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Arvind Sankar, Ard Biesheuvel, Arnd Bergmann, Thomas Gleixner,
	Ingo Molnar, X86 ML, Nathan Chancellor, Nick Desaulniers,
	Arnd Bergmann, Darren Hart, Andy Shevchenko, H. Peter Anvin,
	linux-efi, platform-driver-x86, Linux Kernel Mailing List,
	clang-built-linux, Kirill A. Shutemov

On Fri, Feb 05, 2021 at 12:39:30PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> With CONFIG_X86_5LEVEL, CONFIG_UBSAN and CONFIG_UBSAN_UNSIGNED_OVERFLOW
> enabled, clang fails the build with
> 
>   x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
>   efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'
> 
> which happens due to -fsanitize=unsigned-integer-overflow being enabled:
> 
>   -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
>   the result of an unsigned integer computation cannot be represented
>   in its type. Unlike signed integer overflow, this is not undefined
>   behavior, but it is often unintentional. This sanitizer does not check
>   for lossy implicit conversions performed before such a computation
>   (see -fsanitize=implicit-conversion).
> 
> and that fires when the (intentional) EFI_VA_START/END defines overflow
> an unsigned long, leading to the assertion expressions not getting
> optimized away (on GCC they do)...
> 
> However, those checks are superfluous: the runtime services mapping
> code already makes sure the ranges don't overshoot EFI_VA_END as the
> EFI mapping range is hardcoded. On each runtime services call, it is
> switched to the EFI-specific PGD and even if mappings manage to escape
> that last PGD, this won't remain unnoticed for long.
> 
> So rip them out.
> 
> See https://github.com/ClangBuiltLinux/linux/issues/256 for more info.
> 
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Link: http://lkml.kernel.org/r/20210107223424.4135538-1-arnd@kernel.org
> Signed-off-by: Borislav Petkov <bp@suse.de>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  arch/x86/platform/efi/efi_64.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index e1e8d4e3a213..8efd003540ca 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -115,31 +115,12 @@ void efi_sync_low_kernel_mappings(void)
>  	pud_t *pud_k, *pud_efi;
>  	pgd_t *efi_pgd = efi_mm.pgd;
>  
> -	/*
> -	 * We can share all PGD entries apart from the one entry that
> -	 * covers the EFI runtime mapping space.
> -	 *
> -	 * Make sure the EFI runtime region mappings are guaranteed to
> -	 * only span a single PGD entry and that the entry also maps
> -	 * other important kernel regions.
> -	 */
> -	MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
> -	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> -			(EFI_VA_END & PGDIR_MASK));
> -
>  	pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
>  	pgd_k = pgd_offset_k(PAGE_OFFSET);
>  
>  	num_entries = pgd_index(EFI_VA_END) - pgd_index(PAGE_OFFSET);
>  	memcpy(pgd_efi, pgd_k, sizeof(pgd_t) * num_entries);
>  
> -	/*
> -	 * As with PGDs, we share all P4D entries apart from the one entry
> -	 * that covers the EFI runtime mapping space.
> -	 */
> -	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> -	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
> -
>  	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
>  	pgd_k = pgd_offset_k(EFI_VA_END);
>  	p4d_efi = p4d_offset(pgd_efi, 0);
> -- 
> 2.29.2
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [tip: x86/urgent] x86/efi: Remove EFI PGD build time checks
  2021-01-07 22:34 [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Arnd Bergmann
                   ` (3 preceding siblings ...)
  2021-01-15 19:07 ` Arvind Sankar
@ 2021-02-06 12:56 ` tip-bot2 for Borislav Petkov
  4 siblings, 0 replies; 36+ messages in thread
From: tip-bot2 for Borislav Petkov @ 2021-02-06 12:56 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Arnd Bergmann, Borislav Petkov, Nathan Chancellor,
	Ard Biesheuvel, Nick Desaulniers, x86, linux-kernel

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     816ef8d7a2c4182e19bc06ab65751cb9e3951e94
Gitweb:        https://git.kernel.org/tip/816ef8d7a2c4182e19bc06ab65751cb9e3951e94
Author:        Borislav Petkov <bp@suse.de>
AuthorDate:    Fri, 05 Feb 2021 11:31:31 +01:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Sat, 06 Feb 2021 13:54:14 +01:00

x86/efi: Remove EFI PGD build time checks

With CONFIG_X86_5LEVEL, CONFIG_UBSAN and CONFIG_UBSAN_UNSIGNED_OVERFLOW
enabled, clang fails the build with

  x86_64-linux-ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
  efi_64.c:(.text+0x22c): undefined reference to `__compiletime_assert_354'

which happens due to -fsanitize=unsigned-integer-overflow being enabled:

  -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where
  the result of an unsigned integer computation cannot be represented
  in its type. Unlike signed integer overflow, this is not undefined
  behavior, but it is often unintentional. This sanitizer does not check
  for lossy implicit conversions performed before such a computation
  (see -fsanitize=implicit-conversion).

and that fires when the (intentional) EFI_VA_START/END defines overflow
an unsigned long, leading to the assertion expressions not getting
optimized away (on GCC they do)...

However, those checks are superfluous: the runtime services mapping
code already makes sure the ranges don't overshoot EFI_VA_END as the
EFI mapping range is hardcoded. On each runtime services call, it is
switched to the EFI-specific PGD and even if mappings manage to escape
that last PGD, this won't remain unnoticed for long.

So rip them out.

See https://github.com/ClangBuiltLinux/linux/issues/256 for more info.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Link: http://lkml.kernel.org/r/20210107223424.4135538-1-arnd@kernel.org
---
 arch/x86/platform/efi/efi_64.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index e1e8d4e..8efd003 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -115,31 +115,12 @@ void efi_sync_low_kernel_mappings(void)
 	pud_t *pud_k, *pud_efi;
 	pgd_t *efi_pgd = efi_mm.pgd;
 
-	/*
-	 * We can share all PGD entries apart from the one entry that
-	 * covers the EFI runtime mapping space.
-	 *
-	 * Make sure the EFI runtime region mappings are guaranteed to
-	 * only span a single PGD entry and that the entry also maps
-	 * other important kernel regions.
-	 */
-	MAYBE_BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
-	MAYBE_BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
-			(EFI_VA_END & PGDIR_MASK));
-
 	pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
 	pgd_k = pgd_offset_k(PAGE_OFFSET);
 
 	num_entries = pgd_index(EFI_VA_END) - pgd_index(PAGE_OFFSET);
 	memcpy(pgd_efi, pgd_k, sizeof(pgd_t) * num_entries);
 
-	/*
-	 * As with PGDs, we share all P4D entries apart from the one entry
-	 * that covers the EFI runtime mapping space.
-	 */
-	BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
-	BUILD_BUG_ON((EFI_VA_START & P4D_MASK) != (EFI_VA_END & P4D_MASK));
-
 	pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
 	pgd_k = pgd_offset_k(EFI_VA_END);
 	p4d_efi = p4d_offset(pgd_efi, 0);

^ permalink raw reply related	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2021-02-06 12:57 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-07 22:34 [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Arnd Bergmann
2021-01-07 22:42 ` Nathan Chancellor
2021-01-13 17:51 ` Ard Biesheuvel
2021-01-15 18:23 ` Borislav Petkov
2021-01-15 18:32   ` Nathan Chancellor
2021-01-15 19:07     ` Borislav Petkov
2021-01-15 19:11       ` Arvind Sankar
2021-01-15 19:18         ` Borislav Petkov
2021-01-15 19:54           ` Arnd Bergmann
2021-01-15 20:12             ` Arvind Sankar
2021-01-15 20:32               ` Arvind Sankar
2021-01-15 19:07 ` Arvind Sankar
2021-01-15 20:27   ` Arvind Sankar
2021-01-16 16:34     ` Ard Biesheuvel
2021-01-18 20:24       ` Borislav Petkov
2021-01-18 21:42         ` Arvind Sankar
2021-01-20  9:33           ` Ard Biesheuvel
2021-01-20 11:44             ` Borislav Petkov
2021-02-03 18:51             ` Nathan Chancellor
2021-02-03 20:29               ` Ard Biesheuvel
2021-02-04 10:51                 ` Borislav Petkov
2021-02-04 10:59                   ` Ard Biesheuvel
2021-02-04 19:16                   ` Nathan Chancellor
2021-02-04 21:43                   ` Arvind Sankar
2021-02-04 22:13                     ` Borislav Petkov
2021-02-05  0:08                       ` Arvind Sankar
2021-02-05 11:39                         ` [PATCH] x86/efi: Remove EFI PGD build time checks Borislav Petkov
2021-02-05 11:57                           ` Ard Biesheuvel
2021-02-05 18:14                           ` Nick Desaulniers
2021-02-05 18:56                           ` Nathan Chancellor
2021-02-05 10:34               ` [PATCH] x86: efi: avoid BUILD_BUG_ON() for non-constant p4d_index Borislav Petkov
2021-02-05 18:27                 ` Nick Desaulniers
2021-02-05 18:31                   ` Nathan Chancellor
2021-01-20 11:26           ` Kirill A. Shutemov
2021-01-20 11:06   ` Kirill A. Shutemov
2021-02-06 12:56 ` [tip: x86/urgent] x86/efi: Remove EFI PGD build time checks tip-bot2 for Borislav Petkov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.