Re: [PATCH v4 4/8] RISC-V: KVM: Add ssp context save/restore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing addition of the ZICFISS register count in `num_csr_regs()` causes KVM to under-calculate the required register list size, leading to an out-of-bounds write during the `KVM_GET_REG_LIST` ioctl.
- [Medium] Modifying CSR_SSP in C code before entering the VCPU assembly loop, and zeroing it upon return, overwrites the host's active Shadow Stack Pointer, potentially causing a host kernel panic if the host utilizes Supervisor-mode shadow stacks.
- [Medium] The guest's ZICFISS Shadow Stack Pointer (`zicfiss_csr`) is not cleared during VCPU context reset, leading to guest state leakage across reboots.
--

commit 3d9e79d289458c01fff126fe0b09e2d1859aeecc
Author: Inochi Amaoto <inochiama@gmail.com>

RISC-V: KVM: Add ssp context save/restore

This commit adds ssp context save/restore for guest VCPUs. It also adds the
Zicfiss registers to the ONE_REG interface to allow access from user space.

> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 60017ceec9d2a..e5ed3b0e5a55b 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
[ ... ]
> @@ -203,6 +207,9 @@ struct kvm_vcpu_arch {
>  	/* CPU Smstateen CSR context of Guest VCPU */
>  	struct kvm_vcpu_smstateen_csr smstateen_csr;
>  
> +	/* CPU Zicfiss CSR context of Guest VCPU */
> +	struct kvm_vcpu_zicfiss_csr zicfiss_csr;
> +

[Severity: Medium]
Is the zicfiss_csr structure cleared during a VCPU reset?

Looking at kvm_riscv_vcpu_context_reset() in arch/riscv/kvm/vcpu.c,
it doesn't appear to be zeroed:

	memset(cntx, 0, sizeof(*cntx));
	memset(csr, 0, sizeof(*csr));
	memset(&vcpu->arch.smstateen_csr, 0, sizeof(vcpu->arch.smstateen_csr));

Could this lead to the guest's shadow stack pointer being preserved
across warm reboots or kexec, leaking architectural state?

>  	/* CPU reset state of Guest VCPU */
>  	struct kvm_vcpu_reset_state reset_state;
>  
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index cf6e231e76e25..1c2775a4834eb 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -720,6 +720,7 @@ static void kvm_riscv_update_hvip(struct kvm_vcpu *vcpu)
>  
>  static __always_inline void kvm_riscv_vcpu_swap_in_guest_state(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_vcpu_zicfiss_csr *zicficsr = &vcpu->arch.zicfiss_csr;
>  	struct kvm_vcpu_smstateen_csr *smcsr = &vcpu->arch.smstateen_csr;
>  	struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
>  
> @@ -727,10 +728,13 @@ static __always_inline void kvm_riscv_vcpu_swap_in_guest_state(struct kvm_vcpu *
>  	vcpu->arch.host_senvcfg = csr_swap(CSR_SENVCFG, csr->senvcfg);
>  	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN))
>  		vcpu->arch.host_sstateen0 = csr_swap(CSR_SSTATEEN0, smcsr->sstateen0);
> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICFISS))
> +		csr_write(CSR_SSP, zicficsr->ssp);

[Severity: Medium]
Since this modifies the host's CSR_SSP while still executing host C code,
will this cause any subsequent host function returns (before entering
assembly) to use the guest's shadow stack?

If the host kernel utilizes hardware shadow stacks for Supervisor mode,
this could result in a host kernel panic from an sspopchk failure.

>  }
>  
>  static __always_inline void kvm_riscv_vcpu_swap_in_host_state(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_vcpu_zicfiss_csr *zicficsr = &vcpu->arch.zicfiss_csr;
>  	struct kvm_vcpu_smstateen_csr *smcsr = &vcpu->arch.smstateen_csr;
>  	struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
>  
> @@ -738,6 +742,8 @@ static __always_inline void kvm_riscv_vcpu_swap_in_host_state(struct kvm_vcpu *v
>  	csr->senvcfg = csr_swap(CSR_SENVCFG, vcpu->arch.host_senvcfg);
>  	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN))
>  		smcsr->sstateen0 = csr_swap(CSR_SSTATEEN0, vcpu->arch.host_sstateen0);
> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICFISS))
> +		zicficsr->ssp = csr_swap(CSR_SSP, 0);

[Severity: Medium]
Will hardcoding CSR_SSP to 0 here overwrite the host's shadow stack pointer
rather than correctly restoring the saved host state?

>  }
>  
>  /*
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index bb920e8923c93..bd59aebc8b2e9 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
[ ... ]
> @@ -688,7 +732,7 @@ static int copy_csr_reg_indices(const struct kvm_vcpu *vcpu,
>  				u64 __user *uindices)
>  {
>  	int n1 = sizeof(struct kvm_riscv_csr) / sizeof(unsigned long);
> -	int n2 = 0, n3 = 0;
> +	int n2 = 0, n3 = 0, n4 = 0;
>  
>  	/* copy general csr regs */
>  	for (int i = 0; i < n1; i++) {
> @@ -740,7 +784,25 @@ static int copy_csr_reg_indices(const struct kvm_vcpu *vcpu,
>  		}
>  	}
>  
> -	return n1 + n2 + n3;
> +	/* copy Zicfiss csr regs */
> +	if (riscv_isa_extension_available(vcpu->arch.isa, ZICFISS)) {
> +		n4 = sizeof(struct kvm_riscv_zicfiss_csr) / sizeof(unsigned long);
> +
> +		for (int i = 0; i < n4; i++) {
> +			u64 size = IS_ENABLED(CONFIG_32BIT) ?
> +				   KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64;
> +			u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_CSR |
> +					  KVM_REG_RISCV_CSR_ZICFISS | i;
> +
> +			if (uindices) {
> +				if (put_user(reg, uindices))
> +					return -EFAULT;
> +				uindices++;
> +			}
> +		}
> +	}
> +
> +	return n1 + n2 + n3 + n4;
>  }

[Severity: High]
Does num_csr_regs() also need to be updated to include the ZICFISS register
count?

If num_csr_regs() doesn't include the ZICFISS registers, userspace might
allocate a smaller array for KVM_GET_REG_LIST, causing the put_user()
calls above to write past the end of the userspace array.

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