[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <80178c90-b9f3-4e1f-baa5-f54aa89ec927@linaro.org>
Date: Mon, 1 Sep 2025 13:31:23 +0100
From: James Clark <james.clark@...aro.org>
To: Marc Zyngier <maz@...nel.org>,
Yingchao Deng <yingchao.deng@....qualcomm.com>
Cc: linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
linux-kernel@...r.kernel.org, quic_yingdeng@...cinc.com,
jinlong.mao@....qualcomm.com, tingwei.zhang@....qualcomm.com,
Oliver Upton <oliver.upton@...ux.dev>, Joey Gouly <joey.gouly@....com>,
Suzuki K Poulose <suzuki.poulose@....com>, Zenghui Yu
<yuzenghui@...wei.com>, Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>
Subject: Re: [PATCH] KVM: arm64: Fix NULL pointer access issue
On 01/09/2025 1:24 pm, Marc Zyngier wrote:
> On Mon, 01 Sep 2025 11:36:11 +0100,
> James Clark <james.clark@...aro.org> 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@....qualcomm.com>
>>> ---
>>> 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;
And the same for kvm_disable_trbe() and kvm_tracing_set_el1_configuration().
James
Powered by blists - more mailing lists