diff mbox series

[RFC,4/4] efi: x86: Split PE/COFF .text section into .text and .data

Message ID 20230308202209.2980947-5-ardb@kernel.org
State New
Headers show
Series efi: x86: Use strict W^X mappings in PE/COFF header | expand

Commit Message

Ard Biesheuvel March 8, 2023, 8:22 p.m. UTC 
Modern PE loader implementations used by EFI will honour the PE section
permission attributes, and so we can use them to avoid mappings that are
writable and executable at the same time.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/header.S      | 17 ++++++++++++++++
 arch/x86/boot/tools/build.c | 21 +++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

Comments

Evgeniy Baskov March 9, 2023, 6:02 p.m. UTC | #1 
On 2023-03-08 23:22, Ard Biesheuvel wrote:
> Modern PE loader implementations used by EFI will honour the PE section
> permission attributes, and so we can use them to avoid mappings that 
> are
> writable and executable at the same time.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/boot/header.S      | 17 ++++++++++++++++
>  arch/x86/boot/tools/build.c | 21 +++++++++++++++-----
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 4f1e1791cda4d316..a8ff8bbb17bca7d7 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -253,6 +253,23 @@ section_table:
>  		IMAGE_SCN_MEM_READ		| \
>  		IMAGE_SCN_MEM_EXECUTE		# Characteristics
> 
> +	.ascii	".data"
> +	.byte	0
> +	.byte	0
> +	.byte	0
> +	.long	0
> +	.long	0x0				# startup_{32,64}
> +	.long	0				# Size of initialized data
> +						# on disk
> +	.long	0x0				# startup_{32,64}
> +	.long	0				# PointerToRelocations
> +	.long	0				# PointerToLineNumbers
> +	.word	0				# NumberOfRelocations
> +	.word	0				# NumberOfLineNumbers
> +	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
> +		IMAGE_SCN_MEM_READ		| \
> +		IMAGE_SCN_MEM_WRITE		# Characteristics
> +
>  	.set	section_count, (. - section_table) / 40
>  #endif /* CONFIG_EFI_STUB */
> 
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index 883e6359221cd588..b449c82feaadf2b8 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -119,6 +119,7 @@ static unsigned long efi_boot_params;
>  static unsigned long kernel_info;
>  static unsigned long startup_64;
>  static unsigned long _ehead;
> +static unsigned long _data;
>  static unsigned long _end;
> 
>  
> /*----------------------------------------------------------------------*/
> @@ -347,10 +348,15 @@ static unsigned int
> update_pecoff_sections(unsigned int text_start, unsigned int
>  	init_sz	+= CONFIG_PHYSICAL_ALIGN;
> 
>  	/*
> -	 * Size of code: Subtract the size of the first sector (512 bytes)
> -	 * which includes the header.
> +	 * Size of code: the size of the combined .text/.rodata section, 
> which
> +	 * ends at the _data marker symbol.
>  	 */
> -	put_unaligned_le32(text_sz + bss_sz, &hdr->text_size);
> +	put_unaligned_le32(_data, &hdr->text_size);
> +
> +	/*
> +	 * Size of data: the size of the combined .data/.bss section.
> +	 */
> +	put_unaligned_le32(text_sz - _data + bss_sz, &hdr->data_size);
> 
>  	/* Size of image */
>  	put_unaligned_le32(init_sz, &hdr->image_size);
> @@ -360,9 +366,13 @@ static unsigned int
> update_pecoff_sections(unsigned int text_start, unsigned int
>  	 */
>  	put_unaligned_le32(text_start + efi_pe_entry, &hdr->entry_point);
> 
> -	update_pecoff_section_header_fields(".text", text_start, text_sz + 
> bss_sz,
> -					    text_sz, text_start);
> +	update_pecoff_section_header_fields(".text", text_start, _data,
> +					    _data, text_start);
> 
> +	update_pecoff_section_header_fields(".data", text_start + _data,
> +					    text_sz - _data + bss_sz,
> +					    text_sz - _data,
> +					    text_start + _data);
>  	return text_start + file_sz;
>  }
> 
> @@ -455,6 +465,7 @@ static void parse_zoffset(char *fname)
>  		PARSE_ZOFS(p, kernel_info);
>  		PARSE_ZOFS(p, startup_64);
>  		PARSE_ZOFS(p, _ehead);
> +		PARSE_ZOFS(p, _data);

This also requires _data to be fetched to zoffset.h:

diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index 8203f1a23f7a..0e5a18c3c165 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -91,7 +91,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE

  SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))

-sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] 
\(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define 
ZO_\2 0x\1/p'
+sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] 
\(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|_data\|z_.*\)$$/\#define 
ZO_\2 0x\1/p'

  quiet_cmd_zoffset = ZOFFSET $@
        cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@

>  		PARSE_ZOFS(p, _end);
> 
>  		p = strchr(p, '\n');
Ard Biesheuvel March 9, 2023, 6:03 p.m. UTC | #2 
On Thu, 9 Mar 2023 at 19:02, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> On 2023-03-08 23:22, Ard Biesheuvel wrote:
> > Modern PE loader implementations used by EFI will honour the PE section
> > permission attributes, and so we can use them to avoid mappings that
> > are
> > writable and executable at the same time.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/boot/header.S      | 17 ++++++++++++++++
> >  arch/x86/boot/tools/build.c | 21 +++++++++++++++-----
> >  2 files changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> > index 4f1e1791cda4d316..a8ff8bbb17bca7d7 100644
> > --- a/arch/x86/boot/header.S
> > +++ b/arch/x86/boot/header.S
> > @@ -253,6 +253,23 @@ section_table:
> >               IMAGE_SCN_MEM_READ              | \
> >               IMAGE_SCN_MEM_EXECUTE           # Characteristics
> >
> > +     .ascii  ".data"
> > +     .byte   0
> > +     .byte   0
> > +     .byte   0
> > +     .long   0
> > +     .long   0x0                             # startup_{32,64}
> > +     .long   0                               # Size of initialized data
> > +                                             # on disk
> > +     .long   0x0                             # startup_{32,64}
> > +     .long   0                               # PointerToRelocations
> > +     .long   0                               # PointerToLineNumbers
> > +     .word   0                               # NumberOfRelocations
> > +     .word   0                               # NumberOfLineNumbers
> > +     .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
> > +             IMAGE_SCN_MEM_READ              | \
> > +             IMAGE_SCN_MEM_WRITE             # Characteristics
> > +
> >       .set    section_count, (. - section_table) / 40
> >  #endif /* CONFIG_EFI_STUB */
> >
> > diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> > index 883e6359221cd588..b449c82feaadf2b8 100644
> > --- a/arch/x86/boot/tools/build.c
> > +++ b/arch/x86/boot/tools/build.c
> > @@ -119,6 +119,7 @@ static unsigned long efi_boot_params;
> >  static unsigned long kernel_info;
> >  static unsigned long startup_64;
> >  static unsigned long _ehead;
> > +static unsigned long _data;
> >  static unsigned long _end;
> >
> >
> > /*----------------------------------------------------------------------*/
> > @@ -347,10 +348,15 @@ static unsigned int
> > update_pecoff_sections(unsigned int text_start, unsigned int
> >       init_sz += CONFIG_PHYSICAL_ALIGN;
> >
> >       /*
> > -      * Size of code: Subtract the size of the first sector (512 bytes)
> > -      * which includes the header.
> > +      * Size of code: the size of the combined .text/.rodata section,
> > which
> > +      * ends at the _data marker symbol.
> >        */
> > -     put_unaligned_le32(text_sz + bss_sz, &hdr->text_size);
> > +     put_unaligned_le32(_data, &hdr->text_size);
> > +
> > +     /*
> > +      * Size of data: the size of the combined .data/.bss section.
> > +      */
> > +     put_unaligned_le32(text_sz - _data + bss_sz, &hdr->data_size);
> >
> >       /* Size of image */
> >       put_unaligned_le32(init_sz, &hdr->image_size);
> > @@ -360,9 +366,13 @@ static unsigned int
> > update_pecoff_sections(unsigned int text_start, unsigned int
> >        */
> >       put_unaligned_le32(text_start + efi_pe_entry, &hdr->entry_point);
> >
> > -     update_pecoff_section_header_fields(".text", text_start, text_sz +
> > bss_sz,
> > -                                         text_sz, text_start);
> > +     update_pecoff_section_header_fields(".text", text_start, _data,
> > +                                         _data, text_start);
> >
> > +     update_pecoff_section_header_fields(".data", text_start + _data,
> > +                                         text_sz - _data + bss_sz,
> > +                                         text_sz - _data,
> > +                                         text_start + _data);
> >       return text_start + file_sz;
> >  }
> >
> > @@ -455,6 +465,7 @@ static void parse_zoffset(char *fname)
> >               PARSE_ZOFS(p, kernel_info);
> >               PARSE_ZOFS(p, startup_64);
> >               PARSE_ZOFS(p, _ehead);
> > +             PARSE_ZOFS(p, _data);
>
> This also requires _data to be fetched to zoffset.h:
>

Indeed - I'd fixed that locally but failed to include it in the patch,
apologies.

> diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
> index 8203f1a23f7a..0e5a18c3c165 100644
> --- a/arch/x86/boot/Makefile
> +++ b/arch/x86/boot/Makefile
> @@ -91,7 +91,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
>
>   SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
>
> -sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z]
> \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define
> ZO_\2 0x\1/p'
> +sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z]
> \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|_data\|z_.*\)$$/\#define
> ZO_\2 0x\1/p'
>
>   quiet_cmd_zoffset = ZOFFSET $@
>         cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
>
> >               PARSE_ZOFS(p, _end);
> >
> >               p = strchr(p, '\n');
diff mbox series

