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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 06/12/2021 14:20, Michal Orzel wrote:
> to hypervisor when switching to 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 IMPLEMENTATIONDEFINED"
Typo: Missing space between IMPLEMENTATION and DEFINED.

> Currently Xen does not ensure that the top 32 bits are zeroed and this
> needs to be fixed.
Can you outline why this is a problem and why we need to protect? IIRC, 
the main concern is Xen may misinterpret what the guest requested but we 
are not concerned about Xen using wrong value.
> Fix this bug by zeroing the upper 32 bits of these registers on an
> entry to hypervisor when switching to AArch32 state.
>
> Set default value of parameter compat of macro entry to 0 (AArch64 mode
> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
> when not passed.
Which error do you see otherwise? Is it a compilation error?

> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>   xen/arch/arm/arm64/entry.S | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index fc3811ad0a..d364128175 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -109,8 +109,16 @@
>    * If 0, we rely on the on x0/x1 to have been saved at the correct
>    * position on the stack before.
>    */
> -        .macro  entry, hyp, compat, save_x0_x1=1
> +        .macro  entry, hyp, compat=0, save_x0_x1=1
>           sub     sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR */
> +
> +        /* Zero the upper 32 bits of the registers when switching to AArch32 */
> +        .if \compat == 1      /* AArch32 mode */
> +        .irp 
> nr,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\nr, w\nr
> +        .endr
> +        .endif
So Jan mentioned, the x0/x1 may have already been saved. So you may need 
to fetch them from the stack and then clobber the top 32-bit.
Cheers,

--
Julien Grall