[PATCH v5 02/10] arm64: smccc: clear the Xn registers after SMC calls

[Abdellatif El Khlifi]

On Fri, Sep 30, 2022 at 11:48:04AM +0200, Jens Wiklander wrote:
> On Mon, Sep 26, 2022 at 03:08:19PM +0100, Abdellatif El Khlifi wrote:
> > set to zero the x0-x17 registers
> > 
> > As per the SMCCC v1.2 spec, unused result and scratch registers
> > can leak information after an SMC call. We can mitigate against
> > this risk by returning zero in each register.
> > 
> > The leakage we are referring to is data leakage across exception
> > levels. The intent is to prevent lower exception levels (EL1/EL0)
> > from reading the SMC data exchanged at EL2.
> > 
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> > Cc: Tom Rini <trini at konsulko.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > Cc: Jens Wiklander <jens.wiklander at linaro.org>
> > ---
> > 
> > Changelog:
> > ===============
> > 
> > v4:
> > 
> > * move the clearing code into a new macro: clear_gp_regs
> > 
> > v3:
> > 
> > * clear the Xn registers after SMC calls
> > 
> > 
> >  arch/arm/cpu/armv8/smccc-call.S | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm/cpu/armv8/smccc-call.S b/arch/arm/cpu/armv8/smccc-call.S
> > index ec6f299bc9..32f3eb8eeb 100644
> > --- a/arch/arm/cpu/armv8/smccc-call.S
> > +++ b/arch/arm/cpu/armv8/smccc-call.S
> > @@ -50,6 +50,12 @@ ENDPROC(__arm_smccc_hvc)
> >  
> >  #ifdef CONFIG_ARM64
> >  
> > +	.macro	clear_gp_regs
> > +	.irp	n,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17
> > +	mov	x\n, xzr
> > +	.endr
> > +	.endm
> > +
> >  	.macro SMCCC_1_2 instr
> >  	/* Save `res` and free a GPR that won't be clobbered */
> >  	stp     x1, x19, [sp, #-16]!
> > @@ -84,6 +90,9 @@ ENDPROC(__arm_smccc_hvc)
> >  	stp	x14, x15, [x19, #ARM_SMCCC_1_2_REGS_X14_OFFS]
> >  	stp	x16, x17, [x19, #ARM_SMCCC_1_2_REGS_X16_OFFS]
> >  
> > +	/* x0-x17 registers can leak information after an SMC or HVC call. Let's clear them */
> > +	clear_gp_regs
> > +
> 
> This should in my opinion not be needed. The higher exception level should
> only return what it indends to return and certainly not rely on lower
> exception levels to try to hide eventual unintentionally revealed
> secrets.
> 
> In an earlier conversation you said:
> > The leakage we are referring to is data leakage across exception levels.
> > The intent is to prevent lower exception levels (EL1/EL0) to read the
> > data exchanged at EL2.
> > 
> > The linux kernel clears the general purpose registers before switching
> > to EL0. As far as I know u-boot doesn't.
> > 
> > So, the code above makes sure the registers are cleared.
> 
> U-Boot is as far as I know not changing to EL0. Do you have a real example
> where this cleaning actually would be needed? If it's needed I'd expect
> the cleaning to be done just before changing exception level.

No real example for that. But I tried to follow the spec as much as possible.
Since, no real need for this I'm happy to remove the commit and this has
been done in patchset v6. Thanks.

> 
> Cheers,
> Jens
> 
> >  	/* Restore original x19 */
> >  	ldp     xzr, x19, [sp], #16
> >  	ret
> > -- 
> > 2.17.1
> >