Re: [PATCH v2] xen/arm64: Zero the top 32 bits of gp registers on entry...

[Michal Orzel <michal.orzel@xxxxxxx>]

On 16.12.2021 15:55, Julien Grall wrote:
> Hi,
> 
> On 16/12/2021 14:26, Michal Orzel wrote:
>> On 16.12.2021 14:50, Jan Beulich wrote:
>>> On 16.12.2021 10:21, Michal Orzel wrote:
>>>> to hypervisor when switching from AArch32 state.
>>>>
>>>> According to section D1.20.2 of Arm Arm(DDI 0487A.j):
>>>> "If the general-purpose register was accessible from AArch32 state the
>>>> upper 32 bits either become zero, or hold the value that the same
>>>> architectural register held before any AArch32 execution.
>>>> The choice between these two options is IMPLEMENTATION DEFINED"
>>>>
>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>> needs to be fixed. The reason why is that there are places in Xen
>>>> where we assume that top 32bits are zero for AArch32 guests.
>>>> If they are not, this can lead to misinterpretation of Xen regarding
>>>> what the guest requested. For example hypercalls returning an error
>>>> encoded in a signed long like do_sched_op, do_hmv_op, do_memory_op
>>>> would return -ENOSYS if the command passed as the first argument was
>>>> clobbered.
>>>>
>>>> Create a macro clobber_gp_top_halves to clobber top 32 bits of gp
>>>> registers when hyp == 0 (guest mode) and compat == 1 (AArch32 mode).
>>>> Add a compile time check to ensure that save_x0_x1 == 1 if
>>>> compat == 1.
>>>>
>>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>>> ---
>>>>   xen/arch/arm/arm64/entry.S | 29 +++++++++++++++++++++++++++++
>>>>   1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>>> index fc3811ad0a..01f32324d0 100644
>>>> --- a/xen/arch/arm/arm64/entry.S
>>>> +++ b/xen/arch/arm/arm64/entry.S
>>>> @@ -102,6 +102,30 @@
>>>>           .endif
>>>>             .endm
>>>> +
>>>> +/*
>>>> + * Clobber top 32 bits of gp registers when switching from AArch32
>>>> + */
>>>> +        .macro clobber_gp_top_halves, compat, save_x0_x1
>>>> +
>>>> +        .if \compat == 1      /* AArch32 mode */
>>>> +
>>>> +        /*
>>>> +         * save_x0_x1 is equal to 0 only for guest_sync (compat == 0).
>>>> +         * Add a compile time check to avoid violating this rule.
>>>> +         */
>>>> +        .if \save_x0_x1 == 0
>>>> +        .error "save_x0_x1 is 0 but compat is 1"
>>>> +        .endif
>>>> +
>>>> +        .irp 
>>>> n,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29
>>>> +        mov w\n, w\n
>>>> +        .endr
>>>
>>> What about x30 (aka lr)?
>>>
>> Well the docs says about gp registers as a whole so including lr.
>> However I do not see how clobbering lr would impact Xen.
> 
> Xen may not be directly impacted. However this may be used by some userspace 
> application (such as for VM introspection) and could be dumped on the console.
> 
> So I would cover all the GPR to give a consistent view to everyone.
> 
Ok I fully understand. I will send this change as v3.

> Cheers,
> 
Cheers,
Michal