Patch

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 4f1e1791cda4d316..a8ff8bbb17bca7d7 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -253,6 +253,23 @@  section_table:
 		IMAGE_SCN_MEM_READ		| \
 		IMAGE_SCN_MEM_EXECUTE		# Characteristics
 
+	.ascii	".data"
+	.byte	0
+	.byte	0
+	.byte	0
+	.long	0
+	.long	0x0				# startup_{32,64}
+	.long	0				# Size of initialized data
+						# on disk
+	.long	0x0				# startup_{32,64}
+	.long	0				# PointerToRelocations
+	.long	0				# PointerToLineNumbers
+	.word	0				# NumberOfRelocations
+	.word	0				# NumberOfLineNumbers
+	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
+		IMAGE_SCN_MEM_READ		| \
+		IMAGE_SCN_MEM_WRITE		# Characteristics
+
 	.set	section_count, (. - section_table) / 40
 #endif /* CONFIG_EFI_STUB */
 
diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 883e6359221cd588..b449c82feaadf2b8 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -119,6 +119,7 @@  static unsigned long efi_boot_params;
 static unsigned long kernel_info;
 static unsigned long startup_64;
 static unsigned long _ehead;
+static unsigned long _data;
 static unsigned long _end;
 
 /*----------------------------------------------------------------------*/
@@ -347,10 +348,15 @@  static unsigned int update_pecoff_sections(unsigned int text_start, unsigned int
 	init_sz	+= CONFIG_PHYSICAL_ALIGN;
 
 	/*
-	 * Size of code: Subtract the size of the first sector (512 bytes)
-	 * which includes the header.
+	 * Size of code: the size of the combined .text/.rodata section, which
+	 * ends at the _data marker symbol.
 	 */
-	put_unaligned_le32(text_sz + bss_sz, &hdr->text_size);
+	put_unaligned_le32(_data, &hdr->text_size);
+
+	/*
+	 * Size of data: the size of the combined .data/.bss section.
+	 */
+	put_unaligned_le32(text_sz - _data + bss_sz, &hdr->data_size);
 
 	/* Size of image */
 	put_unaligned_le32(init_sz, &hdr->image_size);
@@ -360,9 +366,13 @@  static unsigned int update_pecoff_sections(unsigned int text_start, unsigned int
 	 */
 	put_unaligned_le32(text_start + efi_pe_entry, &hdr->entry_point);
 
-	update_pecoff_section_header_fields(".text", text_start, text_sz + bss_sz,
-					    text_sz, text_start);
+	update_pecoff_section_header_fields(".text", text_start, _data,
+					    _data, text_start);
 
+	update_pecoff_section_header_fields(".data", text_start + _data,
+					    text_sz - _data + bss_sz,
+					    text_sz - _data,
+					    text_start + _data);
 	return text_start + file_sz;
 }
 
@@ -455,6 +465,7 @@  static void parse_zoffset(char *fname)
 		PARSE_ZOFS(p, kernel_info);
 		PARSE_ZOFS(p, startup_64);
 		PARSE_ZOFS(p, _ehead);
+		PARSE_ZOFS(p, _data);
 		PARSE_ZOFS(p, _end);
 
 		p = strchr(p, '\n');