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

[Julien Grall <julien@xxxxxxx>]

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.

Cheers,

--
Julien Grall