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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/06/2023 10:43 am, Julien Grall wrote:
> On 30/06/2023 10:26, Ayan Kumar Halder wrote:
>> On 29/06/2023 12:55, Julien Grall wrote:
>>>>> 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 :-
>>>
>>> I am confused. In a previous answer, I voiced my concerned with
>>> trying to replace the full MPU table. So it is not clear to me why
>>> you are asking me if the following work. Do you still want to do it?
>>> If so, why?
>>
>> I completely agree with you to set up the MPU table in assembly with
>> the correct permissions for each section (as done by Penny's patch).
>> That would atleast ensure that we don't need to reset the MPU
>> sections for Xen again.
>>
>> What I was trying to understand deeper was some of the objections you
>> had raised and if any sort of mitigations are possible.
>>
>> Again I am not saying that we need to apply the mitigations (if
>> available) in this particular scenario.
>>
>>>
>>>>
>>>> 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.
>>>
>>> I am afraid I don't know how the DMB will enforce that. Can you
>>> clarify?
>>
>> pop {r4}  /* As part of set_boot_mpumap() */
>>
>> dmb /* This should ensure that r4 is fully restored from the stack
>> before the next instruction. At this point, the D cache is still
>> disabled. */
>
> I don't really see how this helps because the C instruction:
>
>  set_boot_mpumap(....)
>
> could also require to read/write the stack for saving r0-r3. And
> AFAIK, you can't control when this would happen.

The argument is far easier than that.

At all point, anywhere in code, the C compiler can emit calls to
memcpy/memset behind your back, including (in principle) for code which
looks like `int foo;` (yes, because of things like -ftrivial-auto-var-init).

With things like UBSAN active, it's a much wider range of functions that
can be called.

If anything is potentially unsafe to the C operating environment, it
*must* be handled in assembly.

~Andrew