Bad error handling in loongarch's kvm_arch_init_vcpu(), need advice

Bad error handling in loongarch's kvm_arch_init_vcpu(), need advice

[Markus Armbruster]

scripts/coccinelle/error-use-after-free.cocci led me to
target/loongarch/kvm/kvm.c:

    int kvm_arch_init_vcpu(CPUState *cs)
    {
        uint64_t val;
        int ret;
        Error *local_err = NULL;

        ret = 0;
        qemu_add_vm_change_state_handler(kvm_loongarch_vm_stage_change, cs);

        if (!kvm_get_one_reg(cs, KVM_REG_LOONGARCH_DEBUG_INST, &val)) {
            brk_insn = val;
        }

        ret = kvm_cpu_check_lsx(cs, &local_err);
        if (ret < 0) {
            error_report_err(local_err);

Reporting an error, but continue anyway.  This is suspicious.

Note for later: @local_error is now non-null.

        }

        ret = kvm_cpu_check_lasx(cs, &local_err);

Passing non-null @local_error to kvm_cpu_check_lasx().  This is wrong.
When kvm_cpu_check_lasx() fails and passes &local_error to error_setg(),
error_setv()'s assertion will fail.

Two possible fixes:

1. If continuing after kvm_cpu_check_lasx() failure is correct, we need
to clear @local_error there.  Since it's not actually an error then, we
should almost certainly not use error_report_err() there.  *Maybe*
warn_report_err().

2. If continuing is wrong, we probably need to return ret.

What is the correct fix?

        if (ret < 0) {
            error_report_err(local_err);

Likewise.

        }

        ret = kvm_cpu_check_lbt(cs, &local_err);
        if (ret < 0) {
            error_report_err(local_err);

Likewise.

        }

        ret = kvm_cpu_check_pmu(cs, &local_err);
        if (ret < 0) {
            error_report_err(local_err);

Likewise.

        }

        ret = kvm_cpu_check_pv_features(cs, &local_err);
        if (ret < 0) {
            error_report_err(local_err);

Best to do the same here.

        }

        return ret;
    }

Re: Bad error handling in loongarch's kvm_arch_init_vcpu(), need advice

[Paolo Bonzini]

On 3/12/25 09:39, Markus Armbruster wrote:
> scripts/coccinelle/error-use-after-free.cocci led me to
> target/loongarch/kvm/kvm.c:
> 
>          ret = kvm_cpu_check_lsx(cs, &local_err);
>          if (ret < 0) {
>              error_report_err(local_err);
> 
> Reporting an error, but continue anyway.  This is suspicious.
> 
>          }
> 
>          ret = kvm_cpu_check_lasx(cs, &local_err);
> 
> Passing non-null @local_error to kvm_cpu_check_lasx().  This is wrong.
> When kvm_cpu_check_lasx() fails and passes &local_error to error_setg(),
> error_setv()'s assertion will fail.
> 
> Two possible fixes:
> 
> 1. If continuing after kvm_cpu_check_lasx() failure is correct, we need
> to clear @local_error there.  Since it's not actually an error then, we
> should almost certainly not use error_report_err() there.  *Maybe*
> warn_report_err().
> 
> 2. If continuing is wrong, we probably need to return ret.

Indeed the correct fix is to return ret, since the Error is set whenever 
an OnOffAuto property is "on" and KVM does not support a feature.

Same for all those below.

Paolo

Re: Bad error handling in loongarch's kvm_arch_init_vcpu(), need advice

[bibo mao]

On 2025/3/12 下午4:59, Paolo Bonzini wrote:
> On 3/12/25 09:39, Markus Armbruster wrote:
>> scripts/coccinelle/error-use-after-free.cocci led me to
>> target/loongarch/kvm/kvm.c:
>>
>>          ret = kvm_cpu_check_lsx(cs, &local_err);
>>          if (ret < 0) {
>>              error_report_err(local_err);
>>
>> Reporting an error, but continue anyway.  This is suspicious.
>>
>>          }
>>
>>          ret = kvm_cpu_check_lasx(cs, &local_err);
>>
>> Passing non-null @local_error to kvm_cpu_check_lasx().  This is wrong.
>> When kvm_cpu_check_lasx() fails and passes &local_error to error_setg(),
>> error_setv()'s assertion will fail.
>>
>> Two possible fixes:
>>
>> 1. If continuing after kvm_cpu_check_lasx() failure is correct, we need
>> to clear @local_error there.  Since it's not actually an error then, we
>> should almost certainly not use error_report_err() there.  *Maybe*
>> warn_report_err().
>>
>> 2. If continuing is wrong, we probably need to return ret.
> 
> Indeed the correct fix is to return ret, since the Error is set whenever 
> an OnOffAuto property is "on" and KVM does not support a feature.
yes, it should return ret immediately, if user forces to enable the 
feature however KVM does not support.

Will submit a patch to fix it, and thanks for reporting.

Regards
Bibo Mao
> 
> Same for all those below.
> 
> Paolo
>