Re: [PATCH 14/19] ARM: linker script: create separate PT_LOAD segments for text, rodata, and data

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 1/5/26 12:26 PM, Sascha Hauer wrote:
> Fix the linker scripts to generate three distinct PT_LOAD segments with
> correct permissions instead of combining .rodata with .data.
> 
> Before this fix, the linker auto-generated only two PT_LOAD segments:
> 1. Text segment (PF_R|PF_X)
> 2. Data segment (PF_R|PF_W) - containing .rodata, .data, .bss, etc.

Did it though? Why did we get the RWX linker warnings then?

> This caused .rodata to be mapped with write permissions when
> pbl_mmu_setup_from_elf() set up MMU permissions based on ELF segments,
> defeating the W^X protection that commit d9ccb0cf14 intended to provide.

What commit hash is this?

> With explicit PHDRS directives, we now generate three segments:
> 1. text segment (PF_R|PF_X): .text and related code sections
> 2. rodata segment (PF_R): .rodata and unwind tables
> 3. data segment (PF_R|PF_W): .data, .bss, and related sections

Not directly related, but this is as good a place as any to ask the
question: How is zero-padding implemented? If the file size is shorter
than the memory size, the loader is supposed to zero-fill, which is used
for BSS zeroing for example. Now if you load the ELF in place, we can't
obviously zero-fill. This is fine, but we should have a check somewhere
to make absolutely sure that we don't end up with too short segments.


> -#define BAREBOX_RELOCATION_TABLE					\
> -	.rel_dyn_start : { *(.__rel_dyn_start) }			\
> -	.BAREBOX_RELOCATION_TYPE.dyn : { *(.BAREBOX_RELOCATION_TYPE*) }	\
> -	.rel_dyn_end : { *(.__rel_dyn_end) }				\
> -	.__dynsym_start :  { *(.__dynsym_start) }			\
> -	.dynsym : { *(.dynsym) }					\
> -	.__dynsym_end : { *(.__dynsym_end) }
> +#define BAREBOX_RELOCATION_TABLE(PHDR)					\
> +	.rel_dyn_start : { *(.__rel_dyn_start) } PHDR			\
> +	.BAREBOX_RELOCATION_TYPE.dyn : { *(.BAREBOX_RELOCATION_TYPE*) }	PHDR \
> +	.rel_dyn_end : { *(.__rel_dyn_end) } PHDR			\
> +	.__dynsym_start :  { *(.__dynsym_start) } PHDR			\
> +	.dynsym : { *(.dynsym) } PHDR					\
> +	.__dynsym_end : { *(.__dynsym_end) } PHDR

Quoting
https://www.sourceware.org/binutils/docs/ld/Output-Section-Phdr.html:

If a section is assigned to one or more segments, then all subsequent
allocated sections will be assigned to those segments as well, unless
they use an explicitly :phdr modifier. You can use :NONE to tell the
linker to not put the section in any segment at all.

This whole hunk is thus unnecessary as it follows the data section.

> +PHDRS
> +{
> +	text PT_LOAD FLAGS(5);     /* PF_R | PF_X */
> +	rodata PT_LOAD FLAGS(4);   /* PF_R */
> +	data PT_LOAD FLAGS(6);     /* PF_R | PF_W */
> +	dynamic PT_DYNAMIC FLAGS(6); /* PF_R | PF_W */

I believe we don't need PF_W for PT_DYNAMIC. You could move it one up to
merge with rodata.

> +}
> +
>  SECTIONS
>  {
>  	. = 0x0;
> -	.image_start : { *(.__image_start) }
> +	.image_start : { *(.__image_start) } :text
>  
>  	. = ALIGN(4);
>  
> -	._text : { *(._text) }
> +	._text : { *(._text) } :text
>  	.text      :
>  	{
>  		_stext = .;
> @@ -31,7 +40,7 @@ SECTIONS
>  		KEEP(*(.text_inplace_exceptions*))
>  		__inplace_exceptions_stop = .;
>  		*(.text*)
> -	}
> +	} :text
>  	BAREBOX_BARE_INIT_SIZE
>  
>  	. = ALIGN(4096);
> @@ -39,7 +48,7 @@ SECTIONS
>  	.rodata : {
>  		*(.rodata*)
>  		RO_DATA_SECTION
> -	}
> +	} :rodata
>  
>  #ifdef CONFIG_ARM_UNWIND
>  	/*
> @@ -50,12 +59,12 @@ SECTIONS
>  		__start_unwind_idx = .;
>  		*(.ARM.exidx*)
>  		__stop_unwind_idx = .;
> -	}
> +	} :rodata
>  	.ARM.unwind_tab : {
>  		__start_unwind_tab = .;
>  		*(.ARM.extab*)
>  		__stop_unwind_tab = .;
> -	}
> +	} :rodata
>  #endif
>  	. = ALIGN(4096);
>  	__end_rodata = .;
> @@ -65,19 +74,21 @@ SECTIONS
>  	. = ALIGN(4);
>  	.data : { *(.data*)
>  		CONSTRUCTORS
> -	}
> +	} :data
> +
> +	.dynamic : { *(.dynamic) } :data :dynamic

As mentioned above. s/:data/:rodata/ and move up?

>  
>  	. = .;
>  
> -	BAREBOX_RELOCATION_TABLE
> +	BAREBOX_RELOCATION_TABLE(:data)
>  
>  	_edata = .;
> -	.image_end : { *(.__image_end) }
> +	.image_end : { *(.__image_end) } :data
>  
>  	. = ALIGN(4);
> -	.__bss_start :  { *(.__bss_start) }
> -	.bss : { *(.bss*) }
> -	.__bss_stop :  { *(.__bss_stop) }
> +	.__bss_start :  { *(.__bss_start) } :data
> +	.bss : { *(.bss*) } :data
> +	.__bss_stop :  { *(.__bss_stop) } :data

Side-effect of having the decompressor take care of zeroing BSS is a big
size increase for CONFIG_IMAGE_COMPRESSION_NONE. I think that's
acceptable, but it's worth a comment here why we don't have a separate
BSS segment (or make bss NOALLOC).

FYI, I have patches to get rid of MAX_BSS_SIZE on top of PBL ELF loader
support, which I will submit once this support is merged.

Cheers,
Ahmad

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |