| Message ID | edf3afbdcd87cb6c61815068084ac6de35be15a2.1678785672.git.baskov@ispras.ru |
|---|---|
| State | New |
| Headers | show |
| Series | x86_64: Improvements at compressed kernel stage | expand |
On Tue, Mar 14, 2023 at 01:13:29PM +0300, Evgeniy Baskov wrote: > Avoid creating sections simultaneously writable and readable to prepare > for W^X implementation for the kernel itself (not the decompressor). > Align kernel sections on page size (4KB) to allow protecting them in the > page tables. > > Split init code form ".init" segment into separate R_X ".inittext" s/form/from/ > segment and make ".init" segment non-executable. "... and make the .init segment RW_." > Also add these segments to x86_32 architecture for consistency. Same comment as before: please refrain from talking about the *what* in a commit message but about the *why*. And considering the matter, you have a *lot* of *why* to talk about. :-) Pls check your whole set. > Currently paging is disabled in x86_32 in compressed kernel, so > protection is not applied anyways, but .init code was incorrectly > placed in non-executable ".data" segment. This should not change > anything meaningful in memory layout now, but might be required in case > memory protection will also be implemented in compressed kernel for > x86_32. I highly doubt that - no one cares about 32-bit x86 anymore. > @@ -226,9 +225,10 @@ SECTIONS > #endif > > INIT_TEXT_SECTION(PAGE_SIZE) > -#ifdef CONFIG_X86_64 > - :init > -#endif > + :inittext > + > + . = ALIGN(PAGE_SIZE); > + > > /* > * Section for code used exclusively before alternatives are run. All > @@ -240,6 +240,7 @@ SECTIONS > .altinstr_aux : AT(ADDR(.altinstr_aux) - LOAD_OFFSET) { > *(.altinstr_aux) > } > + :init Why isn't this placed after inittext but here? I'm thinking you wanna have: :inittext . = ALIGN.. :init <rest> Thx.
Hi, > > Currently paging is disabled in x86_32 in compressed kernel, so > > protection is not applied anyways, but .init code was incorrectly > > placed in non-executable ".data" segment. This should not change > > anything meaningful in memory layout now, but might be required in case > > memory protection will also be implemented in compressed kernel for > > x86_32. > > I highly doubt that - no one cares about 32-bit x86 anymore. Indeed. ia32 edk2 runs without paging even in latest tianocore/edk2, and I don't expect that to change until ia32 support gets removed. take care, Gerd
On 2023-04-05 20:40, Borislav Petkov wrote: > On Tue, Mar 14, 2023 at 01:13:29PM +0300, Evgeniy Baskov wrote: >> Avoid creating sections simultaneously writable and readable to >> prepare >> for W^X implementation for the kernel itself (not the decompressor). >> Align kernel sections on page size (4KB) to allow protecting them in >> the >> page tables. >> >> Split init code form ".init" segment into separate R_X ".inittext" > > s/form/from/ Thanks! > >> segment and make ".init" segment non-executable. > > "... and make the .init segment RW_." Will fix. > >> Also add these segments to x86_32 architecture for consistency. > > Same comment as before: please refrain from talking about the *what* in > a commit message but about the *why*. > > And considering the matter, you have a *lot* of *why* to talk about. > :-) > > Pls check your whole set. I'll try do make descriptions of patches more elaborate and to better reflect the reasoning behind the changes before resubmitting, thanks. > >> Currently paging is disabled in x86_32 in compressed kernel, so >> protection is not applied anyways, but .init code was incorrectly >> placed in non-executable ".data" segment. This should not change >> anything meaningful in memory layout now, but might be required in >> case >> memory protection will also be implemented in compressed kernel for >> x86_32. > > I highly doubt that - no one cares about 32-bit x86 anymore. > True, but in theory it's still possible and also the change makes things more correct. >> @@ -226,9 +225,10 @@ SECTIONS >> #endif >> >> INIT_TEXT_SECTION(PAGE_SIZE) >> -#ifdef CONFIG_X86_64 >> - :init >> -#endif >> + :inittext >> + >> + . = ALIGN(PAGE_SIZE); >> + >> >> /* >> * Section for code used exclusively before alternatives are run. >> All >> @@ -240,6 +240,7 @@ SECTIONS >> .altinstr_aux : AT(ADDR(.altinstr_aux) - LOAD_OFFSET) { >> *(.altinstr_aux) >> } >> + :init > > Why isn't this placed after inittext but here? Because, AFAIK, :init is a part of a section syntax so it must come after the brace, at least according to the documentation: https://sourceware.org/binutils/docs/ld/PHDRS.html > > I'm thinking you wanna have: > > :inittext > . = ALIGN.. > :init > <rest> > > Thx.
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S index 25f155205770..81ea1236d293 100644 --- a/arch/x86/kernel/vmlinux.lds.S +++ b/arch/x86/kernel/vmlinux.lds.S @@ -102,12 +102,11 @@ jiffies = jiffies_64; PHDRS { text PT_LOAD FLAGS(5); /* R_E */ data PT_LOAD FLAGS(6); /* RW_ */ -#ifdef CONFIG_X86_64 -#ifdef CONFIG_SMP +#if defined(CONFIG_X86_64) && defined(CONFIG_SMP) percpu PT_LOAD FLAGS(6); /* RW_ */ #endif - init PT_LOAD FLAGS(7); /* RWE */ -#endif + inittext PT_LOAD FLAGS(5); /* R_E */ + init PT_LOAD FLAGS(6); /* RW_ */ note PT_NOTE FLAGS(0); /* ___ */ } @@ -226,9 +225,10 @@ SECTIONS #endif INIT_TEXT_SECTION(PAGE_SIZE) -#ifdef CONFIG_X86_64 - :init -#endif + :inittext + + . = ALIGN(PAGE_SIZE); + /* * Section for code used exclusively before alternatives are run. All @@ -240,6 +240,7 @@ SECTIONS .altinstr_aux : AT(ADDR(.altinstr_aux) - LOAD_OFFSET) { *(.altinstr_aux) } + :init INIT_DATA_SECTION(16)