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 14:42, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> On 28/06/2023 14:22, Ayan Kumar Halder wrote:
> > 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 ?
> My second point is not about security, it is about been compliant with 
> the Arm Arm. To quote what you wrote:
> " To avoid unexpected unaligment access fault during MPU disabled, 
> set_boot_mpumap shall be written in assembly code."
> What's the guarantee that the compiler will not generate any 
> instructions that could generate an alignment fault?
I thought by writing in assembly, we tell the compiler what instructions 
to generate. For eg
ENTRY(set_boot_mpumap)
    push {r4}
    mov   r2, #0               /* table index */
1:  ldr   r3, [r1], #4         /* r3: prbar */
    ldr   r4, [r1], #12        /* r4: prlar */
    write_pr r2, r3, r4
    add   r2, r2, #1           /* table index ++ */
    cmp   r2, r0
    blt  1b
    pop {r4}
    ret
ENDPROC(set_boot_mpumap)

I ask the compiler to use ldr (and not ldrb) instructions.

May be I am missing something very obvious here.

However, I might be misunderstanding the comment here. It was originally 
written by Penny.
@Penny, Can you explain "To avoid unexpected unaligment access fault 
during MPU disabled, set_boot_mpumap shall be written in assembly code." 
in a bit more detail, please ?
> Furthermore, from my understanding, at least on Armv8-A, there are 
> caching problem because you will need to save some registers (for the 
> call to set_boot_mpumap()) on the stack with cache disabled. This 
> means the cache will be bypassed. But you may then restore the 
> registers with the cache enabled (the compiler could decide that it is 
> not necessary to read the stack before hand). So you could read the 
> wrong data if there is a stale cacheline.
Yes, this makes some sense. So will the following make it correct :-

1. Execute 'dmb' before invoking enable_mpu(). This will ensure that the 
registers are strictly restored in set_boot_mpumap() before the HSCTLR 
is read.
We do have 'dsb sy' before modifying HSCTLR (ie enabling cache), but may 
be we want to be stricter.
2. Invalidate the D cache after "mcr   CP32(r0, HSCTLR)" and then dsb 
(to ensure d cache is invalidated), isb (flush the instruction cache as 
MPU is enabled), ret.
- Ayan

> So overall, I think you want to tightly control the section where the 
> MPU is off. This means writing the logic in assembly rather than C.
> > 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
> Right, but then you return to C world to then call set_boot_mpumap(). 
> So you have still some part in C even if it is small.
> Cheers,