[PATCH 5/7] ARM: MMU: map text segment ro and data segments execute never
Sascha Hauer
s.hauer at pengutronix.de
Tue Jun 17 06:06:18 PDT 2025
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 |
More information about the barebox
mailing list