Re: [PATCH v3 24/52] xen/mpu: build up start-of-day Xen MPU memory region map

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 28/06/2023 12:17, Julien Grall wrote:
> Hi,
Hi Julien,
> On 28/06/2023 11:55, Ayan Kumar Halder wrote:
> > On 26/06/2023 04:34, Penny Zheng wrote:
> > > CAUTION: This message has originated from an External Source. Please 
> > > use proper judgment and caution when opening attachments, clicking 
> > > links, or responding to this email.
> > >
> > > The start-of-day Xen MPU memory region layout shall be like
> > > as follows:
> > >
> > > xen_mpumap[0] : Xen text
> > > xen_mpumap[1] : Xen read-only data
> > > xen_mpumap[2] : Xen read-only after init data
> > > xen_mpumap[3] : Xen read-write data
> > > xen_mpumap[4] : Xen BSS
> > > xen_mpumap[5] : Xen init text
> > > xen_mpumap[6] : Xen init data
> > >
> > > The layout shall be compliant with what we describe in xen.lds.S,
> > > or the codes need adjustment.
> > >
> > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > ---
> > > v3:
> > > - cache maintanence for safety when modifying MPU memory mapping table
> > > - Hardcode index for all data/text sections
> > > - To make sure that alternative instructions are included, use 
> > > "_einitext"
> > > as the start of the "Init data" section.
> > > ---
> > < snip>
> > > +/*
> > > + * Static start-of-day Xen EL2 MPU memory region layout:
> > > + *
> > > + *     xen_mpumap[0] : Xen text
> > > + *     xen_mpumap[1] : Xen read-only data
> > > + *     xen_mpumap[2] : Xen read-only after init data
> > > + *     xen_mpumap[3] : Xen read-write data
> > > + *     xen_mpumap[4] : Xen BSS
> > > + *     xen_mpumap[5] : Xen init text
> > > + *     xen_mpumap[6] : Xen init data
> > > + *
> > > + * Clobbers x0 - x6
> > > + *
> > > + * It shall be compliant with what describes in xen.lds.S, or the 
> > > below
> > > + * codes need adjustment.
> > > + */
> > > +ENTRY(prepare_early_mappings)
> > > +    /* x0: region sel */
> > > +    mov   x0, xzr
> > > +    /* Xen text section. */
> > > +    load_paddr x1, _stext
> > > +    load_paddr x2, _etext
> > > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6, 
> > > attr_prbar=REGION_TEXT_PRBAR
> > > +
> > > +    add   x0, x0, #1
> > > +    /* Xen read-only data section. */
> > > +    load_paddr x1, _srodata
> > > +    load_paddr x2, _erodata
> > > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6, 
> > > attr_prbar=REGION_RO_PRBAR
> > > +
> > > +    add   x0, x0, #1
> > > +    /* Xen read-only after init data section. */
> > > +    load_paddr x1, __ro_after_init_start
> > > +    load_paddr x2, __ro_after_init_end
> > > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6
> > > +
> > > +    add   x0, x0, #1
> > > +    /* Xen read-write data section. */
> > > +    load_paddr x1, __ro_after_init_end
> > > +    load_paddr x2, __init_begin
> > > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6
> > > +
> > > +    add   x0, x0, #1
> > > +    /* Xen BSS section. */
> > > +    load_paddr x1, __bss_start
> > > +    load_paddr x2, __bss_end
> > > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6
> > > +
> > > +    add   x0, x0, #1
> > > +    /* Xen init text section. */
> > > +    load_paddr x1, _sinittext
> > > +    load_paddr x2, _einittext
> > > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6, 
> > > attr_prbar=REGION_TEXT_PRBAR
> > > +
> > > +    add   x0, x0, #1
> > > +    /* Xen init data section. */
> > > +    /*
> > > +     * Even though we are not using alternative instructions in MPU 
> > > yet,
> > > +     * we want to use "_einitext" for the start of the "Init data" 
> > > section
> > > +     * to make sure they are included.
> > > +     */
> > > +    load_paddr x1, _einittext
> > > +    roundup_section x1
> > > +    load_paddr x2, __init_end
> > > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6
> > > +
> > > +    /* Ensure any MPU memory mapping table updates made above have 
> > > occurred. */
> > > +    dsb   nshst
> > > +    ret
> > > +ENDPROC(prepare_early_mappings)
> > Any reason why this is in assembly ?
> I am not Penny. But from my understanding, in your approach, you will 
> require to disable to switch the disable the MPU for using the new 
> sections. While I agree...
> > We have implemented it in C 
> > https://github.com/Xilinx/xen/blob/d1258f1cefe406a3f91237b8106746c089864651/xen/arch/arm_mpu/mm_mpu.c#L941 
> > , so that it can be common between R82 and R52.
> ... this means you can share the code. It also means:
>   * You can't protect Xen properly from the start
Yes, I see what you mean. Refer 
https://github.com/Xilinx/xen/blob/d1258f1cefe406a3f91237b8106746c089864651/xen/arch/arm_mpu/arm32/mpu_v8r.S#L82C7-L82C15 
,
I am mapping CONFIG_XEN_START_ADDRESS + 2 MB. But, probably you mean 
individual sections should be properly mapped from the beginning.
>   * You need to flush the cache (not great for performance)
>   * You need to be more cautious as the MPU will be disabled for a 
> short period of time.
Yes, MPU is disabled when set_boot_mpumap() is invoked. But is this 
really a security issue ?
I mean only a single core is running and it is executing Xen. The MPU is 
turned off for few cycles.
> In fact looking at your switch code in setup_protection_regions(), I 
> am not convinced you can disable the MPU in C and then call 
> set_boot_mpumap(). I think the enable/disable would need to be moved 
> in the assembly function. There are potentially more issues.
disable_mpu(), enable_mpu() are written in assembly. See 
https://github.com/Xilinx/xen/blob/d1258f1cefe406a3f91237b8106746c089864651/xen/arch/arm_mpu/arm32/mpu_v8r.S#L128
- Ayan

> So overall, I am not convinced of the C/common approach.
>
> Cheers,