Re: [BUG] ARM64 regression: NULL pointer dereference in arm_smccc_version_init+0x90/0x1ac

Will Deacon
2025-01-31T12:42:48Z

On Thu, Jan 30, 2025 at 03:56:10PM +0100, Emanuele Rocca wrote:
> On 2025-01-30 12:19, Mark Rutland wrote:
> > For the benefit others, when we looked into this a few days ago it
> > appeared that a GPR was being clobbered across an SMCCC call, resulting
> > in a later crash (as that GPR should hold the ADRP'd base address of
> > 'smccc_version'). I didn't have the time to dig more into that (e.g. to
> > figure out whether kernel/compiler/firmware was to blame).
> >
> > Emanuele, could you please dump the result of:
> >
> > objdump --disassemble=arm_smccc_version_init vmlinux
> >
> > ... for this kernel? That'd make it possible for others to
> > perform/verify the analysis I mentioned above.
>
> Sure, here it is:
> https://people.debian.org/~ema/Z5OpGluX6oX5NLxh@NH27D9T0LF.objdump.txt
>
> > If you can share any more details from the crash, that'd be helpful. The
> > GPR dump would be *enormously* helpful in this case, and even a photo of
> > the crash log might be useful.
>
> pc : arm_smccc_version_init+0x90/0x1ac
> lr : psci_probe+0x1fc/0x2c0
> sp : ffffa3d8eea33ca0
> x29: ffffa3d8eea33ce0 x28: 0000000081000200 x27: ffffa3d8edd72a80
> x26: ffffa3d8edd728b0 x25: ffffa3d8eec66248 x24: ffffa3d8edc528d8
> x23: ffffa3d8eed7f4f8 x22: 0000000000000001 x21: ffffa3d8eea58410
> x20: ffffa3d8eed7f000 x19: 0000000000010001 x18: 0000000000000006
> x17: 0000000009f69730 x16: 00000008760968d0 x15: 0000000000000000
> x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
> x11: 00000000ffffefff x10: ffffa3d8eeac09c8 x9 : ffffa3d8eea687d8
> x8 : ffffa3d8eea33cb8 x7 : 0000000000000000 x6 : 0000000000000000
> x5 : ffffa3d8eed7f000 x4 : ffffa3d8edeb0000 x3 : 0000000000000000
> x2 : 0000000000000000 x1 : 0000000000000001 x0 : 0000000000000000
>
> Call trace:
> arm_smccc_version_init+0x90/0x1ac
> psci_0_2_init+0x20/0x2c
> psci_1_0_init+0x1c/0x58
> psci_dt_init+0x6c/0x98
> setup_arch+0x400/0x5ec
> start_kernel+0x90/0x788
> __primary_switched+0xbc/0xc4
> Code: d29fffe1 eb01001f 1a9f97e0 f0ffd384 (b94264c2)
>
> The above was transcribed by hand. I double-checked, but there may be
> mistakes.
>
> Photo here: https://people.debian.org/~ema/Z5OpGluX6oX5NLxh@NH27D9T0LF.jpg

Crikey, you transcribed all that junk by _hand_?! Thanks for the effort,
but next time please don't waste your time! The screenshot is fine.

Anyway, it looks like x6 gets trashed across the SMC. I think that's fine
per the SMCCC spec:

| Unused result and scratch registers can leak information after an
| SMC or HVC call. An implementation can mitigate this risk by either
| preserving the register state over the call, or returning a constant
| value,such as zero, in each register.

so the question is whether or not the compiler should be relying on the
register being preserved. It looks to me like the arm_smccc_1_1_smc()
macro is actually broken in this regard: it constructs the input list
based on the number of arguments passed and doesn't take the above into
account at all.

So I think we should change that so unused arguments are clobbered
instead. Mark -- what do you think?

Will