diff mbox series

[v5,02/27] x86/build: Remove RWX sections and align on 4KB

Message ID edf3afbdcd87cb6c61815068084ac6de35be15a2.1678785672.git.baskov@ispras.ru
State New
Headers
Series x86_64: Improvements at compressed kernel stage |

Commit Message

Evgeniy Baskov March 14, 2023, 10:13 a.m. UTC 
  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"
segment and make ".init" segment non-executable.

Also add these segments to x86_32 architecture for consistency.
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.

Tested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 arch/x86/kernel/vmlinux.lds.S | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Borislav Petkov April 5, 2023, 5:40 p.m. UTC | #1 
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.
Gerd Hoffmann April 6, 2023, 11:42 a.m. UTC | #2 
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
Evgeniy Baskov April 8, 2023, 3:05 p.m. UTC | #3 
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 mbox series

Patch

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)