Re: [PATCH v2 6/7] RISC-V: KVM: Add support for control-flow integrity FWFT features

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] KVM fails to context-switch the shadow stack pointer (CSR_SSP) when exposing ZICFISS to the guest.
- [High] Direct write to `CSR_HENVCFG` bypasses nested acceleration (NaCl) shared memory synchronization.
- [High] The `unsigned long flag` argument in `henvcfg` helpers zero-extends and corrupts the upper 32 bits on RV32, and silently truncates 64-bit flags.
- [High] Guest can bypass VMM ISA restrictions to enable FWFT features due to caching at vCPU creation.
- [High] Modifying FWFT features via KVM_SET_ONE_REG updates `vcpu->arch.cfg.henvcfg` but fails to mark the CSR as dirty, leading to stale hardware state execution.
- [Medium] SBI FWFT flags are committed even if the feature value set operation fails.
--

commit c2d9c5584a3fada7da09588693c511ec3fdbbcf1
Author: Inochi Amaoto <inochiama@gmail.com>
Subject: RISC-V: KVM: Add support for control-flow integrity FWFT features

This commit exposes the Zicfilp and Zicfiss control-flow integrity features
to guests through the SBI FWFT interface. It also allows the VMM to block
access to these features by disabling the corresponding ISA extensions.

> diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
> index 375269e43c13b..278eb95b97de4 100644
> --- a/arch/riscv/kvm/vcpu_sbi_fwft.c
> +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
> @@ -163,6 +163,58 @@ static long kvm_sbi_fwft_get_misaligned_delegation(struct kvm_vcpu *vcpu,
[ ... ]
> +static bool kvm_sbi_fwft_landing_pad_supported(struct kvm_vcpu *vcpu)
> +{
> +	return riscv_isa_extension_available(vcpu->arch.isa, ZICFILP);
> +}

[Severity: High]
Does checking the ISA extension at vCPU creation allow the guest to bypass VMM
restrictions?

The FWFT framework caches feature availability during kvm_sbi_ext_fwft_init()
which executes at vCPU creation. At that time, vcpu->arch.isa typically
contains all default host-supported extensions.

If the VMM later configures the VM by disabling unsupported extensions via
KVM_SET_ONE_REG, the guest can still successfully enable the hardware features
because the FWFT layer permanently cached conf->supported as true.

[ ... ]
> +static long kvm_sbi_fwft_set_landing_pad(struct kvm_vcpu *vcpu,
> +					 struct kvm_sbi_fwft_config *conf,
> +					 bool one_reg_access, unsigned long value)
> +{
> +	return kvm_sbi_fwft_env_flag_set_helper(vcpu, conf, one_reg_access,
> +						value, ENVCFG_LPE);
> +}

[Severity: High]
Can passing ENVCFG_LPE to this helper cause truncation or zero-extension
issues?

Looking at kvm_sbi_fwft_env_flag_set_helper(), the flag parameter is typed as
unsigned long. On RV32, unsigned long is 32 bits, which would truncate 64-bit
flags.

Furthermore, when clearing a flag via cfg->henvcfg &= ~flag, if flag is 0,
~flag evaluates to the 32-bit 0xFFFFFFFF. Because cfg->henvcfg is a u64,
the 32-bit mask is zero-extended to 0x00000000FFFFFFFFULL.

This zero-extension permanently wipes the entire upper 32 bits of henvcfg and
corrupts hardware states like Svpbmt and Zicbom on 32-bit KVM hosts.

[Severity: High]
Does modifying FWFT features via KVM_SET_ONE_REG lead to stale hardware state?

When userspace modifies an FWFT feature via KVM_SET_ONE_REG, one_reg_access is
true. In kvm_sbi_fwft_env_flag_set_helper():

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

This updates the in-memory vcpu->arch.cfg.henvcfg but skips the immediate CSR
write. However, it fails to set vcpu->arch.csr_dirty = true.

When vcpu_load() later executes, it evaluates !vcpu->arch.csr_dirty and skips
kvm_riscv_vcpu_config_load(), causing the guest to run with stale hardware
configurations.

[Severity: High]
Does writing directly to CSR_HENVCFG bypass nested acceleration?

When one_reg_access is false, kvm_sbi_fwft_env_flag_set_helper() directly
writes the hardware CSR using csr_write(CSR_HENVCFG, ...).

When KVM runs as an L1 nested hypervisor with RISC-V nested acceleration
enabled, hardware CSRs must be synchronized with the L0 hypervisor via a
shared memory region.

Bypassing the ncsr_write() wrapper fails to set the dirty bit in the shared
memory bitmap, meaning the L0 hypervisor remains unaware of the change and
will overwrite the hardware CSR with a stale value on the next nested VM entry.

[ ... ]
> +static bool kvm_sbi_fwft_shadow_stack_supported(struct kvm_vcpu *vcpu)
> +{
> +	return riscv_isa_extension_available(vcpu->arch.isa, ZICFISS);
> +}

[Severity: High]
Does exposing ZICFISS without context-switching CSR_SSP leak guest state into
the host?

When a guest enables and uses the shadow stack, it writes to the shadow stack
pointer CSR_SSP. KVM does not currently save or restore CSR_SSP during VCPU
context switches.

Since U-mode CSRs are not automatically banked by the RISC-V hardware between
host and guest execution modes, the guest's shadow stack pointer remains active
when returning to the host.

When the host's userspace resumes execution, it does so with a corrupted shadow
stack pointer, leading to shadow stack violations and a denial-of-service
crash.

[ ... ]
> @@ -288,6 +340,24 @@ static const struct kvm_sbi_fwft_feature features[] = {
>  		.set = kvm_sbi_fwft_set_misaligned_delegation,
>  		.get = kvm_sbi_fwft_get_misaligned_delegation,
>  	},
> +	{
> +		.id = SBI_FWFT_LANDING_PAD,
> +		.first_reg_num = offsetof(struct kvm_riscv_sbi_fwft, landing_pad.enable) /
> +				 sizeof(unsigned long),
> +		.supported = kvm_sbi_fwft_landing_pad_supported,
> +		.reset = kvm_sbi_fwft_reset_landing_pad,
> +		.set = kvm_sbi_fwft_set_landing_pad,
> +		.get = kvm_sbi_fwft_get_landing_pad,
> +	},

[Severity: Medium]
Are SBI FWFT flags committed even if the set operation fails?

Looking at kvm_sbi_fwft_set(), the flags are assigned before the
feature-specific set callback is executed:

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

If the set callback fails and returns an error like SBI_ERR_INVALID_PARAM,
the flags are not rolled back.

If the guest request included SBI_FWFT_SET_FLAG_LOCK, the feature becomes
permanently locked into a stale state, preventing subsequent corrections.

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