Re: [PATCH 5/7] ARM: MMU: map text segment ro and data segments execute never

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Jun 13, 2025 at 12:12:01PM +0200, Ahmad Fatoum wrote:
> Hi,
> 
> On 6/13/25 09:58, Sascha Hauer wrote:
> > +	pr_debug("%s: 0x%08x 0x%08x type %d\n", __func__, virt_addr, size, map_type);
> 
> I can add a follow up patch turning type into a string.
> 
> > +	unsigned long text_start = (unsigned long)&_stext;
> > +	unsigned long text_size = (unsigned long)&__start_rodata - (unsigned long)&_stext;
> 
> text_size is an unfortunate name as text_start + text_size != _etext -
> 1, which is surprising.
> 
> I would prefer:
> 
> text_start = code_start = &_stext;
> text_size = &_etext - text_start;
> code_size = &__start_ro_data - code_start;

Ok.

> 
> > +	unsigned long rodata_start = (unsigned long)&__start_rodata;
> > +	unsigned long rodata_size = (unsigned long)&__end_rodata - rodata_start;
> >  
> >  	if (!request_barebox_region("ttb", (unsigned long)ttb,
> >  				    ARM_EARLY_PAGETABLE_SIZE))
> > @@ -550,6 +574,8 @@ void __mmu_init(bool mmu_on)
> >  
> >  	pr_debug("ttb: 0x%p\n", ttb);
> >  
> > +	vectors_init();
> 
> Any particular reason to move this around?

Yes. vectors_init() modifies the exception table which on ARMv7 happens
to live in the text segment which is readonly after the loop.

First I thought this is a hack and the exception vectors should be moved
out of the text segment, but maybe it's not a hack. In the end this way
the exception vectors are protected against modification.

> 
> > +
> >  	/*
> >  	 * Early mmu init will have mapped everything but the initial memory area
> >  	 * (excluding final OPTEE_SIZE bytes) uncached. We have now discovered
> > @@ -568,10 +594,22 @@ void __mmu_init(bool mmu_on)
> >  			pos = rsv->end + 1;
> >  		}
> >  
> > -		remap_range((void *)pos, bank->start + bank->size - pos, MAP_CACHED);
> > +		if (IS_ENABLED(CONFIG_ARM_MMU_PERMISSIONS)) {
> > +			if (region_overlap_size(pos, bank->start + bank->size - pos,
> > +			    text_start, text_size)) {
> 
> Wouldn't matter, but we should check overlap against the full range we
> are going to remap specially. With my suggested text_size/code_size
> changes above that would be correct(er).

Yes, fixed that.

> 
> > +				remap_range((void *)pos, text_start - pos, MAP_CACHED);
> 
> This is ok.
> 
> > +				remap_range((void *)text_start, text_size, MAP_CODE);
> > +				remap_range((void *)rodata_start, rodata_size, ARCH_MAP_CACHED_RO);
> 
> These I would move out of the loop after the iteration.
> 
> > +				remap_range((void *)(rodata_start + rodata_size),
> > +					    bank->start + bank->size - (rodata_start + rodata_size),
> > +					    MAP_CACHED);
> > +			} else {
> > +				remap_range((void *)pos, bank->start + bank->size - pos, MAP_CACHED);
> > +			}
> > +		} else {
> > +			remap_range((void *)pos, bank->start + bank->size - pos, MAP_CACHED);
> > +		}
> 
> If we combine the two if conditional into a single &&ed one, we can
> replace the three remap_range calls by a single one:
> 
> 		pos = text_start + text_end;
> 		if (pos >= bank->start + bank->size)
> 			continue;
> 		/* We carved out a gap for the barebox parts, so fall
>                  * through to remapping the rest
>                  */
> 	}
> 
> 	remap_range((void *)pos, bank->start + bank->size - pos,
> 		    MAP_CACHED);
> }
> 
> Then we can do the remapping of the sections here. I think that would
> aid readability.

Ok, will rearrange.

Sascha

-- 
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 |