* [PATCH] RISC-V: KVM: Publish NACL HFENCE configuration last
@ 2026-09-06 8:07 ` Pengpeng Hou
0 siblings, 0 replies; 7+ messages in thread
From: Pengpeng Hou @ 2026-09-06 8:07 UTC (permalink / raw)
To: Anup Patel, Atish Patra
Cc: Pengpeng Hou, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, kvm, kvm-riscv, linux-riscv, linux-kernel,
Andrew Jones
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.
Write the payload first and order those stores before publishing the
configuration word with WRITE_ONCE(). Use a write barrier that also
applies to UP kernels, since the consumer is outside the Linux SMP
domain.
Keep nacl_hfence_mkconfig() unchanged: setting Pending in a local value
does not publish the entry. The shared-memory store is the publication
point.
The issue was found by our static-analysis tool and manually reviewed.
Fixes: d466c19cead5 ("RISC-V: KVM: Add common nested acceleration support")
Assisted-by: GPT-5
Signed-off-by: Pengpeng Hou <hppiscas@163.com>
---
diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
index 9aff03c4..c5c4907a 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 @@ again:
}
}
- 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));
}
int kvm_riscv_nacl_enable(void)
base-commit: 4d7d9486c04d917265f64c55bd23b2cc4fe7749c
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH] RISC-V: KVM: Publish NACL HFENCE configuration last
@ 2026-09-06 8:07 ` Pengpeng Hou
0 siblings, 0 replies; 7+ messages in thread
From: Pengpeng Hou @ 2026-09-06 8:07 UTC (permalink / raw)
To: Anup Patel, Atish Patra
Cc: Pengpeng Hou, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, kvm, kvm-riscv, linux-riscv, linux-kernel,
Andrew Jones
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.
Write the payload first and order those stores before publishing the
configuration word with WRITE_ONCE(). Use a write barrier that also
applies to UP kernels, since the consumer is outside the Linux SMP
domain.
Keep nacl_hfence_mkconfig() unchanged: setting Pending in a local value
does not publish the entry. The shared-memory store is the publication
point.
The issue was found by our static-analysis tool and manually reviewed.
Fixes: d466c19cead5 ("RISC-V: KVM: Add common nested acceleration support")
Assisted-by: GPT-5
Signed-off-by: Pengpeng Hou <hppiscas@163.com>
---
diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
index 9aff03c4..c5c4907a 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 @@ again:
}
}
- 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));
}
int kvm_riscv_nacl_enable(void)
base-commit: 4d7d9486c04d917265f64c55bd23b2cc4fe7749c
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] RISC-V: KVM: Publish NACL HFENCE configuration last
2026-09-06 8:07 ` Pengpeng Hou
(?)
(?)
@ 2026-09-06 8:19 ` sashiko-bot
-1 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-09-06 8:19 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: kvm
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] RISC-V: KVM: Publish NACL HFENCE configuration last
2026-09-06 8:07 ` Pengpeng Hou
(?)
@ 2026-09-12 7:19 ` Anup Patel
-1 siblings, 0 replies; 7+ messages in thread
From: Anup Patel @ 2026-09-12 7:19 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Atish Patra, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, kvm, kvm-riscv, linux-riscv, linux-kernel,
Andrew Jones
On Sun, Sep 6, 2026 at 1:37 PM Pengpeng Hou <hppiscas@163.com> wrote:
>
> 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.
>
> Write the payload first and order those stores before publishing the
> configuration word with WRITE_ONCE(). Use a write barrier that also
> applies to UP kernels, since the consumer is outside the Linux SMP
> domain.
>
> Keep nacl_hfence_mkconfig() unchanged: setting Pending in a local value
> does not publish the entry. The shared-memory store is the publication
> point.
>
> The issue was found by our static-analysis tool and manually reviewed.
>
> Fixes: d466c19cead5 ("RISC-V: KVM: Add common nested acceleration support")
> Assisted-by: GPT-5
> Signed-off-by: Pengpeng Hou <hppiscas@163.com>
There was another patch from Zongmin Zhou which is relatively
more complete hence I have merged that one.
Regards,
Anup
> ---
>
> diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
> index 9aff03c4..c5c4907a 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 @@ again:
> }
> }
>
> - 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));
> }
>
> int kvm_riscv_nacl_enable(void)
>
> base-commit: 4d7d9486c04d917265f64c55bd23b2cc4fe7749c
>
--
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] RISC-V: KVM: Publish NACL HFENCE configuration last
@ 2026-09-12 7:19 ` Anup Patel
0 siblings, 0 replies; 7+ messages in thread
From: Anup Patel @ 2026-09-12 7:19 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Atish Patra, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, kvm, kvm-riscv, linux-riscv, linux-kernel,
Andrew Jones
On Sun, Sep 6, 2026 at 1:37 PM Pengpeng Hou <hppiscas@163.com> wrote:
>
> 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.
>
> Write the payload first and order those stores before publishing the
> configuration word with WRITE_ONCE(). Use a write barrier that also
> applies to UP kernels, since the consumer is outside the Linux SMP
> domain.
>
> Keep nacl_hfence_mkconfig() unchanged: setting Pending in a local value
> does not publish the entry. The shared-memory store is the publication
> point.
>
> The issue was found by our static-analysis tool and manually reviewed.
>
> Fixes: d466c19cead5 ("RISC-V: KVM: Add common nested acceleration support")
> Assisted-by: GPT-5
> Signed-off-by: Pengpeng Hou <hppiscas@163.com>
There was another patch from Zongmin Zhou which is relatively
more complete hence I have merged that one.
Regards,
Anup
> ---
>
> diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
> index 9aff03c4..c5c4907a 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 @@ again:
> }
> }
>
> - 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));
> }
>
> int kvm_riscv_nacl_enable(void)
>
> base-commit: 4d7d9486c04d917265f64c55bd23b2cc4fe7749c
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] RISC-V: KVM: Publish NACL HFENCE configuration last
@ 2026-09-12 7:19 ` Anup Patel
0 siblings, 0 replies; 7+ messages in thread
From: Anup Patel @ 2026-09-12 7:19 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Atish Patra, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, kvm, kvm-riscv, linux-riscv, linux-kernel,
Andrew Jones
On Sun, Sep 6, 2026 at 1:37 PM Pengpeng Hou <hppiscas@163.com> wrote:
>
> 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.
>
> Write the payload first and order those stores before publishing the
> configuration word with WRITE_ONCE(). Use a write barrier that also
> applies to UP kernels, since the consumer is outside the Linux SMP
> domain.
>
> Keep nacl_hfence_mkconfig() unchanged: setting Pending in a local value
> does not publish the entry. The shared-memory store is the publication
> point.
>
> The issue was found by our static-analysis tool and manually reviewed.
>
> Fixes: d466c19cead5 ("RISC-V: KVM: Add common nested acceleration support")
> Assisted-by: GPT-5
> Signed-off-by: Pengpeng Hou <hppiscas@163.com>
There was another patch from Zongmin Zhou which is relatively
more complete hence I have merged that one.
Regards,
Anup
> ---
>
> diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
> index 9aff03c4..c5c4907a 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 @@ again:
> }
> }
>
> - 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));
> }
>
> int kvm_riscv_nacl_enable(void)
>
> base-commit: 4d7d9486c04d917265f64c55bd23b2cc4fe7749c
>
^ permalink raw reply [flat|nested] 7+ messages in thread