Re: [PATCH v4 0/7] x86: Rid .head.text of all abs references

Re: [PATCH v4 0/7] x86: Rid .head.text of all abs references

[Borislav Petkov]

+ linux-toolchains.

Hi Ard,

On Thu, Dec 05, 2024 at 12:28:05PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> This series removes the last remaining absolute symbol references from
> .head.text. Doing so is necessary because code in this section may be
> called from a 1:1 mapping of memory, which deviates from the mapping
> this code was linked and/or relocated to run at. This is not something
> that the toolchains support: even PIC/PIE code is still assumed to
> execute from the same mapping that it was relocated to run from by the
> startup code or dynamic loader. This means we are basically on our own
> here, and need to add measures to ensure the code works as expected in
> this manner.
> 
> Given that the startup code needs to create the kernel virtual mapping
> in the page tables, early references to some kernel virtual addresses
> are valid even if they cannot be dereferenced yet. To avoid having to
> make this distinction at build time, patches #2 and #3 replace such
> valid references with RIP-relative references with an offset applied.
> 
> Patch #1 removes some absolute references from .head.text that don't
> need to be there in the first place.

dunno if you've seen this already and maybe it is not related but the error
message said ".head.text"...

Absolute reference to symbol '.data' not permitted in .head.text
make[3]: *** [arch/x86/Makefile.postlink:32: vmlinux] Error 1
make[2]: *** [scripts/Makefile.vmlinux:77: vmlinux] Error 2
make[2]: *** Deleting file 'vmlinux'
make[1]: *** [/home/amd/bpetkov/kernel/linux/Makefile:1225: vmlinux] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:251: __sub-make] Error 2

That's an allmodconfig with

Ubuntu clang version 14.0.0-1ubuntu1.1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 0/7] x86: Rid .head.text of all abs references

[Ard Biesheuvel]

On Tue, 31 Dec 2024 at 11:02, Borislav Petkov <bp@alien8.de> wrote:
>
> + linux-toolchains.
>
> Hi Ard,
>
> On Thu, Dec 05, 2024 at 12:28:05PM +0100, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > This series removes the last remaining absolute symbol references from
> > .head.text. Doing so is necessary because code in this section may be
> > called from a 1:1 mapping of memory, which deviates from the mapping
> > this code was linked and/or relocated to run at. This is not something
> > that the toolchains support: even PIC/PIE code is still assumed to
> > execute from the same mapping that it was relocated to run from by the
> > startup code or dynamic loader. This means we are basically on our own
> > here, and need to add measures to ensure the code works as expected in
> > this manner.
> >
> > Given that the startup code needs to create the kernel virtual mapping
> > in the page tables, early references to some kernel virtual addresses
> > are valid even if they cannot be dereferenced yet. To avoid having to
> > make this distinction at build time, patches #2 and #3 replace such
> > valid references with RIP-relative references with an offset applied.
> >
> > Patch #1 removes some absolute references from .head.text that don't
> > need to be there in the first place.
>
> dunno if you've seen this already and maybe it is not related but the error
> message said ".head.text"...
>
> Absolute reference to symbol '.data' not permitted in .head.text
> make[3]: *** [arch/x86/Makefile.postlink:32: vmlinux] Error 1
> make[2]: *** [scripts/Makefile.vmlinux:77: vmlinux] Error 2
> make[2]: *** Deleting file 'vmlinux'
> make[1]: *** [/home/amd/bpetkov/kernel/linux/Makefile:1225: vmlinux] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:251: __sub-make] Error 2
>
> That's an allmodconfig with
>
> Ubuntu clang version 14.0.0-1ubuntu1.1
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin
>

This is definitely related, and likely means the new code is working
as expected, and flagging an absolute reference emitted by, e.g., one
of the sanitizers that will blow up if it ever gets dereferenced.

I'll look into this asap, i.e., in a couple of days.

Re: [PATCH v4 0/7] x86: Rid .head.text of all abs references

[Borislav Petkov]

On Tue, Dec 31, 2024 at 11:12:55AM +0100, Ard Biesheuvel wrote:
> I'll look into this asap, i.e., in a couple of days.

:-P

Thanks!

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 0/7] x86: Rid .head.text of all abs references

[Ard Biesheuvel]

(cc Nathan)

On Tue, 31 Dec 2024 at 11:35, Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Dec 31, 2024 at 11:12:55AM +0100, Ard Biesheuvel wrote:
> > I'll look into this asap, i.e., in a couple of days.
>
> :-P
>
> Thanks!
>

I had a quick look, and managed to reproduce it with Clang 14 but not
with Clang 18.

