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

[James Clark <james.clark@linaro.org>]

On 01/09/2025 2:30 pm, Marc Zyngier wrote:
> On Mon, 01 Sep 2025 13:31:23 +0100,
> James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 01/09/2025 1:24 pm, Marc Zyngier wrote:
>>> On Mon, 01 Sep 2025 11:36:11 +0100,
>>> James Clark <james.clark@linaro.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@oss.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;
> 
> 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@linaro.org>