Re: [PATCH] KVM: riscv: SBI FWFT: Apply LOCK flag only on successful set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] SBI FWFT sub-features are enabled by default if supported by the host, violating KVM API architectural rules and breaking live migration.
- [High] Modifying FWFT features via KVM_SET_ONE_REG fails to set vcpu->arch.csr_dirty, which can cause the hardware CSRs to remain out of sync with the vCPU configuration.
- [High] Hardware feature probe in try_to_set_pmm() can be preempted, leading to false negatives and host CSR corruption.
- [High] Direct hardware CSR modifications in FWFT set handlers break nested virtualization (nacl) and are vulnerable to preemption.
--

commit 599bb2245d648d414e424da52cb50ced8f5d755b
Author: SeungJu Cheon <suunj1331@gmail.com>

KVM: riscv: SBI FWFT: Apply LOCK flag only on successful set

kvm_sbi_fwft_set() applies the caller's flags to conf->flags before
invoking the set() callback. If the callback returns an error, the LOCK
bit persists and the feature becomes permanently locked without its value
ever being changed.

Move the flags assignment after the callback so LOCK takes effect only
on success.

> diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
> index 2eab15339694f..a9b5bffc0e572 100644
> --- a/arch/riscv/kvm/vcpu_sbi_fwft.c
> +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
> @@ -303,9 +303,11 @@ static int kvm_sbi_fwft_set(struct kvm_vcpu *vcpu, u32 feature,
>  	if (conf->flags & SBI_FWFT_SET_FLAG_LOCK)
>  		return SBI_ERR_DENIED_LOCKED;
>  
> -	conf->flags = flags;
> +	ret = conf->feature->set(vcpu, conf, false, value);
> +	if (ret == SBI_SUCCESS)
> +		conf->flags = flags;
>  
> -	return conf->feature->set(vcpu, conf, false, value);
> +	return ret;
>  }
>  
>  static int kvm_sbi_fwft_get(struct kvm_vcpu *vcpu, unsigned long feature,

While reviewing this file, I noticed a few pre-existing issues in
arch/riscv/kvm/vcpu_sbi_fwft.c:

In kvm_sbi_ext_fwft_init():

    if (feature->supported)
        conf->supported = feature->supported(vcpu);
    else
        conf->supported = true;

    conf->enabled = conf->supported;

[Severity: High]
This isn't a bug introduced by this patch, but it appears FWFT sub-features
are unconditionally enabled by default if the hardware supports them. Does
this violate the KVM architectural rule that new guest-visible features must
default to OFF?

Could this break live migration if a VM is moved to a host lacking support
for a feature that was silently exposed?


In kvm_sbi_ext_fwft_set_reg():

    case 2:
        ret = conf->feature->set(vcpu, conf, true, value);
        break;

[Severity: High]
This is a pre-existing issue, but does modifying FWFT features via
KVM_SET_ONE_REG fail to set vcpu->arch.csr_dirty?

When vcpu_load() runs on the same physical CPU, it bypasses
kvm_riscv_vcpu_config_load(). Can this leave the hardware CSRs (like
CSR_HEDELEG or CSR_HENVCFG) out of sync with the vCPU configuration?


In try_to_set_pmm():

    csr_set(CSR_HENVCFG, value);
    return (csr_read_clear(CSR_HENVCFG, ENVCFG_PMM) & ENVCFG_PMM) == value;

[Severity: High]
This is a pre-existing issue, but is it safe to modify and read back the CSR
here without disabling preemption?

If the thread is migrated between csr_set() and csr_read_clear(), could this
result in false negatives and corrupt the original CPU's CSR_HENVCFG by
persistently leaving ENVCFG_PMM bits set?


In kvm_sbi_fwft_set_misaligned_delegation():

    if (!one_reg_access)
        csr_set(CSR_HEDELEG, MIS_DELEG);

And in kvm_sbi_fwft_set_pointer_masking_pmlen():

    if (!one_reg_access)
        csr_write(CSR_HENVCFG, vcpu->arch.cfg.henvcfg);

[Severity: High]
This is a pre-existing issue, but do these direct hardware CSR modifications
break nested virtualization? By bypassing nacl_shmem(), the L0 hypervisor
might not observe L1's FWFT changes.

Also, since SBI_EXT_FWFT_SET exits are handled with preemption enabled,
could preempting or migrating these calls corrupt CSR_HEDELEG or CSR_HENVCFG
on a random CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624130238.524706-1-suunj1331@gmail.com?part=1