Re: [PATCH v5 3/5] KVM: RISC-V: SBI FWFT: Fix stale feature exposure after runtime extension changes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] State mutation on error path in SBI FWFT set operation permanently locks features on invalid requests.
- [High] Hiding FWFT features at runtime without reverting architectural state causes hardware to run with unadvertised features.
--

commit 7286cc45c5a1525ff749cb1b680b09d20f49fbe4
Author: Yong-Xuan Wang <yongxuan.wang@sifive.com>

KVM: RISC-V: SBI FWFT: Fix stale feature exposure after runtime extension changes

Fix a bug where FWFT features could be incorrectly exposed to guests
after userspace disables their dependent ISA extensions at runtime. The
fix adds a two-layer checking mechanism to ensure feature availability
is re-evaluated based on the current ISA extension state.

> diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
> index aee951f2b8e63..ab39ac464ffd8 100644
> --- a/arch/riscv/kvm/vcpu_sbi_fwft.c
> +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c

[ ... ]

> @@ -527,6 +535,8 @@ static int kvm_sbi_ext_fwft_set_reg(struct kvm_vcpu *vcpu, unsigned long reg_num
>  	conf = kvm_sbi_fwft_get_config(vcpu, feature->id);
>  	if (!conf || !conf->supported)
>  		return -ENOENT;
> +	else if (conf->feature->supported && !conf->feature->supported(vcpu))
> +		return -ENOENT;

[Severity: High]
This isn't a bug introduced by this patch, but this dynamic check might lock
userspace out of clearing the feature state.

If userspace disables the prerequisite ISA extension (e.g. SMNPM) via ONE_REG,
the extension is cleared from vcpu->arch.isa, but the dependent FWFT
architectural state in vcpu->arch.cfg.henvcfg remains intact.

If userspace then attempts to clear the FWFT feature state via ONE_REG, this
check returns -ENOENT because the underlying ISA extension is now disabled.

When the vCPU runs, kvm_riscv_vcpu_config_load() unconditionally writes the
stale henvcfg to the physical CSR_HENVCFG register:

arch/riscv/kvm/vcpu_config.c:kvm_riscv_vcpu_config_load() {
    ...
    csr_write(CSR_HENVCFG, cfg->henvcfg);
    ...
}

Will this cause the guest to run with unadvertised hardware features enabled?
Should the architectural state be reverted before hiding the feature, or should
userspace be permitted to clear the state even if the feature is unsupported?

>  
>  	switch (reg_num - feature->first_reg_num) {
>  	case 0:


[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in how
feature flags are handled during guest set operations:

arch/riscv/kvm/vcpu_sbi_fwft.c:kvm_sbi_fwft_set() {
    ...
	if (conf->flags & SBI_FWFT_SET_FLAG_LOCK)
		return SBI_ERR_DENIED_LOCKED;

	conf->flags = flags;

	return conf->feature->set(vcpu, conf, false, value);
}

If a guest issues an SBI_EXT_FWFT_SET call with an invalid value but sets
the SBI_FWFT_SET_FLAG_LOCK flag, conf->flags is updated unconditionally.

If the feature-specific set() callback rejects the invalid value and returns
SBI_ERR_INVALID_PARAM, the hypervisor will return this error to the guest
but the lock flag remains permanently set.

Does this permanently lock the feature on invalid requests, preventing
subsequent attempts to set a valid value? Should conf->flags only be
updated if the set() callback succeeds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-kvm-get_reg_list-v2-v5-0-415d08a2813b@sifive.com?part=3