It looks like UBSAN is emitting some instrumentation here, in spite of
the __no_sanitize_undefined annotation (via __head) on
pvalidate_4k_page():

arch/x86/coco/sev/core.o:

0000000000000a00 <pvalidate_4k_page>:
 ...
 b72:   40 88 de                mov    %bl,%sil
 b75:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
        b78: R_X86_64_32S       .data+0xb0
 b7c:   e8 00 00 00 00          callq  b81 <pvalidate_4k_page+0x181>
        b7d: R_X86_64_PLT32     __ubsan_handle_load_invalid_value-0x4

So as far as this series is concerned, things are working correctly,
and an absolute reference to .data is being flagged in code that may
execute before the absolute address in question is even mapped.

Re: [PATCH v4 0/7] x86: Rid .head.text of all abs references

[Nathan Chancellor]

Hi Ard,

On Tue, Dec 31, 2024 at 08:29:17PM +0100, Ard Biesheuvel wrote:
> (cc Nathan)

Thanks for the CC.

> On Tue, 31 Dec 2024 at 11:35, Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Tue, Dec 31, 2024 at 11:12:55AM +0100, Ard Biesheuvel wrote:
> > > I'll look into this asap, i.e., in a couple of days.
> >
> > :-P
> >
> > Thanks!
> >
> 
> I had a quick look, and managed to reproduce it with Clang 14 but not
> with Clang 18.
> 
> It looks like UBSAN is emitting some instrumentation here, in spite of
> the __no_sanitize_undefined annotation (via __head) on
> pvalidate_4k_page():
> 
> arch/x86/coco/sev/core.o:
> 
> 0000000000000a00 <pvalidate_4k_page>:
>  ...
>  b72:   40 88 de                mov    %bl,%sil
>  b75:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
>         b78: R_X86_64_32S       .data+0xb0
>  b7c:   e8 00 00 00 00          callq  b81 <pvalidate_4k_page+0x181>
>         b7d: R_X86_64_PLT32     __ubsan_handle_load_invalid_value-0x4
> 
> So as far as this series is concerned, things are working correctly,
> and an absolute reference to .data is being flagged in code that may
> execute before the absolute address in question is even mapped.

It appears that this is related to UBSAN_BOOL. This is reproducible with
just:

$ echo 'CONFIG_AMD_MEM_ENCRYPT=y
CONFIG_UBSAN=y
CONFIG_UBSAN_BOOL=y
# CONFIG_UBSAN_ALIGNMENT is not set
# CONFIG_UBSAN_BOUNDS is not set
# CONFIG_UBSAN_DIV_ZERO is not set
# CONFIG_UBSAN_ENUM is not set
# CONFIG_UBSAN_SIGNED_WRAP is not set
# CONFIG_UBSAN_SHIFT is not set
# CONFIG_UBSAN_TRAP is not set
# CONFIG_UBSAN_UNREACHABLE is not set' >kernel/configs/repro.config

$ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig repro.config vmlinux
Absolute reference to symbol '.data' not permitted in .head.text
make[5]: *** [arch/x86/Makefile.postlink:32: vmlinux] Error 1
...

Given that this appears in LLVM 14 but not LLVM 15 and newer, I reverse
bisected the fix in LLVM to [1], which was actually a fix from a report
from Linus [2]. That seems like a reasonable change to blame, as UBSAN
is generating this check from the asm() in pvalidate() and after the
LLVM fix, that check is no longer generated.

It does seem fishy that __no_sanitize_undefined does not prevent the
generation of that check... Plugging Linus's original reproducer from
[2] into Compiler Explorer [3], it seems like __no_sanitize_undefined
does get respected. It is my understanding that inlining functions that
do not have attributes that disable instrumentation into ones that do is
supposed to remove the instrumentation, correct? It seems like
pvalidate() does get inlined into pvalidate_4k_page() but the
instrumentation remains. Explicitly adding __no_sanitize_undefined to
pvalidate() hides this for me.

[1]: https://github.com/llvm/llvm-project/commit/92c1bc61586c9d6c7bf0c36b1005fe00b4f48cc0
[2]: https://github.com/llvm/llvm-project/issues/56568
[3]: https://godbolt.org/z/cxhW5orxr

Cheers,
Nathan

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 91f08af31078..7887bac1fbab 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -414,7 +414,7 @@ static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long a
 
 	return rc;
 }
