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

Abdellatif El Khlifi
2022-10-14T10:17:32Z

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@arm.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Jens Wiklander <jens.wiklander@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
> >