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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect usage of EXC_SOFTWARE_CHECK as a direct integer instead of a bitmask when configuring hedeleg, causing unintended exception delegation and un-delegation.
--

commit 061583fda9a0d491f0c4c0df10fbb4b0f97c7b4e
Author: Inochi Amaoto <inochiama@gmail.com>

RISC-V: KVM: Add support for control-flow integrity FWFT features

This commit exposes control-flow integrity features to the guest if
supported by the host, and allows the VMM to configure them via the SBI
FWFT interface.

> diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
> index 7192c229a19e2..cb9b9721ec882 100644
> --- a/arch/riscv/kvm/vcpu_sbi_fwft.c
> +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c

[ ... ]

> @@ -176,6 +176,95 @@ static long kvm_sbi_fwft_get_misaligned_delegation(struct kvm_vcpu *vcpu,
>  	return SBI_SUCCESS;
>  }
>  
> +static long kvm_sbi_fwft_set_cfi(struct kvm_vcpu *vcpu,
> +				 struct kvm_sbi_fwft_config *conf,
> +				 bool one_reg_access, unsigned long value,
> +				 u64 flag)
> +{
> +	struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
> +
> +	if (value == 0)
> +		cfg->henvcfg &= ~flag;
> +	else if (value == 1)
> +		cfg->henvcfg |= flag;
> +	else
> +		return SBI_ERR_INVALID_PARAM;
> +
> +	if (cfg->henvcfg & (ENVCFG_LPE | ENVCFG_SSE))
> +		cfg->hedeleg |= EXC_SOFTWARE_CHECK;
> +	else
> +		cfg->hedeleg &= ~EXC_SOFTWARE_CHECK;

[Severity: High]
Does this code modify the correct bits in hedeleg?

EXC_SOFTWARE_CHECK is an exception cause (value 18, or 0x12). When used
directly in bitwise operations like this, it seems to modify bit 1
(instruction access fault) and bit 4 (load address misaligned) rather than
bit 18.

Should this use BIT(EXC_SOFTWARE_CHECK) or (1UL << EXC_SOFTWARE_CHECK)
instead to ensure the software check exception is delegated properly without
corrupting the other delegation states?

> +
> +	if (!one_reg_access) {
> +		csr_write(CSR_HEDELEG, cfg->hedeleg);

[ ... ]

> +static void kvm_sbi_fwft_reset_landing_pad(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
> +
> +	kvm_sbi_fwft_env_flag_reset_helper(vcpu, ENVCFG_LPE);
> +	if ((cfg->henvcfg & (ENVCFG_LPE | ENVCFG_SSE)) == 0)
> +		cfg->hedeleg &= ~EXC_SOFTWARE_CHECK;
> +}

[Severity: High]
Similar to the question above in kvm_sbi_fwft_set_cfi(), could this
undelegate the wrong exceptions because EXC_SOFTWARE_CHECK is used without
being converted to a bitmask first?

[ ... ]

> +static void kvm_sbi_fwft_reset_shadow_stack(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
> +
> +	kvm_sbi_fwft_env_flag_reset_helper(vcpu, ENVCFG_SSE);
> +	if ((cfg->henvcfg & (ENVCFG_LPE | ENVCFG_SSE)) == 0)
> +		cfg->hedeleg &= ~EXC_SOFTWARE_CHECK;
> +}

[Severity: High]
Would it be necessary to update this location to use a bitmask as well?

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