Re: [PATCH] KVM: arm64: Fix NULL pointer access issue

[Yingchao Deng (Consultant)]

Thanks James & Marc
Will upload v2 and move the warning to the end for better readability.

Thanks,
Yingchao
On 9/1/2025 10:16 PM, James Clark wrote:
> On 01/09/2025 2:30 pm, Marc Zyngier wrote:
> > On Mon, 01 Sep 2025 13:31:23 +0100,
> > James Clark <james.clark@xxxxxxxxxx> wrote:
> > > On 01/09/2025 1:24 pm, Marc Zyngier wrote:
> > > > On Mon, 01 Sep 2025 11:36:11 +0100,
> > > > James Clark <james.clark@xxxxxxxxxx> wrote:
> > > > > On 01/09/2025 11:01 am, Yingchao Deng wrote:
> > > > > > When linux is booted in EL1, macro "host_data_ptr()" is a wrapper 
> > > > > > that
> > > > > > resolves to "&per_cpu_ptr_nvhe_sym(kvm_host_data, cpu)",
> > > > > > is_hyp_mode_available() return false during kvm_arm_init, the 
> > > > > > per-CPU base
> > > > > > pointer __kvm_nvhe_kvm_arm_hyp_percpu_base[cpu] remains 
> > > > > > uninitialized.
> > > > > > Consequently, any access via per_cpu_ptr_nvhe_sym(kvm_host_data, 
> > > > > > cpu)
> > > > > > will result in a NULL pointer.
> > > > > >
> > > > > > Add is_kvm_arm_initialised() condition check to ensure that 
> > > > > > kvm_arm_init
> > > > > > completes all necessary initialization steps, including 
> > > > > > init_hyp_mode.
> > > > > >
> > > > > > Fixes: 054b88391bbe2 ("KVM: arm64: Support trace filtering for 
> > > > > > guests")
> > > > > > Signed-off-by: Yingchao Deng <yingchao.deng@xxxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > Add a check to prevent accessing uninitialized per-CPU data.
> > > > > > ---
> > > > > >     arch/arm64/kvm/debug.c | 7 ++++---
> > > > > >     1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > > > > > index 
> > > > > > 381382c19fe4741980c79b08bbdab6a1bcd825ad..add58056297293b4eb337028773b1b018ecc9d35 
> > > > > > 100644
> > > > > > --- a/arch/arm64/kvm/debug.c
> > > > > > +++ b/arch/arm64/kvm/debug.c
> > > > > > @@ -233,7 +233,7 @@ void kvm_debug_handle_oslar(struct kvm_vcpu 
> > > > > > *vcpu, u64 val)
> > > > > >     void kvm_enable_trbe(void)
> > > > > >     {
> > > > > >         if (has_vhe() || is_protected_kvm_enabled() ||
> > > > > > -        WARN_ON_ONCE(preemptible()))
> > > > > > +        WARN_ON_ONCE(preemptible()) || !is_kvm_arm_initialised())
> > > > >
> > > > > Hi Yingchao,
> > > > >
> > > > > There shouldn't be a warning for this, at least for the case where
> > > > > it's not initialized and never will be. If you're never going to 
> > > > > run a
> > > > > guest these functions can all skip, the same way for !has_vhe() etc.
> > > >
> > > > It's not a warning. It's a bona-fide crash:
> > > >
> > > > void kvm_enable_trbe(void)
> > > > {
> > > >     if (has_vhe() || is_protected_kvm_enabled() ||
> > > >         WARN_ON_ONCE(preemptible()))
> > > >         return;
> > > >
> > > >     host_data_set_flag(TRBE_ENABLED); <--- Explodes here
> > > > }
> > > >
> > > > So the write of the flag has to be skipped if KVM is available, even
> > > > if KVM is compiled in.
> > > >
> > > >     M.
> > >
> > > Yeah. And just in case there is any confusion, I didn't mean that we
> > > should not have the check entirely, just that it shouldn't be in the
> > > WARN_ON_ONCE(). We should put it in the part that makes the functions
> > > silently skip:
> > >
> > >    if (has_vhe() || is_protected_kvm_enabled() ||
> > >        !is_kvm_arm_initialised() ||
> > >        WARN_ON_ONCE(preemptible()))
> > >            return;
> >
> > Which is exactly what the OP wrote, except for swapping the last two
> > terms.
> >
> >     M.
>
> Hah! So it is. Being on the same line as the warning really threw me 
> despite looking at it 10 times.
>
> Not sure if it's just me but I think having the warning at the end or 
> on its own line is more readable.
>
> Either way:
>
> Reviewed-by: James Clark <james.clark@xxxxxxxxxx>