-static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate)
+static inline __no_sanitize_undefined int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate)
 {
 	bool no_rmpupdate;
 	int rc;

Re: [PATCH v4 0/7] x86: Rid .head.text of all abs references

[Ard Biesheuvel]

On Wed, 1 Jan 2025 at 03:43, Nathan Chancellor <nathan@kernel.org> wrote:
>
> Hi Ard,
>
> On Tue, Dec 31, 2024 at 08:29:17PM +0100, Ard Biesheuvel wrote:
> > (cc Nathan)
>
> Thanks for the CC.
>
> > On Tue, 31 Dec 2024 at 11:35, Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > On Tue, Dec 31, 2024 at 11:12:55AM +0100, Ard Biesheuvel wrote:
> > > > I'll look into this asap, i.e., in a couple of days.
> > >
> > > :-P
> > >
> > > Thanks!
> > >
> >
> > I had a quick look, and managed to reproduce it with Clang 14 but not
> > with Clang 18.
> >
> > It looks like UBSAN is emitting some instrumentation here, in spite of
> > the __no_sanitize_undefined annotation (via __head) on
> > pvalidate_4k_page():
> >
> > arch/x86/coco/sev/core.o:
> >
> > 0000000000000a00 <pvalidate_4k_page>:
> >  ...
> >  b72:   40 88 de                mov    %bl,%sil
> >  b75:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
> >         b78: R_X86_64_32S       .data+0xb0
> >  b7c:   e8 00 00 00 00          callq  b81 <pvalidate_4k_page+0x181>
> >         b7d: R_X86_64_PLT32     __ubsan_handle_load_invalid_value-0x4
> >
> > So as far as this series is concerned, things are working correctly,
> > and an absolute reference to .data is being flagged in code that may
> > execute before the absolute address in question is even mapped.
>
> It appears that this is related to UBSAN_BOOL. This is reproducible with
> just:
>
> $ echo 'CONFIG_AMD_MEM_ENCRYPT=y
> CONFIG_UBSAN=y
> CONFIG_UBSAN_BOOL=y
> # CONFIG_UBSAN_ALIGNMENT is not set
> # CONFIG_UBSAN_BOUNDS is not set
> # CONFIG_UBSAN_DIV_ZERO is not set
> # CONFIG_UBSAN_ENUM is not set
> # CONFIG_UBSAN_SIGNED_WRAP is not set
> # CONFIG_UBSAN_SHIFT is not set
> # CONFIG_UBSAN_TRAP is not set
> # CONFIG_UBSAN_UNREACHABLE is not set' >kernel/configs/repro.config
>
> $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig repro.config vmlinux
> Absolute reference to symbol '.data' not permitted in .head.text
> make[5]: *** [arch/x86/Makefile.postlink:32: vmlinux] Error 1
> ...
>
> Given that this appears in LLVM 14 but not LLVM 15 and newer, I reverse
> bisected the fix in LLVM to [1], which was actually a fix from a report
> from Linus [2]. That seems like a reasonable change to blame, as UBSAN
> is generating this check from the asm() in pvalidate() and after the
> LLVM fix, that check is no longer generated.
>
> It does seem fishy that __no_sanitize_undefined does not prevent the
> generation of that check... Plugging Linus's original reproducer from
> [2] into Compiler Explorer [3], it seems like __no_sanitize_undefined
> does get respected. It is my understanding that inlining functions that
> do not have attributes that disable instrumentation into ones that do is
> supposed to remove the instrumentation, correct? It seems like
> pvalidate() does get inlined into pvalidate_4k_page() but the
> instrumentation remains. Explicitly adding __no_sanitize_undefined to
> pvalidate() hides this for me.
>

Thanks for the analysis.

Should we perhaps just add the below? All the other sanitizers are
already disabled for core.o, which is the only object file being built
in this sub-directory.

--- a/arch/x86/coco/sev/Makefile
+++ b/arch/x86/coco/sev/Makefile
@@ -13,3 +13,5 @@ KCOV_INSTRUMENT_core.o        := n
 # With some compiler versions the generated code results in boot hangs, caused
 # by several compilation units. To be safe, disable all instrumentation.
 KCSAN_SANITIZE         := n
+
+UBSAN_SANITIZE         := n

Re: [PATCH v4 0/7] x86: Rid .head.text of all abs references

[Borislav Petkov]

On Wed, Jan 01, 2025 at 09:01:49AM +0100, Ard Biesheuvel wrote:
> Thanks for the analysis.

Ditto.

> Should we perhaps just add the below? All the other sanitizers are
> already disabled for core.o, which is the only object file being built
> in this sub-directory.

Yes, please.

Thx.

-- 
Regards/Gruss,
    Boris.

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