Re: [PATCH] RISC-V: KVM: Publish NACL HFENCE configuration last

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] KVM silently drops TLB flush (HFENCE) requests when the NACL shared memory is exhausted, leading to guest memory corruption and use-after-free vulnerabilities.
- [High] Per-CPU `nacl_shmem()` is evaluated and accessed in a preemptible context, causing a race condition that corrupts shared memory entries and drops TLB flushes.
- [High] The shared memory entry configuration is read without `READ_ONCE()`, violating Linux Kernel Memory Model (LKMM) rules for shared memory and control dependencies.
--

commit c8294fdfde274970a69be416dee2db8089a33132
Author: Pengpeng Hou <hppiscas@163.com>
Subject: RISC-V: KVM: Publish NACL HFENCE configuration last

The SBI NACL interface requires software to write Page_Number and
Page_Count before publishing an HFENCE entry with Config.Pending set.
__kvm_riscv_nacl_hfence() currently stores the pending configuration first.

> diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
> index 9aff03c4f667a..c5c4907aca08a 100644
> --- a/arch/riscv/kvm/nacl.c
> +++ b/arch/riscv/kvm/nacl.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/kvm_host.h>
>  #include <linux/vmalloc.h>
> +#include <asm/barrier.h>
>  #include <asm/kvm_nacl.h>
>  
>  DEFINE_STATIC_KEY_FALSE(kvm_riscv_nacl_available);
> @@ -42,12 +43,15 @@ void __kvm_riscv_nacl_hfence(void *shmem,

[Severity: High]
This isn't a bug introduced by this patch, but in __kvm_riscv_nacl_hfence(),
KVM silently drops TLB flush requests when the NACL shared memory fills up.

When the shared memory queue fills up, the function calls
nacl_sync_hfence(-1UL) and retries 5 times. If the firmware is sluggish or
fails to drain the queue in time, it returns void without notifying the
caller:

arch/riscv/kvm/nacl.c:__kvm_riscv_nacl_hfence() {
    ...
	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;
		}
	}
    ...
}

Could KVM return a hard error or BUG() instead if a TLB flush cannot be
completed, to prevent guests from accessing stale physical memory mappings
and causing data corruption or use-after-free?

[Severity: High]
This is a pre-existing issue, but does reading the shared memory entry
configuration without READ_ONCE() violate the Linux Kernel Memory Model for
shared memory and control dependencies?

arch/riscv/kvm/nacl.c:__kvm_riscv_nacl_hfence() {
    ...
again:
	for (i = 0; i < SBI_NACL_SHMEM_HFENCE_ENTRY_MAX; i++) {
		entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_CONFIG(i);
		if (lelong_to_cpu(*entp) & SBI_NACL_SHMEM_HFENCE_CONFIG_PEND)
			continue;
    ...
}

While this patch correctly adds a WRITE_ONCE() and a barrier for publishing
the payload, the read side is left unprotected. Without READ_ONCE() around
*entp, might the compiler optimize the read from shared memory by caching the
value or issuing multiple loads, which could result in an infinite loop or
incorrect synchronization with the firmware?

>  		}
>  	}
>  
> -	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);
> +
> +	/* Publish the payload before setting Config.Pending. */
> +	wmb();
> +	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_CONFIG(i);
> +	WRITE_ONCE(*entp, cpu_to_lelong(control));
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to evaluate and
access the per-CPU nacl_shmem() in a preemptible context?

In kvm_arch_vcpu_ioctl_run(), vCPU requests are processed before preemption
is disabled:

arch/riscv/kvm/vcpu.c:kvm_arch_vcpu_ioctl_run() {
    ...
		ret = kvm_riscv_check_vcpu_requests(vcpu);
		if (ret <= 0)
			continue;

		preempt_disable();
    ...
}

When processing HFENCE requests, kvm_riscv_hfence_process() evaluates
nacl_shmem(), which expands to a per-CPU pointer:

arch/riscv/include/asm/kvm_nacl.h:
#define nacl_shmem()							\
	this_cpu_ptr(&kvm_riscv_nacl)->shmem

arch/riscv/kvm/tlb.c:kvm_riscv_hfence_process() {
    ...
				nacl_hfence_gvma_vmid(nacl_shmem(), d.vmid,
						      d.addr, d.size, d.order);
    ...
}

If the vCPU thread is preempted and migrated to another CPU after evaluating
nacl_shmem(), wouldn't this cause a race condition that overwrites another
CPU's TLB flush queue, potentially leading to dropped flushes and guest
isolation breaches?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/caplitmus-rv.c91c005b844342d9a9f60949b04c520d.d65@163.com?part=1