| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87a57v9pt4.fsf@redhat.com>
Date: Fri, 02 May 2025 14:09:59 +0200
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: x86@...nel.org, linux-efi@...r.kernel.org, Thomas Gleixner
<tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Dave Hansen
<dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>, Peter
Jones <pjones@...hat.com>, Daniel Berrange <berrange@...hat.com>, Emanuele
Giuseppe Esposito <eesposit@...hat.com>, Gerd Hoffmann
<kraxel@...hat.com>, Greg KH <gregkh@...uxfoundation.org>, Luca Boccassi
<bluca@...ian.org>, Peter Zijlstra <peterz@...radead.org>, Matthew Garrett
<mjg59@...f.ucam.org>, James Bottomley
<James.Bottomley@...senpartnership.com>, Eric Snowberg
<eric.snowberg@...cle.com>, Paolo Bonzini <pbonzini@...hat.com>, Paul
Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] x86/efi: Implement support for embedding SBAT data
for x86
Ard Biesheuvel <ardb@...nel.org> writes:
> On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@...hat.com> wrote:
>>
>> Ard Biesheuvel <ardb@...nel.org> writes:
>>
>> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@...hat.com> wrote:
...
>> >> $(obj)/vmlinux: $(vmlinux-objs-y) $(vmlinux-libs-y) FORCE
>> >> $(call if_changed,ld)
>> >>
>> >> diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
>> >> index 3b2bc61c9408..d0a27905de90 100644
>> >> --- a/arch/x86/boot/compressed/vmlinux.lds.S
>> >> +++ b/arch/x86/boot/compressed/vmlinux.lds.S
>> >> @@ -49,9 +49,22 @@ SECTIONS
>> >> *(.data.*)
>> >>
>> >> /* Add 4 bytes of extra space for the obsolete CRC-32 checksum */
>> >> +#ifndef CONFIG_EFI_SBAT
>> >> . = ALIGN(. + 4, 0x200);
>> >> +#else
>> >> + /* Avoid gap between '.data' and '.sbat' */
>> >> + . = ALIGN(. + 4, 0x1000);
>> >> +#endif
>> >> _edata = . ;
>> >> }
>> >> +#ifdef CONFIG_EFI_SBAT
>> >> + .sbat : ALIGN(0x1000) {
>> >> + _sbat = . ;
>> >> + *(.sbat)
>> >> + _esbat = ALIGN(0x200);
>> >> + . = _esbat;
>> >> + }
>> >> +#endif
>> >> . = ALIGN(L1_CACHE_BYTES);
>> >> .bss : {
>> >> _bss = . ;
>> >
>> > This looks a bit odd - see below
>> >
>> >> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>> >> index b5c79f43359b..ab851490ef74 100644
>> >> --- a/arch/x86/boot/header.S
>> >> +++ b/arch/x86/boot/header.S
>> >> @@ -207,6 +207,19 @@ pecompat_fstart:
>> >> IMAGE_SCN_MEM_READ | \
>> >> IMAGE_SCN_MEM_WRITE # Characteristics
>> >>
>> >> +#ifdef CONFIG_EFI_SBAT
>> >> + .ascii ".sbat\0\0\0"
>> >> + .long ZO__esbat - ZO__sbat # VirtualSize
>> >> + .long setup_size + ZO__sbat # VirtualAddress
>> >> + .long ZO__esbat - ZO__sbat # SizeOfRawData
>> >> + .long setup_size + ZO__sbat # PointerToRawData
>> >> +
>> >> + .long 0, 0, 0
>> >> + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
>> >> + IMAGE_SCN_MEM_READ | \
>> >> + IMAGE_SCN_MEM_DISCARDABLE # Characteristics
>> >> +#endif
>> >> +
>> >
>> > This puts the .sbat section at the very end of the file. However, the
>> > virtual size of .data is 'ZO__end - ZO__data' not 'ZO__edata -
>> > ZO__data', and so the .sbat section will overlap with .bss in the
>> > memory view of the image.
>>
>> Missed that, will fix, thanks! A stupid question though: does this
>> matter in practice for SBAT? I don't think anyone needs SBAT data after
>> kernel starts booting so we can consider it 'discarded'. BSS data can
>> then do whatever it wants.
>>
>
> It violates the PE/COFF spec, and some PE loaders and signing tools
> are very pedantic about the layout.
Turns out it the problem is slightly harder to address then I initially
thought. On x86, arch/x86/boot/bzImage is composed of setup.bin and
vmlinux.bin and the later is produced from vmlinux with
objcopy -O binary -R .note -R .comment -S arch/x86/boot/compressed/vmlinux arch/x86/boot/vmlinux.bin
"objcopy -O binary" basically dumps memory representation of vmlinux. In
case we put '.sbat' before '.bss' we get:
Sections:
Idx Name Size VMA LMA File off Algn
...
4 .data 00002000 0000000000aef000 0000000000aef000 00af0000 2**12
CONTENTS, ALLOC, LOAD, DATA
5 .sbat 00001000 0000000000af1000 0000000000af1000 00af2000 2**12
CONTENTS, ALLOC, LOAD, READONLY, DATA
6 .bss 00023078 0000000000af2000 0000000000af2000 00af3000 2**12
ALLOC
7 .pgtable 00021000 0000000000b16000 0000000000b16000 00af3000 2**12
ALLOC
...
so we can't have a single '.data' section in PE covering both '.data'
and '.bss'/'.pgtable' as '.sbat' is in the middle and we want it to be a
separate section. If we try putting '.sbat' after '.bss' we are going to
get a hole size of '.bss' + '.pgtable' in arch/x86/boot/vmlinux.bin
because 'objcopy -O binary' is not going to squeeze anything (and it has
no idea what '.sbat' is and if it can be moved).
The problem is similar for zboot. I have two ideas:
1) Get back to the idea of putting '.sbat' between '.text' and '.data'
(was in my RFC).
2) Introduce a separate '.bss' section to the PE binary, basically:
diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index b5c79f43359b..dae8202705c4 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -197,7 +197,7 @@ pecompat_fstart:
IMAGE_SCN_MEM_EXECUTE # Characteristics
.ascii ".data\0\0\0"
- .long ZO__end - ZO__data # VirtualSize
+ .long ZO__edata - ZO__data # VirtualSize
.long setup_size + ZO__data # VirtualAddress
.long ZO__edata - ZO__data # SizeOfRawData
.long setup_size + ZO__data # PointerToRawData
@@ -207,6 +207,17 @@ pecompat_fstart:
IMAGE_SCN_MEM_READ | \
IMAGE_SCN_MEM_WRITE # Characteristics
+ .ascii ".bss\0\0\0\0"
+ .long ZO__end - ZO__edata # VirtualSize
+ .long setup_size + ZO__edata # VirtualAddress
+ .long 0 # SizeOfRawData
+ .long 0 # PointerToRawData
+
+ .long 0, 0, 0
+ .long IMAGE_SCN_CNT_UNINITIALIZED_DATA| \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_WRITE # Characteristics
+
.set section_count, (. - section_table) / 40
#endif /* CONFIG_EFI_STUB */
This way '.data' doesn't need to be the last meaninful section in
'vmlinux' and we can then pub '.sbat' right after it. I'm going to give
it a try but also I'm wondering why do we have '.data' covering '.bss'
and '.pgtable' in the first place.
--
Vitaly
Powered by blists - more mailing lists