Re: [PATCH v1] riscv: kvm: Use endian-specific __lelong for NACL shared memory

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Infinite loop in `__kvm_riscv_nacl_hfence` due to missing `try_count` decrement.
- [High] Race condition and memory ordering bugs in NACL shared memory updates.
--

commit 041fd3e0bc4c133e6f04a198d4795da99daa15bb
Author: Sean Chang <seanwascoding@gmail.com>

riscv: kvm: Use endian-specific __lelong for NACL shared memory

This patch fixes bitwise type warnings from sparse by introducing __lelong
and replacing raw unsigned long pointers in SBI NACL shared memory. It also
adds a __force cast to __get_user_asm to safely suppress implicit cast
warnings.

> diff --git a/arch/riscv/include/asm/kvm_nacl.h b/arch/riscv/include/asm/kvm_nacl.h
> index 4124d5e06a0ff..f45407bcaa26a 100644
> --- a/arch/riscv/include/asm/kvm_nacl.h
> +++ b/arch/riscv/include/asm/kvm_nacl.h

[ ... ]

> @@ -176,7 +178,7 @@ __kvm_riscv_nacl_hfence(__shmem,					\
>  do {									\
>  	void *__s = (__shmem);						\
>  	unsigned int __i = SBI_NACL_SHMEM_CSR_INDEX(__csr);		\
> -	unsigned long *__a = (__s) + SBI_NACL_SHMEM_CSR_OFFSET;		\
> +	__lelong *__a = (__s) + SBI_NACL_SHMEM_CSR_OFFSET;		\
>  	u8 *__b = (__s) + SBI_NACL_SHMEM_DBITMAP_OFFSET;		\
>  	__a[__i] = cpu_to_lelong(__val);				\
>  	__b[__i >> 3] |= 1U << (__i & 0x7);				\

[Severity: High]
This is a pre-existing issue, but does the lack of a memory barrier between
the CSR value update and the dirty bit update risk a race condition?

On RISC-V's relaxed memory model, these stores could be reordered without an
smp_wmb(), potentially allowing the hardware or firmware to observe the
dirty bit before the payload is visible in nacl_csr_write.

> @@ -186,7 +188,7 @@ do {									\
>  ({									\
>  	void *__s = (__shmem);						\
>  	unsigned int __i = SBI_NACL_SHMEM_CSR_INDEX(__csr);		\
> -	unsigned long *__a = (__s) + SBI_NACL_SHMEM_CSR_OFFSET;		\
> +	__lelong *__a = (__s) + SBI_NACL_SHMEM_CSR_OFFSET;		\
>  	u8 *__b = (__s) + SBI_NACL_SHMEM_DBITMAP_OFFSET;		\
>  	unsigned long __r = lelong_to_cpu(__a[__i]);			\
>  	__a[__i] = cpu_to_lelong(__val);				\
>  	__b[__i >> 3] |= 1U << (__i & 0x7);				\

[Severity: High]
This isn't a bug introduced by this patch, but similar to nacl_csr_write,
could these writes be reordered by the CPU in the nacl_csr_swap macro?

[ ... ]

> diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
> index 08a95ad9ada2c..6f9f8963e9ddc 100644
> --- a/arch/riscv/kvm/nacl.c
> +++ b/arch/riscv/kvm/nacl.c
> @@ -20,7 +20,7 @@ void __kvm_riscv_nacl_hfence(void *shmem,
>  			     unsigned long page_count)
>  {
>  	int i, ent = -1, try_count = 5;
> -	unsigned long *entp;
> +	__lelong *entp;
>  
>  again:
>  	for (i = 0; i < SBI_NACL_SHMEM_HFENCE_ENTRY_MAX; i++) {

[Severity: High]
This is also a pre-existing issue, but does this retry loop run infinitely
if the queue remains full?

Looking at __kvm_riscv_nacl_hfence(), it appears try_count is never
decremented before taking the goto:

    if (ent < 0) {
        if (try_count) {
            nacl_sync_hfence(-1UL);
            goto again;
        } else {
            pr_warn("KVM: No free entry in NACL shared memory\n");
            return;
        }
    }

Additionally, there seems to be a pre-existing ordering hazard when writing
the entry payload further down in __kvm_riscv_nacl_hfence():

    entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_CONFIG(i);
    *entp = cpu_to_lelong(control);
    entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_PNUM(i);
    *entp = cpu_to_lelong(page_num);
    entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_PCOUNT(i);
    *entp = cpu_to_lelong(page_count);

Since the CONFIG element contains the valid bit, writing it before PNUM and
PCOUNT could allow another vCPU on the same physical CPU to trigger
nacl_sync_hfence() if KVM is preempted right after the CONFIG write. Would
this cause the firmware to process uninitialized payload data?

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