* [PATCH v2 01/13] KVM: arm64: Donate MMIO to the hypervisor
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 16:58 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 02/13] KVM: arm64: Track host-unmapped MMIO regions in a static array Sebastian Ene
` (11 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
From: Mostafa Saleh <smostafa@google.com>
Extend the pKVM API to allow the donation of MMIO from the host
address space to the hypervisor linear map.
Initialize the host s2 page table with an invalid leaf with the owner ID
of the hypervisor to prevent the host from mapping the page on faults.
Prevent kvm_pgtable_stage2_unmap() from removing owner ID from
stage-2 PTEs, as this can be triggered from recycle logic under memory
pressure.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 7 +
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 137 +++++++++++++++++-
arch/arm64/kvm/hyp/pgtable.c | 11 +-
3 files changed, 148 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
index 29935c7da1de..6aa83b129e61 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
@@ -36,6 +36,13 @@ int __pkvm_guest_share_host(struct pkvm_hyp_vcpu *vcpu, u64 gfn);
int __pkvm_guest_unshare_host(struct pkvm_hyp_vcpu *vcpu, u64 gfn);
int __pkvm_host_unshare_hyp(u64 pfn);
int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages);
+/*
+ * Donate MMIO range to the hypervisor, it will be mapped in the hypervisor's
+ * linea map and unmapped from the host stage-2.
+ */
+int __pkvm_host_donate_hyp_mmio(phys_addr_t addr, size_t size);
+/* Remaps MMIO range in the host, typically used in error path. */
+int __pkvm_hyp_donate_host_mmio(phys_addr_t addr, size_t size);
int __pkvm_hyp_donate_host(u64 pfn, u64 nr_pages);
int __pkvm_host_share_ffa(u64 pfn, u64 nr_pages);
int __pkvm_host_unshare_ffa(u64 pfn, u64 nr_pages);
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 4e329e39a695..5cf7c4a0ed20 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -378,7 +378,11 @@ static int host_stage2_unmap_dev_all(void)
u64 addr = 0;
int i, ret;
- /* Unmap all non-memory regions to recycle the pages */
+ /*
+ * Unmap all non-memory regions to recycle the pages.
+ * That relies on kvm_pgtable_stage2_unmap() not clearing
+ * counted PTEs which include hypervisor MMIO.
+ */
for (i = 0; i < hyp_memblock_nr; i++, addr = reg->base + reg->size) {
reg = &hyp_memory[i];
ret = kvm_pgtable_stage2_unmap(pgt, addr, reg->base - addr);
@@ -1119,6 +1123,137 @@ int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages)
return ret;
}
+int __pkvm_host_donate_hyp_mmio(phys_addr_t addr, size_t size)
+{
+ kvm_pte_t pte;
+ u64 offset;
+ void *virt;
+ int ret;
+
+ /* Only before de-privilege. */
+ if (static_branch_unlikely(&kvm_protected_mode_initialized))
+ return -EPERM;
+
+ if (!PAGE_ALIGNED(addr | size) ||
+ !pfn_range_is_valid(hyp_phys_to_pfn(addr), size >> PAGE_SHIFT))
+ return -EINVAL;
+
+ host_lock_component();
+ hyp_lock_component();
+
+ for (offset = 0; offset < size; offset += PAGE_SIZE) {
+ if (addr_is_memory(addr + offset)) {
+ ret = -EINVAL;
+ goto err_with_mapping;
+ }
+
+ ret = kvm_pgtable_get_leaf(&host_mmu.pgt, addr + offset, &pte, NULL);
+ if (ret)
+ goto err_with_mapping;
+
+ if (pte && !kvm_pte_valid(pte)) {
+ ret = -EPERM;
+ goto err_with_mapping;
+ }
+
+ virt = __hyp_va(addr + offset);
+ ret = kvm_pgtable_get_leaf(&pkvm_pgtable, (u64)virt, &pte, NULL);
+ if (ret)
+ goto err_with_mapping;
+ if (pte) {
+ ret = -EBUSY;
+ goto err_with_mapping;
+ }
+
+ ret = pkvm_create_mappings_locked(virt, virt + PAGE_SIZE, PAGE_HYP_DEVICE);
+ if (ret)
+ goto err_with_mapping;
+ }
+
+ /*
+ * We set HYP as the owner of the MMIO pages in the host stage-2, for:
+ * - host aborts: host_stage2_adjust_range() would fail for invalid non zero PTEs.
+ * - recycle under memory pressure: host_stage2_unmap_dev_all() would call
+ * kvm_pgtable_stage2_unmap() which will not clear non zero invalid ptes (counted).
+ * - other MMIO donation: Would fail as we check that the PTE is valid or empty.
+ */
+ ret = host_stage2_try(kvm_pgtable_stage2_annotate, &host_mmu.pgt,
+ addr, size, &host_s2_pool,
+ KVM_HOST_INVALID_PTE_TYPE_DONATION,
+ FIELD_PREP(KVM_HOST_DONATION_PTE_OWNER_MASK, PKVM_ID_HYP));
+ if (ret)
+ goto err_with_mapping;
+unlock:
+ hyp_unlock_component();
+ host_unlock_component();
+ return ret;
+err_with_mapping:
+ if (!offset)
+ goto unlock;
+
+ while (offset) {
+ offset -= PAGE_SIZE;
+ virt = __hyp_va(addr + offset);
+ WARN_ON(kvm_pgtable_hyp_unmap(&pkvm_pgtable, (u64)virt, PAGE_SIZE) != PAGE_SIZE);
+ }
+ goto unlock;
+}
+
+int __pkvm_hyp_donate_host_mmio(phys_addr_t addr, size_t size)
+{
+ kvm_pte_t pte;
+ u64 offset;
+ int ret = 0;
+ void *virt;
+
+ if (static_branch_unlikely(&kvm_protected_mode_initialized))
+ return -EPERM;
+
+ if (!PAGE_ALIGNED(addr | size) ||
+ !pfn_range_is_valid(hyp_phys_to_pfn(addr), size >> PAGE_SHIFT))
+ return -EINVAL;
+
+ host_lock_component();
+ hyp_lock_component();
+
+ for (offset = 0; offset < size; offset += PAGE_SIZE) {
+ if (addr_is_memory(addr + offset)) {
+ ret = -EINVAL;
+ goto err_with_unmap;
+ }
+ ret = kvm_pgtable_get_leaf(&host_mmu.pgt, addr + offset, &pte, NULL);
+ if (ret)
+ goto err_with_unmap;
+ if (!pte || kvm_pte_valid(pte)) {
+ ret = -EINVAL;
+ goto err_with_unmap;
+ }
+ if (FIELD_GET(KVM_HOST_DONATION_PTE_OWNER_MASK, pte) != PKVM_ID_HYP) {
+ ret = -EPERM;
+ goto err_with_unmap;
+ }
+
+ virt = __hyp_va(addr + offset);
+ if (kvm_pgtable_hyp_unmap(&pkvm_pgtable, (u64)virt, PAGE_SIZE) != PAGE_SIZE)
+ goto err_with_unmap;
+ }
+ WARN_ON(host_stage2_idmap_locked(addr, size, PKVM_HOST_MMIO_PROT));
+unlock:
+ hyp_unlock_component();
+ host_unlock_component();
+ return ret;
+err_with_unmap:
+ if (!offset)
+ goto unlock;
+
+ while (offset) {
+ offset -= PAGE_SIZE;
+ virt = __hyp_va(addr + offset);
+ WARN_ON(pkvm_create_mappings_locked(virt, virt + PAGE_SIZE, PAGE_HYP_DEVICE));
+ }
+ goto unlock;
+}
+
int __pkvm_hyp_donate_host(u64 pfn, u64 nr_pages)
{
u64 phys = hyp_pfn_to_phys(pfn);
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index b74dd5ce1efd..7638213bd893 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1161,13 +1161,12 @@ static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx,
kvm_pte_t *childp = NULL;
bool need_flush = false;
- if (!kvm_pte_valid(ctx->old)) {
- if (stage2_pte_is_counted(ctx->old)) {
- kvm_clear_pte(ctx->ptep);
- mm_ops->put_page(ctx->ptep);
- }
+ /*
+ * That also ignores stage2_pte_is_counted() instead of clearing
+ * the PTE as the MMIO can be owned by the hypervisor.
+ */
+ if (!kvm_pte_valid(ctx->old))
return 0;
- }
if (kvm_pte_table(ctx->old, ctx->level)) {
childp = kvm_pte_follow(ctx->old, mm_ops);
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 01/13] KVM: arm64: Donate MMIO to the hypervisor
2026-08-07 16:43 ` [PATCH v2 01/13] KVM: arm64: Donate MMIO to the hypervisor Sebastian Ene
@ 2026-08-07 16:58 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 16:58 UTC (permalink / raw)
To: Sebastian Ene; +Cc: kvmarm, Oliver Upton, Marc Zyngier
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] In `__pkvm_hyp_donate_host_mmio`, if `kvm_pgtable_hyp_unmap` fails, the function jumps to the error path but fails to set `ret` to an error code, silently returning success (0).
--
--- Patch [1]: [PATCH v2 01/13] KVM: arm64: Donate MMIO to the hypervisor ---
commit 91906bfffc8e911901f2cb86959abcf8b184a527
Author: Mostafa Saleh <smostafa@google.com>
KVM: arm64: Donate MMIO to the hypervisor
This patch introduces the ability for the host to donate MMIO ranges to
the hypervisor's linear map in pKVM. It initializes the host stage-2 page
table with an invalid leaf containing the hypervisor's owner ID to prevent
host mapping faults.
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 4e329e39a695a..5cf7c4a0ed208 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
[ ... ]
> +int __pkvm_hyp_donate_host_mmio(phys_addr_t addr, size_t size)
> +{
> + kvm_pte_t pte;
> + u64 offset;
> + int ret = 0;
> + void *virt;
[ ... ]
> + for (offset = 0; offset < size; offset += PAGE_SIZE) {
> + if (addr_is_memory(addr + offset)) {
> + ret = -EINVAL;
> + goto err_with_unmap;
> + }
> + ret = kvm_pgtable_get_leaf(&host_mmu.pgt, addr + offset, &pte, NULL);
> + if (ret)
> + goto err_with_unmap;
[ ... ]
> + virt = __hyp_va(addr + offset);
> + if (kvm_pgtable_hyp_unmap(&pkvm_pgtable, (u64)virt, PAGE_SIZE) != PAGE_SIZE)
> + goto err_with_unmap;
[Severity: High]
When kvm_pgtable_hyp_unmap() fails here, does the code return success?
Looking at __pkvm_hyp_donate_host_mmio() in mem_protect.c, the ret variable
would be 0 from the earlier successful kvm_pgtable_get_leaf() call. By
jumping to err_with_unmap without updating ret, the function silently returns
0 to the caller even though the unmap operation failed.
> + }
> + WARN_ON(host_stage2_idmap_locked(addr, size, PKVM_HOST_MMIO_PROT));
> +unlock:
> + hyp_unlock_component();
> + host_unlock_component();
> + return ret;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=1
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 02/13] KVM: arm64: Track host-unmapped MMIO regions in a static array
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
2026-08-07 16:43 ` [PATCH v2 01/13] KVM: arm64: Donate MMIO to the hypervisor Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:00 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 03/13] KVM: arm64: Support host MMIO trap handlers for unmapped devices Sebastian Ene
` (10 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Introduce a registry to track protected MMIO regions that are unmapped
from the host stage-2 page tables. These regions are stored in a
fixed-size array and their ownership is donated to the hypervisor during
initialization to ensure host-exclusion and persistent tracking.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/include/asm/kvm_pkvm.h | 11 +++++++++++
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 3 +++
arch/arm64/kvm/hyp/nvhe/setup.c | 24 ++++++++++++++++++++++++
3 files changed, 38 insertions(+)
diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
index 74fedd9c5ff0..ab26bec079d6 100644
--- a/arch/arm64/include/asm/kvm_pkvm.h
+++ b/arch/arm64/include/asm/kvm_pkvm.h
@@ -17,6 +17,17 @@
#define HYP_MEMBLOCK_REGIONS 128
+/* The maximum number of hypervisor protected regions from the host */
+#define PKVM_PROTECTED_REGS_NUM 8
+
+struct pkvm_protected_reg {
+ u64 pfn;
+ u64 nr_pages;
+};
+
+extern struct pkvm_protected_reg kvm_nvhe_sym(pkvm_protected_regs)[];
+extern unsigned int kvm_nvhe_sym(num_protected_reg);
+
int pkvm_init_host_vm(struct kvm *kvm, unsigned long type);
int pkvm_create_hyp_vm(struct kvm *kvm);
bool pkvm_hyp_vm_is_created(struct kvm *kvm);
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 5cf7c4a0ed20..500c18c2fd48 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -28,6 +28,9 @@ struct host_mmu host_mmu;
static struct hyp_pool host_s2_pool;
+unsigned int num_protected_reg;
+struct pkvm_protected_reg pkvm_protected_regs[PKVM_PROTECTED_REGS_NUM];
+
static DEFINE_PER_CPU(struct pkvm_hyp_vm *, __current_vm);
#define current_vm (*this_cpu_ptr(&__current_vm))
diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index 75b00c323310..64c0290da888 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -284,6 +284,26 @@ static int fix_hyp_pgtable_refcnt(void)
&walker);
}
+static int donate_protected_mmio_regions(void)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < num_protected_reg; i++) {
+ ret = __pkvm_host_donate_hyp_mmio(hyp_pfn_to_phys(pkvm_protected_regs[i].pfn),
+ pkvm_protected_regs[i].nr_pages << PAGE_SHIFT);
+ if (ret)
+ goto err_setup;
+ }
+
+ return 0;
+err_setup:
+ while (--i >= 0)
+ __pkvm_hyp_donate_host_mmio(hyp_pfn_to_phys(pkvm_protected_regs[i].pfn),
+ pkvm_protected_regs[i].nr_pages << PAGE_SHIFT);
+ return ret;
+}
+
void __noreturn __pkvm_init_finalise(void)
{
struct kvm_cpu_context *host_ctxt = host_data_ptr(host_ctxt);
@@ -324,6 +344,10 @@ void __noreturn __pkvm_init_finalise(void)
if (ret)
goto out;
+ ret = donate_protected_mmio_regions();
+ if (ret)
+ goto out;
+
ret = hyp_ffa_init(ffa_proxy_pages);
if (ret)
goto out;
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 02/13] KVM: arm64: Track host-unmapped MMIO regions in a static array
2026-08-07 16:43 ` [PATCH v2 02/13] KVM: arm64: Track host-unmapped MMIO regions in a static array Sebastian Ene
@ 2026-08-07 17:00 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:00 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Marc Zyngier, Oliver Upton, kvmarm
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The error rollback path incorrectly maps previously unmapped protected MMIO regions into the host stage-2 page tables, breaking isolation.
- [High] Invalid use of the linear map macro `__hyp_va()` for physical MMIO addresses causes hypervisor page table bounds checks to fail or collide with private VA regions.
--
--- Patch [2]: [PATCH v2 02/13] KVM: arm64: Track host-unmapped MMIO regions in a static array ---
commit 8dc1d17638f0ac1801faef9ffe3be3d70a9ddf5c
Author: Sebastian Ene <sebastianene@google.com>
Subject: KVM: arm64: Track host-unmapped MMIO regions in a static array
This commit introduces a registry to track protected MMIO regions that are
unmapped from the host stage-2 page tables. It stores these regions in a
fixed-size array and donates their ownership to the hypervisor during
initialization to ensure host-exclusion and persistent tracking.
> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> index 75b00c3233102..64c0290da888f 100644
> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
[ ... ]
> @@ -284,6 +284,26 @@ static int fix_hyp_pgtable_refcnt(void)
> +static int donate_protected_mmio_regions(void)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < num_protected_reg; i++) {
> + ret = __pkvm_host_donate_hyp_mmio(hyp_pfn_to_phys(pkvm_protected_regs[i].pfn),
> + pkvm_protected_regs[i].nr_pages << PAGE_SHIFT);
[Severity: High]
This is a pre-existing issue, but does calling __pkvm_host_donate_hyp_mmio()
here work for physical MMIO addresses?
Looking at __pkvm_host_donate_hyp_mmio(), it uses __hyp_va() to compute the
virtual address:
__pkvm_host_donate_hyp_mmio() {
...
virt = __hyp_va(addr + offset);
...
}
Since __hyp_va() applies the linear map offset, which is sized and aligned
specifically for RAM, applying it to physical MMIO addresses (which reside
outside RAM boundaries) yields an invalid virtual address. This causes the
subsequent hypervisor page table bounds checks to fail with -ERANGE, or
silently corrupts EL2 private memory structures, reliably forcing the failure
path to execute.
> + if (ret)
> + goto err_setup;
> + }
> +
> + return 0;
> +err_setup:
> + while (--i >= 0)
> + __pkvm_hyp_donate_host_mmio(hyp_pfn_to_phys(pkvm_protected_regs[i].pfn),
> + pkvm_protected_regs[i].nr_pages << PAGE_SHIFT);
[Severity: High]
This isn't a bug introduced by this patch, but could this error rollback path
inadvertently map previously unmapped regions into the host stage-2?
The regions tracked in pkvm_protected_regs are intended to remain unmapped
from the host stage-2 page tables. However, __pkvm_hyp_donate_host_mmio()
unconditionally idmaps the region to the host at the end of the function:
__pkvm_hyp_donate_host_mmio() {
...
WARN_ON(host_stage2_idmap_locked(addr, size, PKVM_HOST_MMIO_PROT));
...
}
Since the failure path is triggered by the __hyp_va() issue mentioned above,
this rollback will execute and forcibly install a valid mapping in the host
stage-2 page tables for regions that were originally unmapped, breaking the
hypervisor-host isolation boundary.
> + return ret;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 03/13] KVM: arm64: Support host MMIO trap handlers for unmapped devices
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
2026-08-07 16:43 ` [PATCH v2 01/13] KVM: arm64: Donate MMIO to the hypervisor Sebastian Ene
2026-08-07 16:43 ` [PATCH v2 02/13] KVM: arm64: Track host-unmapped MMIO regions in a static array Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:03 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM Sebastian Ene
` (9 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Hook a handler to the host mem abort so that the hypervisor can
intercept host accesses to unmapped memory regions.
When a Stage-2 fault occurs on a registered device region, the
hypervisor will look if there is any registered function that
can handle the access. On the back of this, mediate host accesses
to devices and emulate them in pKVM.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
Signed-off-by: Bartłomiej Grzesik <bgrzesik@google.com>
---
arch/arm64/include/asm/kvm_arm.h | 2 ++
arch/arm64/include/asm/kvm_pkvm.h | 4 +++
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 49 +++++++++++++++++++++++++++
arch/arm64/kvm/hyp/nvhe/setup.c | 3 ++
4 files changed, 58 insertions(+)
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 3f9233b5a130..6360c90f9855 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -304,6 +304,8 @@
/* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
#define HPFAR_MASK (~UL(0xf))
+#define FAR_MASK GENMASK_ULL(11, 0)
+
/*
* We have
* PAR [PA_Shift - 1 : 12] = PA [PA_Shift - 1 : 12]
diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
index ab26bec079d6..0a471564be00 100644
--- a/arch/arm64/include/asm/kvm_pkvm.h
+++ b/arch/arm64/include/asm/kvm_pkvm.h
@@ -20,9 +20,13 @@
/* The maximum number of hypervisor protected regions from the host */
#define PKVM_PROTECTED_REGS_NUM 8
+struct pkvm_protected_reg;
+typedef void(pkvm_emulate_handler)(struct pkvm_protected_reg *region, u64 offset,
+ bool write, u64 *reg, u8 reg_size);
struct pkvm_protected_reg {
u64 pfn;
u64 nr_pages;
+ pkvm_emulate_handler *cb;
};
extern struct pkvm_protected_reg kvm_nvhe_sym(pkvm_protected_regs)[];
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 500c18c2fd48..7e978e0c44b9 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -14,6 +14,7 @@
#include <asm/stage2_pgtable.h>
#include <hyp/fault.h>
+#include <hyp/adjust_pc.h>
#include <nvhe/arm-smccc.h>
#include <nvhe/gfp.h>
@@ -752,6 +753,50 @@ static void host_inject_mem_abort(struct kvm_cpu_context *host_ctxt)
inject_host_exception(esr);
}
+static bool handle_host_mmio_trap(struct kvm_cpu_context *host_ctxt, u64 esr, u64 addr)
+{
+ u64 offset, reg_value = 0, start, end;
+ u8 reg_size, reg_index;
+ bool write;
+ int i;
+
+ for (i = 0; i < num_protected_reg; i++) {
+ if (!pkvm_protected_regs[i].pfn || !pkvm_protected_regs[i].nr_pages ||
+ !pkvm_protected_regs[i].cb)
+ continue;
+
+ start = PFN_PHYS(pkvm_protected_regs[i].pfn);
+ end = start + PFN_PHYS(pkvm_protected_regs[i].nr_pages);
+ reg_size = BIT((esr & ESR_ELx_SAS) >> ESR_ELx_SAS_SHIFT);
+
+ if (start > addr || addr + reg_size > end)
+ continue;
+
+ reg_index = (esr & ESR_ELx_SRT_MASK) >> ESR_ELx_SRT_SHIFT;
+ write = (esr & ESR_ELx_WNR) == ESR_ELx_WNR;
+ offset = addr - start;
+
+ if (write && reg_index != 31)
+ reg_value = host_ctxt->regs.regs[reg_index];
+
+ pkvm_protected_regs[i].cb(&pkvm_protected_regs[i], offset, write,
+ ®_value, reg_size);
+
+ if (!write && reg_index != 31)
+ host_ctxt->regs.regs[reg_index] = reg_value;
+
+ kvm_skip_host_instr();
+ return true;
+ }
+
+ return false;
+}
+
+static bool is_dabt(u64 esr)
+{
+ return (ESR_ELx_EC(esr) == ESR_ELx_EC_DABT_LOW) && (esr & ESR_ELx_ISV);
+}
+
void handle_host_mem_abort(struct kvm_cpu_context *host_ctxt)
{
struct kvm_vcpu_fault_info fault;
@@ -774,6 +819,10 @@ void handle_host_mem_abort(struct kvm_cpu_context *host_ctxt)
BUG_ON(!(fault.hpfar_el2 & HPFAR_EL2_NS));
addr = FIELD_GET(HPFAR_EL2_FIPA, fault.hpfar_el2) << 12;
+ if (is_dabt(esr) && !addr_is_memory(addr) &&
+ handle_host_mmio_trap(host_ctxt, esr, addr | (fault.far_el2 & FAR_MASK)))
+ return;
+
switch (host_stage2_idmap(addr)) {
case -EPERM:
host_inject_mem_abort(host_ctxt);
diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index 64c0290da888..4395595b7f7e 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -294,6 +294,9 @@ static int donate_protected_mmio_regions(void)
pkvm_protected_regs[i].nr_pages << PAGE_SHIFT);
if (ret)
goto err_setup;
+
+ if (pkvm_protected_regs[i].cb)
+ pkvm_protected_regs[i].cb = kern_hyp_va(pkvm_protected_regs[i].cb);
}
return 0;
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 03/13] KVM: arm64: Support host MMIO trap handlers for unmapped devices
2026-08-07 16:43 ` [PATCH v2 03/13] KVM: arm64: Support host MMIO trap handlers for unmapped devices Sebastian Ene
@ 2026-08-07 17:03 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:03 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Oliver Upton, Marc Zyngier, kvmarm
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Host MMIO trap handler fails to properly mask, sign-extend, and truncate register values during emulation.
- [Medium] Unconditional 4-byte instruction skip breaks AArch32 EL0 Thumb execution.
--
--- Patch [3]: [PATCH v2 03/13] KVM: arm64: Support host MMIO trap handlers for unmapped devices ---
commit 4f82f551a35a497988ae3208761d26c15c28de61
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Support host MMIO trap handlers for unmapped devices
This commit introduces a stage-2 fault handler in the hypervisor to intercept
and emulate host memory accesses to unmapped or protected MMIO regions. It
hooks into handle_host_mem_abort() to check if a registered callback can handle
the access.
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 500c18c2fd48b..7e978e0c44b9e 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
[ ... ]
> @@ -752,6 +753,50 @@ static void host_inject_mem_abort(struct kvm_cpu_context *host_ctxt)
> inject_host_exception(esr);
> }
>
> +static bool handle_host_mmio_trap(struct kvm_cpu_context *host_ctxt, u64 esr, u64 addr)
> +{
> + u64 offset, reg_value = 0, start, end;
> + u8 reg_size, reg_index;
> + bool write;
> + int i;
> +
> + for (i = 0; i < num_protected_reg; i++) {
> + if (!pkvm_protected_regs[i].pfn || !pkvm_protected_regs[i].nr_pages ||
> + !pkvm_protected_regs[i].cb)
> + continue;
> +
> + start = PFN_PHYS(pkvm_protected_regs[i].pfn);
> + end = start + PFN_PHYS(pkvm_protected_regs[i].nr_pages);
> + reg_size = BIT((esr & ESR_ELx_SAS) >> ESR_ELx_SAS_SHIFT);
> +
> + if (start > addr || addr + reg_size > end)
> + continue;
> +
> + reg_index = (esr & ESR_ELx_SRT_MASK) >> ESR_ELx_SRT_SHIFT;
> + write = (esr & ESR_ELx_WNR) == ESR_ELx_WNR;
> + offset = addr - start;
> +
> + if (write && reg_index != 31)
> + reg_value = host_ctxt->regs.regs[reg_index];
> +
> + pkvm_protected_regs[i].cb(&pkvm_protected_regs[i], offset, write,
> + ®_value, reg_size);
> +
> + if (!write && reg_index != 31)
> + host_ctxt->regs.regs[reg_index] = reg_value;
[Severity: High]
Does this emulation handle sign extension and register width properly?
When the host executes an instruction like LDRSB, shouldn't the handler inspect
the ESR_ELx_SSE (Sign Extend) bit to properly sign-extend the value? Also, for
32-bit register destinations (ESR_ELx_SF == 0), the architecture requires the
upper 32 bits to be zeroed, but it looks like this assigns the value directly.
> +
> + kvm_skip_host_instr();
[Severity: Medium]
Can this break AArch32 EL0 Thumb execution?
kvm_skip_host_instr() unconditionally adds 4 to the ELR. If the stage-2 data
abort originates from a host userspace process executing a 2-byte Thumb
instruction, shouldn't it inspect the ESR_ELx_IL bit to advance the PC by 2
bytes instead?
> + return true;
> + }
> +
> + return false;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=3
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (2 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 03/13] KVM: arm64: Support host MMIO trap handlers for unmapped devices Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:09 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup Sebastian Ene
` (8 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Identify the ITS base address from the device tree and store it in the
pkvm_protected_regs array so that it will be unmapped from the host
address space.
Register a callback to forward all the MMIO requests to the device to
prevent breaking ITS functionality in this patch. The patch by itself
shouldn't break any existing functionality even though all the accesses
from the gic-ITS driver are now mediated inside pKVM.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/include/asm/kvm_pkvm.h | 2 ++
arch/arm64/kvm/hyp/nvhe/Makefile | 3 +-
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 37 +++++++++++++++++++
arch/arm64/kvm/pkvm.c | 52 +++++++++++++++++++++++++++
4 files changed, 93 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kvm/hyp/nvhe/its_emulate.c
diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
index 0a471564be00..370225f0e72c 100644
--- a/arch/arm64/include/asm/kvm_pkvm.h
+++ b/arch/arm64/include/asm/kvm_pkvm.h
@@ -31,6 +31,8 @@ struct pkvm_protected_reg {
extern struct pkvm_protected_reg kvm_nvhe_sym(pkvm_protected_regs)[];
extern unsigned int kvm_nvhe_sym(num_protected_reg);
+extern void kvm_nvhe_sym(its_emulate_forward_req)(struct pkvm_protected_reg *region, u64 offset,
+ bool write, u64 *reg, u8 reg_size);
int pkvm_init_host_vm(struct kvm *kvm, unsigned long type);
int pkvm_create_hyp_vm(struct kvm *kvm);
diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
index f57450ebcb49..70fbca325852 100644
--- a/arch/arm64/kvm/hyp/nvhe/Makefile
+++ b/arch/arm64/kvm/hyp/nvhe/Makefile
@@ -24,7 +24,8 @@ CFLAGS_switch.nvhe.o += -Wno-override-init
hyp-obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o \
hyp-main.o hyp-smp.o psci-relay.o early_alloc.o page_alloc.o \
- cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o
+ cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o \
+ its_emulate.o
hyp-obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
../hyp-entry.o ../exception.o ../pgtable.o ../vgic-v5-sr.o
hyp-obj-y += ../../../kernel/smccc-call.o
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
new file mode 100644
index 000000000000..63a42f520ed2
--- /dev/null
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <asm/kvm_pkvm.h>
+#include <nvhe/mem_protect.h>
+
+void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool write, u64 *reg,
+ u8 reg_size)
+{
+ void __iomem *addr = __hyp_va(PFN_PHYS(region->pfn) + offset);
+
+ switch (reg_size) {
+ case 1:
+ if (!write)
+ *reg = readb_relaxed(addr);
+ else
+ writeb_relaxed(*reg, addr);
+ break;
+ case 2:
+ if (!write)
+ *reg = readw_relaxed(addr);
+ else
+ writew_relaxed(*reg, addr);
+ break;
+ case 4:
+ if (!write)
+ *reg = readl_relaxed(addr);
+ else
+ writel_relaxed(*reg, addr);
+ break;
+ case 8:
+ if (!write)
+ *reg = readq_relaxed(addr);
+ else
+ writeq_relaxed(*reg, addr);
+ break;
+ }
+}
diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
index 428723b1b0f5..4bfffbedac4c 100644
--- a/arch/arm64/kvm/pkvm.c
+++ b/arch/arm64/kvm/pkvm.c
@@ -9,8 +9,13 @@
#include <linux/kmemleak.h>
#include <linux/kvm_host.h>
#include <asm/kvm_mmu.h>
+#include <asm/kvm_pkvm.h>
#include <linux/memblock.h>
#include <linux/mutex.h>
+#include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/irqchip/arm-gic-v3.h>
#include <asm/kvm_pkvm.h>
@@ -39,6 +44,47 @@ static int __init register_memblock_regions(void)
return 0;
}
+static int __init register_its_emulated_region(void)
+{
+ struct device_node *np;
+ struct resource res;
+ int i = 0;
+ int ret;
+
+ for_each_compatible_node(np, NULL, "arm,gic-v3-its") {
+ ret = of_address_to_resource(np, 0, &res);
+ if (ret)
+ goto out_fail;
+
+ if (i >= PKVM_PROTECTED_REGS_NUM) {
+ kvm_err("Out of protected region slots\n");
+ ret = -ENOSPC;
+ goto out_fail;
+ }
+
+ /*
+ * Note: don't unmap the entire animal from the host because devices need
+ * to be able to access GITS_TRANSLATER to raise MSIs. If the
+ * page where GITS_TRANSLATER is given to HYP, devices won't be
+ * able to map it in their IOMMU when the IOMMU is managed by
+ * pKVM.
+ */
+ kvm_nvhe_sym(pkvm_protected_regs)[i].pfn = PHYS_PFN(res.start);
+ kvm_nvhe_sym(pkvm_protected_regs)[i].cb =
+ lm_alias(&kvm_nvhe_sym(its_emulate_forward_req));
+ kvm_nvhe_sym(pkvm_protected_regs)[i].nr_pages =
+ PFN_DOWN(min_t(u64, resource_size(&res), PAGE_ALIGN_DOWN(GITS_TRANSLATER)));
+
+ i++;
+ }
+
+ kvm_nvhe_sym(num_protected_reg) = i;
+ return 0;
+out_fail:
+ of_node_put(np);
+ return ret;
+}
+
void __init kvm_hyp_reserve(void)
{
u64 hyp_mem_pages = 0;
@@ -57,6 +103,12 @@ void __init kvm_hyp_reserve(void)
return;
}
+ ret = register_its_emulated_region();
+ if (ret) {
+ kvm_err("Failed to register ITS region %d\n", ret);
+ return;
+ }
+
hyp_mem_pages += hyp_s1_pgtable_pages();
hyp_mem_pages += host_s2_pgtable_pages();
hyp_mem_pages += hyp_vm_table_pages();
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM
2026-08-07 16:43 ` [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM Sebastian Ene
@ 2026-08-07 17:09 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:09 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Marc Zyngier, Oliver Upton, kvmarm
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The function pointer for the MMIO emulator passed to EL2 points to unmapped/NX memory, causing an Instruction Abort.
- [High] MMIO emulation in `its_emulate_forward_req` double-swaps on Big Endian kernels by using `readX_relaxed` and `writeX_relaxed`.
- [High] pKVM incorrectly maps and accesses MMIO devices using the hypervisor's linear map (`__hyp_va()`) rather than a private VA range.
- [High] handle_host_mmio_trap ignores the ESR_ELx_SSE (Sign Extend) bit for MMIO reads, returning zero-extended values for signed loads.
--
--- Patch [4]: [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM ---
commit 6f98f20a817e227941fab67881459ac609810480
Author: Sebastian Ene <sebastianene@google.com>
KVM: Parse the device tree and register the ITS region with pKVM
Identify the ITS base address from the device tree and store it in the
pkvm_protected_regs array so that it will be unmapped from the host
address space. Register a callback to forward all the MMIO requests to
the device to prevent breaking ITS functionality.
> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> new file mode 100644
> index 0000000000000..63a42f520ed25
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <asm/kvm_pkvm.h>
> +#include <nvhe/mem_protect.h>
> +
> +void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool write, u64 *reg,
> + u8 reg_size)
> +{
> + void __iomem *addr = __hyp_va(PFN_PHYS(region->pfn) + offset);
[Severity: High]
Does using __hyp_va() here work for MMIO addresses? The __hyp_va() macro
subtracts hyp_physvirt_offset, which is based on RAM physical addresses.
Since MMIO regions often reside at lower physical addresses than RAM,
wouldn't this underflow and produce a virtual address with the top bits set,
leading to a Translation Fault? This same issue seems to be present in
__pkvm_host_donate_hyp_mmio() where it populates the page tables.
> +
> + switch (reg_size) {
> + case 1:
> + if (!write)
> + *reg = readb_relaxed(addr);
> + else
> + writeb_relaxed(*reg, addr);
[Severity: High]
Will these relaxed accessors cause a double-swap on Big Endian kernels?
When the host traps to EL2 on an MMIO access, the CPU register already
contains the exact data the host intended to write (swapped to Little
Endian by the host driver's writel). Using writeb_relaxed and similar
helpers will apply cpu_to_le* again. Should this use the __raw_read*
and __raw_write* accessors to preserve the trapped register's exact bit
representation?
> + break;
[ ... ]
> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
> index 053e4f733e4be..5e80da18f3120 100644
> --- a/arch/arm64/kvm/pkvm.c
> +++ b/arch/arm64/kvm/pkvm.c
[ ... ]
> @@ -39,6 +44,47 @@ static int __init register_memblock_regions(void)
>
> +static int __init register_its_emulated_region(void)
> +{
[ ... ]
> + kvm_nvhe_sym(pkvm_protected_regs)[i].pfn = PHYS_PFN(res.start);
> + kvm_nvhe_sym(pkvm_protected_regs)[i].cb =
> + lm_alias(&kvm_nvhe_sym(its_emulate_forward_req));
[Severity: Critical]
Can this function pointer be safely executed at EL2? The host initializes
the callback using lm_alias, producing a host linear-map address. When
the hypervisor later applies kern_hyp_va() to it in
donate_protected_mmio_regions(), it converts it to a hyp linear-map
address. However, .hyp.text physical pages are not part of the
dynamically allocated hyp memory pool, so they are unmapped in the hyp
linear map. Does jumping to this pointer in handle_host_mmio_trap()
trigger an Instruction Abort?
[Severity: High]
Regarding how this callback's result is handled in handle_host_mmio_trap(),
does the trap handler process sign extension for MMIO reads? When the host
traps to EL2 on a stage-2 data abort caused by a signed read access (like
LDRSB), the ESR_ELx_SSE bit indicates the destination register should be
sign-extended. If handle_host_mmio_trap() writes the zero-extended value
directly into the host context, could this alter functional behavior for
signed loads?
> + kvm_nvhe_sym(pkvm_protected_regs)[i].nr_pages =
> + PFN_DOWN(min_t(u64, resource_size(&res), PAGE_ALIGN_DOWN(GITS_TRANSLATER)));
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=4
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (3 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:00 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation Sebastian Ene
` (7 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Introduce two new helper functions to allow locking the ITS and setting
up a copy of the host ITS state that will be given to the pKVM
emulation. The caller of these functions is responsible to implement a
callback which will be used to setup the emulation layer. The calling
flow is expected to do the following:
pkvm_its_emulate_setup(its_phys, host)
// allocate memory for the priv state of the ITS emulation
// call the its emulation setup(its_phys, host, priv_state);
pkvm_drop_host_privileges()
its_emulate_acquire_locks(&flags);
on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
its_emulate_release_locks(ret, &flags, pkvm_its_emulate_setup);
Augment the its_baser structure with a new fiels that will hold a
pointer to the base table copy. The gic ITS driver will use the pointer
to the base table copy when emulation is enabled, as this allows us to
hide away the original first level of an indirect table to prevent the
following:
// assumming an indirect Device Table layout
1. malicious host patches an entry in the 1st level table with an
address that it wants to write to.
2. malicious host issues MAPD to install a DTE in the table pointed by
the address from (1).
As the driver only manipulates a copy of the table, the emulation is
responsible for looking at the updates from the copy table, sanitizing
them and updating the original table before talking to the hardware.
In a simillar fashion, when emulation is in place we no longer let the
gic ITS driver use the original command queue but we present the driver
a copy of it and we hide away the original command queue from the driver
as this will be used entirely by the emulation layer.
Co-authored-by: Bartłomiej Grzesik <bgrzesik@google.com>
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
drivers/irqchip/irq-gic-v3-its.c | 157 +++++++++++++++++++++++++++--
include/linux/irqchip/arm-gic-v3.h | 39 +++++++
2 files changed, 185 insertions(+), 11 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 6f5811aae59c..e74ae9220af5 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -78,17 +78,6 @@ struct its_collection {
u16 col_id;
};
-/*
- * The ITS_BASER structure - contains memory information, cached
- * value of BASER register configuration and ITS page size.
- */
-struct its_baser {
- void *base;
- u64 val;
- u32 order;
- u32 psz;
-};
-
struct its_device;
/*
@@ -5226,6 +5215,152 @@ static int __init its_compute_its_list_map(struct its_node *its)
return its_number;
}
+static void its_free_snapshot(struct its_host_state *snapshot)
+{
+ int i;
+
+ if (snapshot->cmd_host_copy)
+ its_free_pages(snapshot->cmd_host_copy, get_order(ITS_CMD_QUEUE_SZ));
+
+ for (i = 0; i < GITS_BASER_NR_REGS; i++) {
+ if (!snapshot->tables[i].base_snapshot)
+ continue;
+
+ its_free_pages(snapshot->tables[i].base_snapshot, snapshot->tables[i].order);
+ }
+
+ its_free_pages(snapshot, 0);
+}
+
+static struct its_host_state *its_snapshot_host_state(struct its_node *its)
+{
+ void *page;
+ struct its_host_state *snapshot;
+ int i;
+
+ page = its_alloc_pages_node(its->numa_node, GFP_ATOMIC | __GFP_ZERO, 0);
+ if (!page)
+ return NULL;
+
+ snapshot = (void *)page_address(page);
+ page = its_alloc_pages_node(its->numa_node, GFP_ATOMIC | __GFP_ZERO,
+ get_order(ITS_CMD_QUEUE_SZ));
+ if (!page)
+ goto err_alloc;
+
+ snapshot->cmd_host_copy = page_address(page);
+ snapshot->cmdq_len = ITS_CMD_QUEUE_SZ;
+ snapshot->cmd_original = its->cmd_base;
+ snapshot->cmd_write = its->cmd_write;
+
+ memcpy(snapshot->tables, its->tables, sizeof(struct its_baser) * GITS_BASER_NR_REGS);
+
+ for (i = 0; i < GITS_BASER_NR_REGS; i++) {
+ if (!(snapshot->tables[i].val & GITS_BASER_VALID))
+ continue;
+
+ if (!(snapshot->tables[i].val & GITS_BASER_INDIRECT))
+ continue;
+
+ page = its_alloc_pages_node(its->numa_node,
+ GFP_ATOMIC | __GFP_ZERO,
+ snapshot->tables[i].order);
+ if (!page)
+ goto err_alloc;
+
+ snapshot->tables[i].base_snapshot = page_address(page);
+
+ memcpy(snapshot->tables[i].base_snapshot, snapshot->tables[i].base,
+ PAGE_ORDER_TO_SIZE(snapshot->tables[i].order));
+ }
+
+ return snapshot;
+
+err_alloc:
+ its_free_snapshot(snapshot);
+ return NULL;
+}
+
+static int its_emulate_switch_queues_locked(struct its_node *its, its_emulate_setup cb)
+{
+ struct its_host_state *host_snaphsot, host;
+ int i, ret;
+ u64 baser_phys;
+
+ host_snaphsot = its_snapshot_host_state(its);
+ if (!host_snaphsot)
+ return -ENOMEM;
+
+ /*
+ * The snapshot of the ITS state will be given to the emulation, make a copy of it
+ * so that we don't go in weeds.
+ */
+ memcpy(&host, host_snaphsot, sizeof(host));
+
+ ret = cb(its->phys_base, host_snaphsot);
+ if (ret) {
+ its_free_snapshot(host_snaphsot);
+ return ret;
+ }
+
+ /* Switch the driver command queue to use the host copy and update the write index */
+ its->cmd_write = (its->cmd_write - its->cmd_base) +
+ (struct its_cmd_block *)host.cmd_host_copy;
+ its->cmd_base = host.cmd_host_copy;
+
+ /*
+ * Replace the first level of the indirect tables with the snapshot table as the
+ * emulation layer will make it innaccessible to the host.
+ */
+ for (i = 0; i < GITS_BASER_NR_REGS; i++) {
+ if (!(host.tables[i].val & GITS_BASER_INDIRECT))
+ continue;
+
+ baser_phys = virt_to_phys(host.tables[i].base_snapshot);
+ if (IS_ENABLED(CONFIG_ARM64_64K_PAGES) && (baser_phys >> 48))
+ baser_phys = GITS_BASER_PHYS_52_to_48(baser_phys);
+
+ its->tables[i].val &= ~GENMASK(47, 12);
+ its->tables[i].val |= baser_phys;
+ its->tables[i].base = host.tables[i].base_snapshot;
+ }
+
+ return 0;
+}
+
+void its_emulate_acquire_locks(unsigned long *flags)
+{
+ struct its_node *its;
+
+ if (WARN_ON(!flags))
+ return;
+
+ raw_spin_lock_irqsave(&its_lock, *flags);
+
+ list_for_each_entry(its, &its_nodes, entry)
+ raw_spin_lock(&its->lock);
+}
+
+int its_emulate_release_locks(int ret_pkvm_finalize, unsigned long *flags, its_emulate_setup cb)
+{
+ struct its_node *its;
+ int ret = 0;
+
+ if (WARN_ON(!flags || !cb))
+ ret = -EINVAL;
+
+ list_for_each_entry(its, &its_nodes, entry) {
+ if (!ret_pkvm_finalize && !ret)
+ ret = its_emulate_switch_queues_locked(its, cb);
+
+ raw_spin_unlock(&its->lock);
+ }
+
+ raw_spin_unlock_irqrestore(&its_lock, *flags);
+
+ return ret;
+}
+
static int __init its_probe_one(struct its_node *its)
{
u64 baser, tmp;
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index ea5fd2374ebe..b75f82cef4bf 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -657,6 +657,45 @@ static inline bool gic_enable_sre(void)
return !!(val & ICC_SRE_EL1_SRE);
}
+/*
+ * The ITS_BASER structure - contains memory information, cached
+ * value of BASER register configuration and ITS page size.
+ */
+struct its_baser {
+ void *base;
+
+ /*
+ * The table used when emulation is in place and indirect layout is
+ * configured.
+ */
+ void *base_snapshot;
+ u64 val;
+ u32 order;
+ u32 psz;
+};
+
+struct its_host_state {
+ struct its_baser tables[GITS_BASER_NR_REGS];
+
+ /* The command queue used after the emulation is in place */
+ void *cmd_host_copy;
+
+ /* The command queue configured by the ITS driver at boot */
+ void *cmd_original;
+ void *cmd_write;
+ size_t cmdq_len;
+};
+
+/*
+ * Callback used to initialize the emulation. It is expected to allocate memory for the private
+ * state of the emulation and receive as arguments copy of the host ITS driver state along
+ * with the address of the ITS.
+ */
+typedef int (*its_emulate_setup)(phys_addr_t its_phys_base, struct its_host_state *host);
+
+void its_emulate_acquire_locks(unsigned long *flags);
+int its_emulate_release_locks(int ret_pkvm_finalize, unsigned long *flags, its_emulate_setup cb);
+
#endif
#endif
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup
2026-08-07 16:43 ` [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup Sebastian Ene
@ 2026-08-07 17:00 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:00 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Marc Zyngier, kvmarm, Oliver Upton
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The intended API design dictates calling `on_each_cpu()` while hardware interrupts are disabled, which triggers SMP deadlocks.
- [High] Page allocations are performed while holding raw spinlocks, violating PREEMPT_RT lock nesting rules and causing 'sleeping while atomic' panics.
- [High] Partial failure during ITS emulation setup leaves the system in a broken, partially-emulated state with no rollback.
- [Medium] Defensive check `WARN_ON(!flags)` in `its_emulate_release_locks` leads to a NULL pointer dereference and unbalanced unlocks on the error path.
- [Medium] Acquiring an unbounded number of identical locks triggers lockdep's same-lock-class tracking.
--
--- Patch [5]: [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup ---
commit 96beb9b6951a65e060abceadeb7be17ef57b5218
Author: Sebastian Ene <sebastianene@google.com>
irqchip/gic-v3-its: Add support for the ITS emulation setup
This commit introduces helper functions to lock the ITS and duplicate its state
for pKVM emulation. It allows an emulation layer to hide original hardware
tables and command queues from the host.
> pkvm_drop_host_privileges()
> its_emulate_acquire_locks(&flags);
> on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
> its_emulate_release_locks(ret, &flags, pkvm_its_emulate_setup);
[Severity: High]
Will this sequence trigger a deadlock? its_emulate_acquire_locks() uses
raw_spin_lock_irqsave(), disabling local interrupts. Calling on_each_cpu()
sends cross-CPU IPIs and waits for completion, which is strictly
prohibited while interrupts are disabled because the local CPU cannot
process incoming IPIs.
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
[ ... ]
> +static struct its_host_state *its_snapshot_host_state(struct its_node *its)
> +{
> + void *page;
> + struct its_host_state *snapshot;
> + int i;
> +
> + page = its_alloc_pages_node(its->numa_node, GFP_ATOMIC | __GFP_ZERO, 0);
[Severity: High]
Is it safe to call the page allocator here? This function is called from
its_emulate_switch_queues_locked() inside the its_emulate_release_locks()
loop, where both the global its_lock and the local its->lock raw spinlocks
are held.
Even with GFP_ATOMIC, the page allocator acquires zone->lock, which is a
sleeping rt_mutex on PREEMPT_RT kernels. Acquiring a sleepable lock while
holding a raw spinlock causes a sleeping while atomic panic.
[ ... ]
> +void its_emulate_acquire_locks(unsigned long *flags)
> +{
> + struct its_node *its;
> +
> + if (WARN_ON(!flags))
> + return;
> +
> + raw_spin_lock_irqsave(&its_lock, *flags);
> +
> + list_for_each_entry(its, &its_nodes, entry)
> + raw_spin_lock(&its->lock);
> +}
[Severity: Medium]
Does holding all these identical locks concurrently trigger lockdep? Since
all its->lock instances belong to the same lock class, acquiring an
unbounded number of them sequentially without unlocking or using nested
annotations will trigger lockdep's same-lock-class tracking and disable
further lockdep checks.
> +int its_emulate_release_locks(int ret_pkvm_finalize, unsigned long *flags, its_emulate_setup cb)
> +{
> + struct its_node *its;
> + int ret = 0;
> +
> + if (WARN_ON(!flags || !cb))
> + ret = -EINVAL;
> +
> + list_for_each_entry(its, &its_nodes, entry) {
> + if (!ret_pkvm_finalize && !ret)
> + ret = its_emulate_switch_queues_locked(its, cb);
> +
> + raw_spin_unlock(&its->lock);
> + }
> +
> + raw_spin_unlock_irqrestore(&its_lock, *flags);
> +
> + return ret;
> +}
[Severity: Medium]
If flags is NULL, the WARN_ON triggers and sets ret = -EINVAL, but execution
continues. Doesn't this lead to a NULL pointer dereference in
raw_spin_unlock_irqrestore() at the end of the function, as well as
unbalanced unlocks for all its->lock instances (since the acquire
function returns early)?
[Severity: High]
If its_emulate_switch_queues_locked() fails for a node, ret is populated
and subsequent nodes are skipped. However, what happens to the nodes that
were already successfully switched?
Without rolling back the hardware state for those previous nodes, the
system is left partially emulated, which would cause a loss of
synchronization between the driver and hardware and lead to IOMMU or
interrupt faults.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=5
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (4 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 16:57 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 07/13] KVM: arm64: Restrict host access to the private ITS tables Sebastian Ene
` (6 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Expose two functions that will be used to setup the entry point into the
pKVM ITS emulation. One will be called from an hvc to setup the ITS
structures and the other one will be called from a data abort to handle
the emulation. The later one will be stored in a pkvm_protected_reg as
part of the register_its_emulated_region once the command emulation is in
place.
Donate two memory regions as part of the emulation setup phase. One
holds GIC ITS driver state information and the other is used to store
private state information for the emulation and it is zeroed out.
Shadow the command queue by sharing a copy of it from the GIC ITS driver
and donate the original queue to the hypervisor. The host will use a
copy, while the emulation will use the original queue programmed in
hardware. This makes sure that the original queue is not accessible to
the host.
When the GIC ITS driver writes a command, the emulation will trap the
access to the CWRITER register and it will validate the
command before copying it to the original queue.
Re-use some of the definitions for command format and move them
from the GIC ITS driver to the public header.
Co-authored-by: Bartłomiej Grzesik <bgrzesik@google.com>
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/include/asm/kvm_pkvm.h | 1 +
arch/arm64/kvm/hyp/include/nvhe/its_emulate.h | 14 +
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 285 ++++++++++++++++++
drivers/irqchip/irq-gic-v3-its.c | 12 -
include/linux/irqchip/arm-gic-v3.h | 12 +
5 files changed, 312 insertions(+), 12 deletions(-)
create mode 100644 arch/arm64/kvm/hyp/include/nvhe/its_emulate.h
diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
index 370225f0e72c..78597210a53c 100644
--- a/arch/arm64/include/asm/kvm_pkvm.h
+++ b/arch/arm64/include/asm/kvm_pkvm.h
@@ -27,6 +27,7 @@ struct pkvm_protected_reg {
u64 pfn;
u64 nr_pages;
pkvm_emulate_handler *cb;
+ void *priv;
};
extern struct pkvm_protected_reg kvm_nvhe_sym(pkvm_protected_regs)[];
diff --git a/arch/arm64/kvm/hyp/include/nvhe/its_emulate.h b/arch/arm64/kvm/hyp/include/nvhe/its_emulate.h
new file mode 100644
index 000000000000..29429feb30a9
--- /dev/null
+++ b/arch/arm64/kvm/hyp/include/nvhe/its_emulate.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __NVHE_ITS_EMULATE_H
+#define __NVHE_ITS_EMULATE_H
+
+#include <asm/kvm_pkvm.h>
+
+struct its_host_state;
+
+int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_state, void *priv,
+ size_t priv_num_pages);
+void pkvm_its_emulate_handler(struct pkvm_protected_reg *region, u64 offset, bool write, u64 *reg,
+ u8 reg_size);
+#endif /* __NVHE_ITS_EMULATE_H */
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
index 63a42f520ed2..e943ab972aa5 100644
--- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -2,6 +2,9 @@
#include <asm/kvm_pkvm.h>
#include <nvhe/mem_protect.h>
+#include <nvhe/its_emulate.h>
+
+#include <linux/irqchip/arm-gic-v3.h>
void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool write, u64 *reg,
u8 reg_size)
@@ -35,3 +38,285 @@ void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool
break;
}
}
+
+struct its_handler {
+ u64 offset;
+ u8 access_size;
+ void (*write)(struct pkvm_protected_reg *region, u64 offset, u64 value);
+ void (*read)(struct pkvm_protected_reg *region, u64 offset, u64 *read);
+};
+
+#define ITS_HANDLER(off, sz, write_cb, read_cb) \
+{ \
+ .offset = (off), \
+ .access_size = (sz), \
+ .write = (write_cb), \
+ .read = (read_cb), \
+}
+
+struct its_priv_state {
+ /* The location of the ITS in the hypervisor VA */
+ void __iomem *base;
+
+ /* ITS command queue use by the hardware */
+ void *cmd_original;
+ void *cmd_host_copy;
+ u64 cmd_offset;
+ bool needs_flush;
+ hyp_spinlock_t its_lock;
+
+ struct its_host_state *host_state;
+};
+
+#define GITS_CWRITER_RETRY BIT_ULL(0)
+#define GITS_CWRITER_OFFSET GENMASK_ULL(19, 5)
+
+#define GITS_CREADR_STALLED BIT_ULL(0)
+#define GITS_CREADR_OFFSET GENMASK_ULL(19, 5)
+
+static int submit_single_cmd(struct its_priv_state *its, bool retry)
+{
+ size_t cmdq_sz = its->host_state->cmdq_len;
+ u64 timeout = 1000;
+ u64 offset, cwriter, creadr;
+
+ offset = (its->cmd_offset + sizeof(struct its_cmd_block)) % cmdq_sz;
+
+ cwriter = offset & GITS_CWRITER_OFFSET;
+ cwriter |= FIELD_PREP(GITS_CWRITER_RETRY, retry);
+ writeq_relaxed(cwriter, its->base + GITS_CWRITER);
+
+ while (its->cmd_offset != offset) {
+ creadr = readq_relaxed(its->base + GITS_CREADR);
+
+ /* Command failed. */
+ if (FIELD_GET(GITS_CREADR_STALLED, creadr))
+ return -EIO;
+
+ its->cmd_offset = creadr & GITS_CREADR_OFFSET;
+ if (its->cmd_offset == offset)
+ return 0;
+
+ /*
+ * We can't spin here forever and we can't roll back
+ * the cmd queue pointer. Let's revert the cmd effects in the
+ * emulation layer and then go back to the driver to let it
+ * decide what to do next.
+ */
+ if (!timeout--)
+ return -EBUSY;
+ }
+
+ return 0;
+}
+
+static int process_cmd(struct its_priv_state *its, struct its_cmd_block *cmd,
+ bool rollback)
+{
+ /* Passthrough everything for now */
+ return 0;
+}
+
+static void cwriter_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
+{
+ struct its_priv_state *its = region->priv;
+ struct its_cmd_block cmd, raw;
+ u64 new_offset;
+ bool retry;
+ int i;
+
+ new_offset = value & GITS_CWRITER_OFFSET;
+ if (new_offset >= its->host_state->cmdq_len)
+ return;
+
+ retry = FIELD_GET(GITS_CWRITER_RETRY, value);
+ while (its->cmd_offset != new_offset) {
+ memcpy(&raw, its->cmd_host_copy + its->cmd_offset, sizeof(raw));
+
+ for (i = 0; i < ARRAY_SIZE(cmd.raw_cmd); i++)
+ cmd.raw_cmd[i] = le64_to_cpu(raw.raw_cmd_le[i]);
+
+ if (process_cmd(its, &cmd, /* rollback */ false))
+ return;
+
+ memcpy(its->cmd_original + its->cmd_offset, &raw, sizeof(struct its_cmd_block));
+
+ if (its->needs_flush)
+ gic_flush_dcache_to_poc(its->cmd_original + its->cmd_offset, sizeof(cmd));
+ else
+ dsb(ishst);
+
+ if (submit_single_cmd(its, retry)) {
+ WARN_ON(process_cmd(its, &cmd, /* rollback */ true));
+ return;
+ }
+ }
+}
+
+static void cwriter_read(struct pkvm_protected_reg *region, u64 offset, u64 *read)
+{
+ struct its_priv_state *its = region->priv;
+ *read = readq_relaxed(its->base + GITS_CWRITER);
+}
+
+static struct its_handler its_handlers[] = {
+ ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
+ {},
+};
+
+void pkvm_its_emulate_handler(struct pkvm_protected_reg *region, u64 offset, bool write, u64 *reg,
+ u8 reg_size)
+{
+ struct its_priv_state *priv = region->priv;
+ struct its_handler *reg_handler;
+
+ if (!priv || !IS_ALIGNED(offset, reg_size))
+ return;
+
+ for (reg_handler = its_handlers; reg_handler->access_size; reg_handler++) {
+ if (reg_handler->offset > offset ||
+ reg_handler->offset + reg_handler->access_size <= offset)
+ continue;
+
+ if (reg_handler->access_size < reg_size)
+ return;
+
+ if (write && reg_handler->write) {
+ hyp_spin_lock(&priv->its_lock);
+ reg_handler->write(region, offset, *reg);
+ hyp_spin_unlock(&priv->its_lock);
+ return;
+ }
+
+ if (!write && reg_handler->read) {
+ hyp_spin_lock(&priv->its_lock);
+ reg_handler->read(region, offset, reg);
+ hyp_spin_unlock(&priv->its_lock);
+ return;
+ }
+
+ return;
+ }
+
+ its_emulate_forward_req(region, offset, write, reg, reg_size);
+}
+
+static int pkvm_setup_its_shadow_cmdq(struct its_host_state *host_state)
+{
+ u64 start_pfn, num_pages, i;
+ int ret;
+
+ start_pfn = hyp_virt_to_pfn(host_state->cmd_host_copy);
+ num_pages = host_state->cmdq_len >> PAGE_SHIFT;
+
+ for (i = 0; i < num_pages; i++) {
+ ret = __pkvm_host_share_hyp(start_pfn + i);
+ if (ret)
+ goto unshare_cmd_host;
+ }
+
+ ret = hyp_pin_shared_mem(host_state->cmd_host_copy,
+ host_state->cmd_host_copy + host_state->cmdq_len);
+ if (ret)
+ goto unshare_cmd_host;
+
+ ret = __pkvm_host_donate_hyp(hyp_virt_to_pfn(host_state->cmd_original), num_pages);
+ if (ret) {
+ hyp_unpin_shared_mem(host_state->cmd_host_copy,
+ host_state->cmd_host_copy + host_state->cmdq_len);
+ goto unshare_cmd_host;
+ }
+
+ return ret;
+unshare_cmd_host:
+ if (i == 0)
+ return ret;
+
+ for (i = i - 1; i >= 0; i--)
+ __pkvm_host_unshare_hyp(start_pfn + i);
+ return ret;
+}
+
+static struct pkvm_protected_reg *get_region(phys_addr_t dev_addr)
+{
+ int i;
+
+ for (i = 0; i < num_protected_reg; i++) {
+ if (PFN_PHYS(pkvm_protected_regs[i].pfn) == dev_addr)
+ return &pkvm_protected_regs[i];
+ }
+
+ return NULL;
+}
+
+DEFINE_HYP_SPINLOCK(its_setup_lock);
+
+int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_state, void *priv,
+ size_t priv_num_pages)
+{
+ struct pkvm_protected_reg *its_reg;
+ struct its_priv_state *priv_state;
+ int ret;
+
+ if (!PAGE_ALIGNED(host_state) || !PAGE_ALIGNED(priv) || !priv_num_pages)
+ return -EINVAL;
+
+ host_state = kern_hyp_va(host_state);
+ priv = kern_hyp_va(priv);
+
+ hyp_spin_lock(&its_setup_lock);
+ its_reg = get_region(dev_addr);
+ if (!its_reg) {
+ ret = -ENODEV;
+ goto err_unlock;
+ }
+
+ if (its_reg->priv) {
+ ret = -EOPNOTSUPP;
+ goto err_unlock;
+ }
+
+ ret = __pkvm_host_donate_hyp(hyp_virt_to_pfn(priv), priv_num_pages);
+ if (ret)
+ goto err_unlock;
+
+ priv_state = priv;
+ memset(priv_state, 0, priv_num_pages << PAGE_SHIFT);
+
+ ret = __pkvm_host_donate_hyp(hyp_virt_to_pfn(host_state), 1);
+ if (ret)
+ goto err_with_priv;
+
+ host_state->cmd_original = kern_hyp_va(host_state->cmd_original);
+ host_state->cmd_host_copy = kern_hyp_va(host_state->cmd_host_copy);
+
+ ret = pkvm_setup_its_shadow_cmdq(host_state);
+ if (ret)
+ goto err_with_host_state;
+
+ hyp_spin_lock_init(&priv_state->its_lock);
+
+ priv_state->host_state = host_state;
+ priv_state->base = (void __iomem *)__hyp_va(dev_addr);
+ priv_state->cmd_original = host_state->cmd_original;
+ priv_state->cmd_host_copy = host_state->cmd_host_copy;
+
+ priv_state->cmd_offset = readq_relaxed(priv_state->base + GITS_CREADR) &
+ GITS_CREADR_OFFSET;
+ priv_state->needs_flush =
+ (readq_relaxed(priv_state->base + GITS_CBASER) & GITS_CBASER_SHAREABILITY_MASK) !=
+ GITS_CBASER_InnerShareable;
+
+ its_reg->priv = priv_state;
+
+ hyp_spin_unlock(&its_setup_lock);
+
+ return 0;
+err_with_host_state:
+ WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(host_state), 1));
+err_with_priv:
+ WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(priv_state), 1));
+err_unlock:
+ hyp_spin_unlock(&its_setup_lock);
+ return ret;
+}
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e74ae9220af5..4736e49e3f2d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -121,8 +121,6 @@ static DEFINE_PER_CPU(struct its_node *, local_4_1_its);
#define is_v4_1(its) (!!((its)->typer & GITS_TYPER_VMAPP))
#define device_ids(its) (FIELD_GET(GITS_TYPER_DEVBITS, (its)->typer) + 1)
-#define ITS_ITT_ALIGN SZ_256
-
/* The maximum number of VPEID bits supported by VLPI commands */
#define ITS_MAX_VPEID_BITS \
({ \
@@ -515,16 +513,6 @@ struct its_cmd_desc {
};
};
-/*
- * The ITS command block, which is what the ITS actually parses.
- */
-struct its_cmd_block {
- union {
- u64 raw_cmd[4];
- __le64 raw_cmd_le[4];
- };
-};
-
#define ITS_CMD_QUEUE_SZ SZ_64K
#define ITS_CMD_QUEUE_NR_ENTRIES (ITS_CMD_QUEUE_SZ / sizeof(struct its_cmd_block))
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index b75f82cef4bf..7f72632115b8 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -524,6 +524,8 @@
#define GITS_CMD_VSGI GITS_CMD_GICv4(3)
#define GITS_CMD_INVDB GITS_CMD_GICv4(0xe)
+#define ITS_ITT_ALIGN SZ_256
+
/*
* ITS error numbers
*/
@@ -686,6 +688,16 @@ struct its_host_state {
size_t cmdq_len;
};
+/*
+ * The ITS command block, which is what the ITS actually parses.
+ */
+struct its_cmd_block {
+ union {
+ u64 raw_cmd[4];
+ __le64 raw_cmd_le[4];
+ };
+};
+
/*
* Callback used to initialize the emulation. It is expected to allocate memory for the private
* state of the emulation and receive as arguments copy of the host ITS driver state along
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation
2026-08-07 16:43 ` [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation Sebastian Ene
@ 2026-08-07 16:57 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 16:57 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Oliver Upton, Marc Zyngier, kvmarm
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] The hypervisor ITS emulation fails to trap critical configuration registers (e.g., GITS_CBASER, GITS_CTLR), allowing the host to bypass emulation and execute arbitrary ITS commands.
- [Critical] Rolling back the emulation state when hardware command processing times out causes a critical state desynchronization, leading to memory corruption.
- [High] Unvalidated `cmd_offset` initialization from hardware allows the host to cause out-of-bounds memory accesses in the hypervisor.
- [High] An unsigned loop counter underflow in the error path of `pkvm_setup_its_shadow_cmdq` causes an infinite loop and out-of-bounds memory unsharing.
- [High] The ITS emulation uses `__hyp_va()` on physical MMIO device addresses, causing a Translation Fault.
- [High] The error path in `pkvm_its_emulate_setup` leaks randomized hypervisor virtual addresses (hyp tags) to the host kernel.
- [Medium] The ITS emulation mishandles 32-bit MMIO accesses to the 64-bit `GITS_CWRITER` and `GITS_CREADR` registers, applying bitmasks to the wrong payload bits.
- [Medium] The error path in `pkvm_its_emulate_setup` hardcodes a 1-page return for private memory, permanently leaking hypervisor pages if more were allocated.
- [Medium] Mismatched size calculations in `pkvm_setup_its_shadow_cmdq` cause it to attempt pinning unshared pages when addresses or lengths are unaligned.
--
--- Patch [6]: [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation ---
commit 44d4bb1ba5e1c6603582c76567c28f1efdb0f865
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Shadow the ITS command queue and setup emulation
This patch introduces pKVM ITS command queue emulation. It sets up private
and shared memory regions to track the ITS command queue and intercepts
writes to GITS_CWRITER to validate commands before pushing them to the
hardware queue.
> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> index 63a42f520ed25..e943ab972aa5e 100644
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
[ ... ]
> @@ -35,3 +38,285 @@ void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool
> +static int submit_single_cmd(struct its_priv_state *its, bool retry)
> +{
[ ... ]
> + /*
> + * We can't spin here forever and we can't roll back
> + * the cmd queue pointer. Let's revert the cmd effects in the
> + * emulation layer and then go back to the driver to let it
> + * decide what to do next.
> + */
> + if (!timeout--)
> + return -EBUSY;
[Severity: Critical]
Can this state rollback cause memory corruption?
If the hardware takes too long to process the command and we time out here,
returning -EBUSY causes cwriter_write() to call process_cmd(..., true)
to revert the hypervisor's internal tracking state.
However, the hardware is still asynchronously processing the command and cannot
be aborted. Will the hardware operate on memory that the hypervisor believes
has already been unmapped and returned to the host?
> + }
> +
> + return 0;
> +}
[ ... ]
> +static void cwriter_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
> +{
> + struct its_priv_state *its = region->priv;
> + struct its_cmd_block cmd, raw;
> + u64 new_offset;
> + bool retry;
> + int i;
> +
> + new_offset = value & GITS_CWRITER_OFFSET;
[Severity: Medium]
Does this properly handle 32-bit partial MMIO accesses?
The pkvm_its_emulate_handler() passes the 32-bit payload directly to
the handler as the value parameter, but cwriter_write() ignores the
offset parameter and treats the incoming 32-bit payload as the absolute
64-bit value. This might extract the wrong bits.
cwriter_read() below also ignores the offset and always returns the full
64-bit register.
> + if (new_offset >= its->host_state->cmdq_len)
> + return;
> +
> + retry = FIELD_GET(GITS_CWRITER_RETRY, value);
> + while (its->cmd_offset != new_offset) {
> + memcpy(&raw, its->cmd_host_copy + its->cmd_offset, sizeof(raw));
[Severity: High]
Can the host exploit the initial cmd_offset to cause out-of-bounds memory
accesses here?
The cmd_offset is initialized directly from the hardware's GITS_CREADR in
pkvm_its_emulate_setup(), which the host could have manipulated prior to
setup. This cmd_offset is used as an index for memcpy() operations without
checking if it exceeds the provided cmdq_len.
> +
> + for (i = 0; i < ARRAY_SIZE(cmd.raw_cmd); i++)
> + cmd.raw_cmd[i] = le64_to_cpu(raw.raw_cmd_le[i]);
> +
> + if (process_cmd(its, &cmd, /* rollback */ false))
> + return;
> +
> + memcpy(its->cmd_original + its->cmd_offset, &raw, sizeof(struct its_cmd_block));
> +
> + if (its->needs_flush)
> + gic_flush_dcache_to_poc(its->cmd_original + its->cmd_offset, sizeof(cmd));
> + else
> + dsb(ishst);
> +
> + if (submit_single_cmd(its, retry)) {
> + WARN_ON(process_cmd(its, &cmd, /* rollback */ true));
> + return;
> + }
> + }
> +}
> +
> +static void cwriter_read(struct pkvm_protected_reg *region, u64 offset, u64 *read)
> +{
> + struct its_priv_state *its = region->priv;
> + *read = readq_relaxed(its->base + GITS_CWRITER);
> +}
> +
> +static struct its_handler its_handlers[] = {
> + ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
> + {},
> +};
[Severity: Critical]
Is it safe to only intercept GITS_CWRITER in this handler array?
It appears all other configuration registers (like GITS_CBASER and
GITS_CTLR) fall through to its_emulate_forward_req() which passes
the unintercepted writes directly to the hardware.
Could a malicious host directly modify GITS_CBASER to point the hardware queue
to an unvalidated memory page, bypassing all emulation checks and compromising
hypervisor isolation?
[ ... ]
> +static int pkvm_setup_its_shadow_cmdq(struct its_host_state *host_state)
> +{
> + u64 start_pfn, num_pages, i;
> + int ret;
> +
> + start_pfn = hyp_virt_to_pfn(host_state->cmd_host_copy);
> + num_pages = host_state->cmdq_len >> PAGE_SHIFT;
> +
> + for (i = 0; i < num_pages; i++) {
> + ret = __pkvm_host_share_hyp(start_pfn + i);
> + if (ret)
> + goto unshare_cmd_host;
> + }
> +
> + ret = hyp_pin_shared_mem(host_state->cmd_host_copy,
> + host_state->cmd_host_copy + host_state->cmdq_len);
[Severity: Medium]
Could this cause a mismatch in the number of pages pinned vs shared?
The calculation "num_pages = host_state->cmdq_len >> PAGE_SHIFT" truncates
the size, sharing exactly num_pages. However, hyp_pin_shared_mem() page-aligns
the boundaries up and down.
If cmd_host_copy or cmdq_len are unaligned, the pin operation could span
num_pages + 1 pages, attempting to pin an unshared page and failing the
ownership check.
> + if (ret)
> + goto unshare_cmd_host;
[ ... ]
> +unshare_cmd_host:
> + if (i == 0)
> + return ret;
> +
> + for (i = i - 1; i >= 0; i--)
> + __pkvm_host_unshare_hyp(start_pfn + i);
[Severity: High]
Will this loop underflow and cause an infinite loop?
The variable 'i' is defined as an unsigned 64-bit integer (u64) at the top of
the function. The condition 'i >= 0' is always true for unsigned variables,
meaning when 'i' reaches 0 it will wrap around to UINT64_MAX.
This could unshare wildly out-of-bounds PFNs and crash the hypervisor.
> + return ret;
> +}
[ ... ]
> +int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_state, void *priv,
> + size_t priv_num_pages)
> +{
[ ... ]
> + host_state->cmd_original = kern_hyp_va(host_state->cmd_original);
> + host_state->cmd_host_copy = kern_hyp_va(host_state->cmd_host_copy);
> +
> + ret = pkvm_setup_its_shadow_cmdq(host_state);
> + if (ret)
> + goto err_with_host_state;
[Severity: High]
Does this leak the randomized hypervisor virtual address to the host?
We mutate the host-provided host_state page in-place using kern_hyp_va()
which applies the hypervisor's KASLR tag. If pkvm_setup_its_shadow_cmdq()
fails, this modified page is returned directly to the host unscrubbed in
the err_with_host_state cleanup path, exposing the hypervisor's linear map tag.
> +
> + hyp_spin_lock_init(&priv_state->its_lock);
> +
> + priv_state->host_state = host_state;
> + priv_state->base = (void __iomem *)__hyp_va(dev_addr);
[Severity: High]
Is it correct to use __hyp_va() on a physical MMIO device address?
In ARM64 pKVM, __hyp_va() applies a fixed offset intended only for
mapping system RAM into the hypervisor's linear map. It does not work
for physical MMIO regions.
Attempting to access these virtual addresses will likely cause a Translation
Fault and crash the hypervisor during setup. its_emulate_forward_req() also
appears to use this macro.
> + priv_state->cmd_original = host_state->cmd_original;
> + priv_state->cmd_host_copy = host_state->cmd_host_copy;
> +
> + priv_state->cmd_offset = readq_relaxed(priv_state->base + GITS_CREADR) &
> + GITS_CREADR_OFFSET;
> + priv_state->needs_flush =
> + (readq_relaxed(priv_state->base + GITS_CBASER) & GITS_CBASER_SHAREABILITY_MASK) !=
> + GITS_CBASER_InnerShareable;
> +
> + its_reg->priv = priv_state;
> +
> + hyp_spin_unlock(&its_setup_lock);
> +
> + return 0;
> +err_with_host_state:
> + WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(host_state), 1));
> +err_with_priv:
> + WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(priv_state), 1));
[Severity: Medium]
Should this use priv_num_pages instead of a hardcoded 1?
The hypervisor allocates priv_num_pages for the private state tracking,
but if an error occurs here, it explicitly returns only 1 page to the
host. If priv_num_pages > 1, won't the remaining pages permanently leak?
> +err_unlock:
> + hyp_spin_unlock(&its_setup_lock);
> + return ret;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=6
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 07/13] KVM: arm64: Restrict host access to the private ITS tables
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (5 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:02 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 08/13] KVM: arm64: Trap & emulate the ITS MAPD command Sebastian Ene
` (5 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Make the last level of the tables(DeviceTable, Collection and vPE)
inaccessible to the host by donating them to the hypervisor.
This prevents a compromised host from patching an entry with an
address that it wants to write to and then using an ITS command to
write over the memory content from that address.
When tables are configured with indirect layout, shadow the first
layer by copying it to a separate table, update the gic ITS host
driver to use the copy instead of the original table and share the copy
between the host and the hypervisor. Make the original layer
innaccessible to the host by donating the table memory from the host to
the hypervisor.
This ensures that the pKVM ITS emulation mediates the
configuration written by the driver in the first layer of the table and
sanitizes the entries before writing to the original table programmed
in hardware. The update phase of the original table from the copy will
be done when commands are sent to the ITS.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 161 ++++++++++++++++++++++++++
1 file changed, 161 insertions(+)
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
index e943ab972aa5..1ce2f9d8fcf9 100644
--- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -237,6 +237,20 @@ static int pkvm_setup_its_shadow_cmdq(struct its_host_state *host_state)
return ret;
}
+static void pkvm_teardown_its_shadow_cmdq(struct its_host_state *host_state)
+{
+ u64 i, start_pfn, num_pages = host_state->cmdq_len >> PAGE_SHIFT;
+
+ start_pfn = hyp_virt_to_pfn(host_state->cmd_host_copy);
+ hyp_unpin_shared_mem(host_state->cmd_host_copy,
+ host_state->cmd_host_copy + host_state->cmdq_len);
+
+ for (i = 0; i < num_pages; i++)
+ WARN_ON(__pkvm_host_unshare_hyp(start_pfn + i));
+
+ WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(host_state->cmd_original), num_pages));
+}
+
static struct pkvm_protected_reg *get_region(phys_addr_t dev_addr)
{
int i;
@@ -249,6 +263,147 @@ static struct pkvm_protected_reg *get_region(phys_addr_t dev_addr)
return NULL;
}
+static void pkvm_unshare_shadow_table(void *shadow, u64 nr_pages)
+{
+ u64 i, start_pfn = hyp_virt_to_pfn(shadow);
+
+ hyp_unpin_shared_mem(shadow, shadow + (nr_pages << PAGE_SHIFT));
+
+ for (i = 0; i < nr_pages; i++)
+ WARN_ON(__pkvm_host_unshare_hyp(start_pfn + i));
+}
+
+static int pkvm_host_unmap_last_level(void *shadow, size_t num_pages, u32 psz)
+{
+ phys_addr_t table_addr;
+ u64 *table = shadow;
+ int i, end;
+ int ret;
+
+ end = (num_pages << PAGE_SHIFT) / sizeof(*table);
+ for (i = 0; i < end; i++) {
+ if (!(table[i] & GITS_BASER_VALID))
+ continue;
+
+ table_addr = table[i] & PHYS_MASK;
+ ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(table_addr), psz >> PAGE_SHIFT);
+ if (ret)
+ goto err_donate;
+ }
+
+ return 0;
+err_donate:
+ for (i = i - 1; i >= 0; i--) {
+ if (!(table[i] & GITS_BASER_VALID))
+ continue;
+
+ table_addr = table[i] & PHYS_MASK;
+ __pkvm_hyp_donate_host(hyp_phys_to_pfn(table_addr), psz >> PAGE_SHIFT);
+ }
+ return ret;
+}
+
+static int pkvm_share_shadow_table(void *shadow, u64 nr_pages)
+{
+ u64 i, ret, start_pfn = hyp_virt_to_pfn(shadow);
+
+ for (i = 0; i < nr_pages; i++) {
+ ret = __pkvm_host_share_hyp(start_pfn + i);
+ if (ret)
+ goto unshare;
+ }
+
+ ret = hyp_pin_shared_mem(shadow, shadow + (nr_pages << PAGE_SHIFT));
+ if (ret)
+ goto unshare;
+
+ return ret;
+unshare:
+ while (i--)
+ __pkvm_host_unshare_hyp(start_pfn + i);
+ return ret;
+}
+
+static void pkvm_host_map_last_level(void *shadow, size_t num_pages, u32 psz)
+{
+ u64 *table = shadow;
+ int i, end = (num_pages << PAGE_SHIFT) / sizeof(*table);
+ phys_addr_t table_addr;
+
+ for (i = 0; i < end; i++) {
+ if (!(table[i] & GITS_BASER_VALID))
+ continue;
+
+ table_addr = table[i] & PHYS_MASK;
+ WARN_ON(__pkvm_hyp_donate_host(hyp_phys_to_pfn(table_addr), psz >> PAGE_SHIFT));
+ }
+}
+
+static int pkvm_setup_its_shadow_baser(struct its_host_state *host_state)
+{
+ u64 baser_val, num_pages;
+ void *original_table, *snapshot_table;
+ int ret;
+ int i;
+
+ for (i = 0; i < GITS_BASER_NR_REGS; i++) {
+ baser_val = host_state->tables[i].val;
+ if (!(baser_val & GITS_BASER_VALID))
+ continue;
+
+ original_table = kern_hyp_va(host_state->tables[i].base);
+ num_pages = (1 << host_state->tables[i].order);
+
+ ret = __pkvm_host_donate_hyp(hyp_virt_to_pfn(original_table), num_pages);
+ if (ret)
+ goto err_donate;
+
+ if (baser_val & GITS_BASER_INDIRECT) {
+ if (!host_state->tables[i].base_snapshot) {
+ ret = -EINVAL;
+ goto err_with_donation;
+ }
+
+ snapshot_table = kern_hyp_va(host_state->tables[i].base_snapshot);
+ ret = pkvm_share_shadow_table(snapshot_table, num_pages);
+ if (ret)
+ goto err_with_donation;
+
+ ret = pkvm_host_unmap_last_level(original_table, num_pages,
+ host_state->tables[i].psz);
+ if (ret)
+ goto err_with_share;
+ }
+ }
+
+ return 0;
+err_with_share:
+ pkvm_unshare_shadow_table(snapshot_table, num_pages);
+err_with_donation:
+ __pkvm_hyp_donate_host(hyp_virt_to_pfn(original_table), num_pages);
+err_donate:
+ for (i = i - 1; i >= 0; i--) {
+ baser_val = host_state->tables[i].val;
+ if (!(baser_val & GITS_BASER_VALID))
+ continue;
+
+ original_table = kern_hyp_va(host_state->tables[i].base);
+ num_pages = (1 << host_state->tables[i].order);
+
+ if (baser_val & GITS_BASER_INDIRECT) {
+ snapshot_table = kern_hyp_va(host_state->tables[i].base_snapshot);
+ pkvm_unshare_shadow_table(snapshot_table, num_pages);
+
+ pkvm_host_map_last_level(original_table, num_pages,
+ host_state->tables[i].psz);
+ }
+
+ WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(original_table), num_pages));
+ }
+
+ return ret;
+}
+
DEFINE_HYP_SPINLOCK(its_setup_lock);
int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_state, void *priv,
@@ -294,6 +449,10 @@ int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_sta
if (ret)
goto err_with_host_state;
+ ret = pkvm_setup_its_shadow_baser(host_state);
+ if (ret)
+ goto err_with_shadow_cmdq;
+
hyp_spin_lock_init(&priv_state->its_lock);
priv_state->host_state = host_state;
@@ -312,6 +471,8 @@ int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_sta
hyp_spin_unlock(&its_setup_lock);
return 0;
+err_with_shadow_cmdq:
+ pkvm_teardown_its_shadow_cmdq(host_state);
err_with_host_state:
WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(host_state), 1));
err_with_priv:
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 07/13] KVM: arm64: Restrict host access to the private ITS tables
2026-08-07 16:43 ` [PATCH v2 07/13] KVM: arm64: Restrict host access to the private ITS tables Sebastian Ene
@ 2026-08-07 17:02 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:02 UTC (permalink / raw)
To: Sebastian Ene; +Cc: kvmarm, Oliver Upton, Marc Zyngier
Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [Critical] Host can bypass ITS table protection via mismatched virtual-to-physical address mapping.
- [Critical] TOCTOU vulnerability in `check_table_update` during rollback allows a malicious host to intentionally crash the hypervisor.
- [Critical] Wrong architectural valid bit is used for L1 ITS table entries, breaking L2 table donation.
- [Critical] EL2 ITS emulation handlers process host commands without acquiring `its->its_lock`, creating a critical data race.
- [Critical] Software rollback of EL2 ITS tracking state on command timeout leaves hardware desynchronized, allowing memory corruption.
- [Critical] Missing alignment validation for Level 2 table physical addresses allows hardware masking exploits.
- [Critical] `get_num_itt_pages` ignores intra-page offsets, allowing unaligned ITTs to overflow into unverified memory.
- [High] Missing rollback on partial command processing failure leaks donated pages and desynchronizes EL2 metadata.
- [High] Undefined Behavior via unsafe bitshift on host-controlled `table->order`.
- [High] `check_table_update` incorrectly returns early during rollback, permanently leaking Level 2 table pages.
- [High] Error path for `kvm_iodev_add` failure completely bypasses shadow resource teardown.
- [High] Potential resource leak due to unaligned `cmdq_len` calculating to 0 pages during teardown.
- [High] ITS indirect table entries are accessed natively instead of little-endian, breaking behavior on big-endian kernels.
- [High] Bitwise calculation donates 0 pages if the ITS table page size is smaller than the KVM page size, breaking isolation.
- [Medium] `track_pfn_remove` fails to clear the device tracking entry if the PFN remains pinned by another device, causing a resource leak.
--
--- Patch [7]: [PATCH v2 07/13] KVM: arm64: Restrict host access to the private ITS tables ---
commit 427f53daa5c5d6b87ee6ee19cf96de15042a131a
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Restrict host access to the private ITS tables
Make the last level of the tables(DeviceTable, Collection and vPE)
inaccessible to the host by donating them to the hypervisor. This prevents
a compromised host from patching an entry with an address that it wants to
write to and then using an ITS command to write over the memory content
from that address.
> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> index e943ab972aa5e..1ce2f9d8fcf9c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> @@ -237,6 +237,20 @@ static int pkvm_setup_its_shadow_cmdq(struct its_host_state *host_state)
> return ret;
> }
>
> +static void pkvm_teardown_its_shadow_cmdq(struct its_host_state *host_state)
> +{
> + u64 i, start_pfn, num_pages = host_state->cmdq_len >> PAGE_SHIFT;
[Severity: High]
Does this code leak hypervisor memory if the command queue is unaligned?
If the host-provided host_state->cmdq_len is less than PAGE_SIZE (e.g., a
4KB command queue on a KVM host with 64KB pages), host_state->cmdq_len >>
PAGE_SHIFT evaluates to 0.
The subsequent loop to unshare and the call to __pkvm_hyp_donate_host
execute 0 times, leaking the shadow command queue pages in the hypervisor.
> +
> + start_pfn = hyp_virt_to_pfn(host_state->cmd_host_copy);
> + hyp_unpin_shared_mem(host_state->cmd_host_copy,
> + host_state->cmd_host_copy + host_state->cmdq_len);
> +
> + for (i = 0; i < num_pages; i++)
> + WARN_ON(__pkvm_host_unshare_hyp(start_pfn + i));
> +
> + WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(host_state->cmd_original), num_pages));
> +}
> +
[ ... ]
> +static int pkvm_host_unmap_last_level(void *shadow, size_t num_pages, u32 psz)
> +{
> + phys_addr_t table_addr;
> + u64 *table = shadow;
> + int i, end;
> + int ret;
> +
> + end = (num_pages << PAGE_SHIFT) / sizeof(*table);
> + for (i = 0; i < end; i++) {
> + if (!(table[i] & GITS_BASER_VALID))
> + continue;
[Severity: Critical]
Does this code use the wrong architectural valid bit for L1 ITS table
entries?
The condition checks if (!(table[i] & GITS_BASER_VALID)) to verify L1
table entries. However, GITS_BASER_VALID is bit 62, which is architecturally
reserved as zero for memory-resident L1 table entries. The actual Valid bit
is architecturally bit 63.
This evaluates to true, the loop continues, and no L2 tables are ever
unmapped or donated.
This issue is also present in check_table_update():
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
if (!((new_entry ^ prev_entry) & GITS_BASER_VALID))
return 0;
...
}
> +
> + table_addr = table[i] & PHYS_MASK;
> + ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(table_addr), psz >> PAGE_SHIFT);
> + if (ret)
> + goto err_donate;
> + }
> +
> + return 0;
[ ... ]
> +static int pkvm_setup_its_shadow_baser(struct its_host_state *host_state)
> +{
> + u64 baser_val, num_pages;
> + void *original_table, *snapshot_table;
> + int ret;
> + int i;
> +
> + for (i = 0; i < GITS_BASER_NR_REGS; i++) {
> + baser_val = host_state->tables[i].val;
> + if (!(baser_val & GITS_BASER_VALID))
> + continue;
> +
> + original_table = kern_hyp_va(host_state->tables[i].base);
> + num_pages = (1 << host_state->tables[i].order);
[Severity: High]
Does this code trigger undefined behavior through an unsafe bitshift?
If the host supplies a table->order value of 31 or greater, (1 << order)
overflows into the sign bit because the literal 1 is a signed 32-bit integer.
This overflow leads to improper memory sizing.
This issue is also present in check_table_update():
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
lvl1_table_sz = (1 << table->order) << PAGE_SHIFT;
...
}
[Severity: Critical]
Can the host bypass ITS table protection via mismatched virtual-to-physical
address mapping?
The host supplies host_state->tables[i] where .base is a dummy virtual
address it owns, but .val contains the physical address of hypervisor memory.
pkvm_setup_its_shadow_baser() donates the dummy page at .base:
> + ret = __pkvm_host_donate_hyp(hyp_virt_to_pfn(original_table), num_pages);
Then baser_write() sets the hardware GITS_BASER directly to the physical
address in .val without verifying they correspond:
arch/arm64/kvm/hyp/nvhe/its_emulate.c:baser_write() {
...
baser = its->host_state->tables[baser_idx].val;
...
value |= (baser & GENMASK_ULL(47, 12)) | (baser & GENMASK_ULL(9, 0));
writeq_relaxed(value, its->base + offset);
...
}
This allows the host to point the hardware ITS at arbitrary EL2 memory,
bypassing KVM's memory isolation.
> + if (ret)
> + goto err_donate;
[ ... ]
> @@ -294,6 +449,10 @@ int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_sta
> if (ret)
> goto err_with_host_state;
>
> + ret = pkvm_setup_its_shadow_baser(host_state);
> + if (ret)
> + goto err_with_shadow_cmdq;
> +
> hyp_spin_lock_init(&priv_state->its_lock);
>
> priv_state->host_state = host_state;
> @@ -312,6 +471,8 @@ int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_sta
> hyp_spin_unlock(&its_setup_lock);
>
> return 0;
> +err_with_shadow_cmdq:
> + pkvm_teardown_its_shadow_cmdq(host_state);
> err_with_host_state:
> WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(host_state), 1));
> err_with_priv:
[Severity: High]
Does this code skip critical hypervisor-side resource cleanup when
kvm_iodev_add fails?
If kvm_iodev_add fails during initialization, the code executes goto
err_with_priv; which is located after the teardown labels err_with_shadow_cmdq
and err_with_host_state.
This skips the cleanup of the shadow structures and permanently leaks shadow
ITS command queues and donated memory.
[Severity: High]
Does process_its_mapd() leak donated pages and desynchronize EL2 metadata
when partial command processing fails?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:process_its_mapd() {
...
ret = check_table_update(its, device_id, GITS_BASER_TYPE_DEVICE, rollback);
if (ret)
return ret;
return track_pfn(its, device_id, itt_pfn, num_pages, remove);
}
If check_table_update() succeeds in donating an L2 page but the subsequent
track_pfn() fails, process_its_mapd() returns an error directly.
This error is propagated to cwriter_write(), which blindly returns without
invoking rollback:
arch/arm64/kvm/hyp/nvhe/its_emulate.c:cwriter_write() {
...
if (process_cmd(its, &cmd, /* rollback */ false))
return;
...
}
This leaves EL2 metadata desynchronized from the actual hardware execution
and permanently leaks L2 table pages in EL2 memory.
[Severity: Medium]
Does track_pfn_remove() leak resources by failing to clear the device tracking
entry if the PFN remains pinned?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:track_pfn_remove() {
...
ret = __pkvm_host_unshare_hyp(pfn);
if (ret == -EBUSY) {
hyp_unpin_shared_mem(virt, virt + PAGE_SIZE);
ret = __pkvm_host_unshare_hyp(pfn);
if (ret == -EBUSY)
return 0;
WARN_ON(ret);
}
memset(&entries[i], 0, sizeof(struct dte_entry));
...
}
If an ITT page is shared between multiple devices and __pkvm_host_unshare_hyp()
returns -EBUSY, the function executes an early return 0;, skipping the
memset() step. This leaves a zombie entry in the fixed-size
its->tracked_entries array.
[Severity: High]
Does check_table_update() incorrectly return early during rollback, permanently
leaking Level 2 table pages?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
new_entry = snapshot_table[new_entry_index];
prev_entry = original_table[new_entry_index];
if (!((new_entry ^ prev_entry) & GITS_BASER_VALID))
return 0;
...
}
When a command fails and check_table_update() is called with rollback=true,
it evaluates ((new_entry ^ prev_entry) & GITS_BASER_VALID). Since the forward
pass already updated original_table to match snapshot_table, both entries are
identical and their XOR is 0.
The function erroneously returns 0 early, skipping the rollback donation logic
entirely.
[Severity: Critical]
Does check_table_update() contain a time-of-check to time-of-use vulnerability
during rollback that allows a malicious host to crash the hypervisor?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
new_entry = snapshot_table[new_entry_index];
...
if (rollback)
new_entry = new_entry ^ GITS_BASER_VALID;
if (new_entry & GITS_BASER_VALID)
ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK),
table->psz >> PAGE_SHIFT);
...
}
During rollback, KVM re-reads new_entry from the host-shared snapshot_table.
If the host concurrently mutated it to have the valid bit cleared, the XOR
check proceeds. The rollback logic then flips new_entry to be valid and KVM
attempts to call __pkvm_host_donate_hyp() on the physical address embedded in
this host-controlled new_entry.
If the address is invalid, the donation fails, and the returned error is blindly
wrapped in a fatal WARN_ON in cwriter_write(), crashing EL2:
arch/arm64/kvm/hyp/nvhe/its_emulate.c:cwriter_write() {
...
if (submit_single_cmd(its, retry)) {
WARN_ON(process_cmd(its, &cmd, /* rollback */ true));
return;
}
...
}
[Severity: Critical]
Does cwriter_write() process host commands without acquiring its->its_lock,
creating a critical data race?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:cwriter_write() {
struct its_priv_state *its = region->priv;
struct its_cmd_block cmd, raw;
u64 new_offset;
bool retry;
int i;
new_offset = value & GITS_CWRITER_OFFSET;
...
}
The cwriter_write() MMIO handler modifies its->cmd_offset and shared internal
tracking structures like its->tracked_entries without acquiring its->its_lock.
A malicious host can issue concurrent writes to GITS_CWRITER from multiple
physical CPUs, causing the unlocked code running at EL2 to corrupt its own
metadata.
[Severity: Critical]
Does cwriter_write() leave hardware desynchronized on command timeout,
allowing memory corruption?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:cwriter_write() {
...
if (submit_single_cmd(its, retry)) {
WARN_ON(process_cmd(its, &cmd, /* rollback */ true));
return;
}
...
}
If the hardware stalls, submit_single_cmd() times out. EL2 then rolls back
its software metadata, returning the ITT page to the host. However, the
physical hardware queue has already advanced.
When the hardware un-stalls and processes the command, it will use the physical
page as an ITT, blindly overwriting whatever data the host subsequently placed
there.
[Severity: Critical]
Does check_table_update() miss alignment validation for Level 2 table physical
addresses, allowing hardware masking exploits?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
if (new_entry & GITS_BASER_VALID)
ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK),
table->psz >> PAGE_SHIFT);
...
}
KVM donates the range starting at the physical address provided in new_entry,
without verifying the address is appropriately aligned to table->psz. The
hardware ITS expects addresses aligned to table->psz (e.g., 64KB) and will
implicitly mask lower bits.
If the host provides a 4KB-aligned address inside a 64KB region, KVM donates
the 64KB range starting there, but the hardware accesses the true 64KB-aligned
base below it, writing into hypervisor memory that was never checked or donated.
[Severity: Critical]
Does get_num_itt_pages() ignore intra-page offsets, allowing unaligned ITTs
to overflow into unverified memory?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:get_num_itt_pages() {
...
sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
return PAGE_ALIGN(sz) >> PAGE_SHIFT;
}
get_num_itt_pages() calculates the required pages using PAGE_ALIGN(sz) >>
PAGE_SHIFT, completely disregarding the starting alignment of itt_addr, which
only has a 256-byte alignment requirement.
If an ITT fits within a single page logically but crosses a page boundary
physically (e.g., size 512 bytes starting near the end of a page), the math
returns 1 page. KVM pins only the first page, allowing the hardware ITS to
write into the unpinned next page, potentially corrupting hypervisor state.
[Severity: High]
Does check_table_update() access ITS indirect table entries natively instead
of little-endian, breaking behavior on big-endian kernels?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
new_entry = snapshot_table[new_entry_index];
...
}
The host Linux driver architecturally writes GIC structures in little-endian
format (cpu_to_le64). The KVM hypervisor accesses these memory-resident
entries via a native 64-bit load.
On a big-endian system, the resulting bytes are swapped, causing validation
checks and physical address extractions to return completely incorrect values.
[Severity: High]
Does check_table_update() donate 0 pages if the ITS table page size is smaller
than the KVM page size, breaking isolation?
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
if (new_entry & GITS_BASER_VALID)
ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK),
table->psz >> PAGE_SHIFT);
...
}
KVM determines the amount of memory to donate for L2 tables using
table->psz >> PAGE_SHIFT. If the hardware is configured to use a smaller
page size than the hypervisor (e.g., psz=4096, KVM PAGE_SIZE=65536), this
evaluates to 0.
__pkvm_host_donate_hyp() receives 0 pages and the host retains full write
access to the table memory, bypassing isolation.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=7
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 08/13] KVM: arm64: Trap & emulate the ITS MAPD command
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (6 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 07/13] KVM: arm64: Restrict host access to the private ITS tables Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 16:57 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 09/13] KVM: arm64: Trap & emulate the ITS MAPC command Sebastian Ene
` (4 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Parse the MAPD command and extract the ITT address to sanitize it. When
the command has the valid bit set, share and pin the memory that holds
the ITT table with the hypervisor to prevent it from being given to
someone else (eg. a VM). Use the pinning as a mechanism to get a grip
to the page and to prevent other users of the pKVM API from sharing or
donating the page for something else.
This is to prevent a a situation where a page is given to someone else
and then a MAPTI command is used to create an ITE entry in that page.
Implement shadow table updates for the first level of the indirect
tables when a MAPD command is issued. Compare the host view of the table
for the entry identified by the deviceId with the original table at the
same index and check if the valid bit is changed. If it didn't change,
don't update the original table. If it changed, verify if the new entry
has the valid bit set and donate the level2 table from the host to the
hypervisor (with the address of the table used from the new entry).
If the new entry has the valid bit cleared, donate the level2 table
from the hypervisor to the host with the address of the table extracted
from the original table managed by the hypervisor.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 240 +++++++++++++++++++++++++-
1 file changed, 238 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
index 1ce2f9d8fcf9..071a08d3602d 100644
--- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -54,6 +54,11 @@ struct its_handler {
.read = (read_cb), \
}
+struct dte_entry {
+ u32 device_id;
+ u64 itt_pfn;
+};
+
struct its_priv_state {
/* The location of the ITS in the hypervisor VA */
void __iomem *base;
@@ -66,6 +71,9 @@ struct its_priv_state {
hyp_spinlock_t its_lock;
struct its_host_state *host_state;
+ u16 empty_entry;
+ u16 num_tracked_entries;
+ struct dte_entry tracked_entries[];
};
#define GITS_CWRITER_RETRY BIT_ULL(0)
@@ -110,11 +118,236 @@ static int submit_single_cmd(struct its_priv_state *its, bool retry)
return 0;
}
+static int get_num_itt_pages(struct its_priv_state *its, u8 num_bits)
+{
+ u64 gits_typer, nr_ites;
+ size_t sz;
+
+ gits_typer = readq_relaxed(its->base + GITS_TYPER);
+ if (num_bits > FIELD_GET(GITS_TYPER_IDBITS, gits_typer))
+ return -EINVAL;
+
+ nr_ites = BIT_ULL(num_bits + 1);
+ sz = nr_ites * (FIELD_GET(GITS_TYPER_ITT_ENTRY_SIZE, gits_typer) + 1);
+ sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
+
+ return PAGE_ALIGN(sz) >> PAGE_SHIFT;
+}
+
+static struct its_baser *get_table_from_snapshot(struct its_host_state *host, u64 baser_type)
+{
+ int i;
+
+ for (i = 0; i < GITS_BASER_NR_REGS; i++) {
+ if (GITS_BASER_TYPE(host->tables[i].val) == baser_type)
+ return &host->tables[i];
+ }
+
+ return NULL;
+}
+
+static int check_table_update(struct its_priv_state *its, u32 device_id, u64 type, bool rollback)
+{
+ struct its_baser *table = get_table_from_snapshot(its->host_state, type);
+ size_t lvl2_entry_sz, lvl1_table_sz, num_lvl2_entries, num_lvl1_entries;
+ u64 *snapshot_table, *original_table;
+ u64 prev_entry, new_entry;
+ u32 new_entry_index;
+ int ret;
+
+ if (!table)
+ return -EINVAL;
+
+ /* We only do shadow udates for the first level of indirect tables */
+ if (!(table->val & GITS_BASER_INDIRECT))
+ return 0;
+
+ lvl2_entry_sz = GITS_BASER_ENTRY_SIZE(table->val);
+ num_lvl2_entries = table->psz / lvl2_entry_sz;
+
+ lvl1_table_sz = (1 << table->order) << PAGE_SHIFT;
+ num_lvl1_entries = lvl1_table_sz / sizeof(u64);
+
+ new_entry_index = device_id / num_lvl2_entries;
+ if (new_entry_index >= num_lvl1_entries)
+ return -ENOSPC;
+
+ snapshot_table = kern_hyp_va(table->base_snapshot);
+ original_table = kern_hyp_va(table->base);
+
+ /*
+ * Look at the host table copy and if the entry hasn't changed the valid
+ * bit compared to the original table used by the hardwre, don't update anything.
+ */
+ new_entry = snapshot_table[new_entry_index];
+ prev_entry = original_table[new_entry_index];
+ if (!((new_entry ^ prev_entry) & GITS_BASER_VALID))
+ return 0;
+
+ /*
+ * The host can play nasty tricks with read-modify-write after a
+ * rollback is triggered but we still hold on to the original tables
+ * which are hyp managed and we don't give back any other page to the
+ * host.
+ */
+ if (rollback)
+ new_entry = new_entry ^ GITS_BASER_VALID;
+
+ if (new_entry & GITS_BASER_VALID)
+ ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK),
+ table->psz >> PAGE_SHIFT);
+ else
+ ret = __pkvm_hyp_donate_host(hyp_phys_to_pfn(prev_entry & PHYS_MASK),
+ table->psz >> PAGE_SHIFT);
+ if (ret)
+ return ret;
+
+ original_table[new_entry_index] = new_entry;
+ return 0;
+}
+
+static int track_pfn_add(struct its_priv_state *its, u32 device_id, u64 pfn)
+{
+ void *virt = hyp_phys_to_virt(hyp_pfn_to_phys(pfn));
+ struct dte_entry *entries = &its->tracked_entries[0];
+ bool pfn_shared = false;
+ int ret;
+ int i;
+
+ for (i = 0; i < its->num_tracked_entries; i++) {
+ if (entries[i].itt_pfn == pfn) {
+ if (entries[i].device_id != device_id) {
+ pfn_shared = true;
+ break;
+ } else {
+ return hyp_pin_shared_mem(virt, virt + PAGE_SIZE);
+ }
+ }
+ }
+
+ if (its->empty_entry >= its->num_tracked_entries)
+ return -ENOSPC;
+
+ if (!pfn_shared) {
+ ret = __pkvm_host_share_hyp(pfn);
+ if (ret)
+ return ret;
+ }
+
+ ret = hyp_pin_shared_mem(virt, virt + PAGE_SIZE);
+ if (ret) {
+ __pkvm_host_unshare_hyp(pfn);
+ return ret;
+ }
+
+ entries[its->empty_entry].itt_pfn = pfn;
+ entries[its->empty_entry].device_id = device_id;
+
+ for (i = 0; i < its->num_tracked_entries; i++) {
+ if (!entries[i].itt_pfn && !entries[i].device_id)
+ break;
+ }
+ its->empty_entry = i;
+ return 0;
+}
+
+static int track_pfn_remove(struct its_priv_state *its, u32 device_id, u64 pfn)
+{
+ void *virt = hyp_phys_to_virt(hyp_pfn_to_phys(pfn));
+ struct dte_entry *entries = &its->tracked_entries[0];
+ int ret;
+ int i;
+
+ for (i = 0; i < its->num_tracked_entries; i++) {
+ if (entries[i].itt_pfn != pfn || entries[i].device_id != device_id)
+ continue;
+
+ /* To decrement the refcount, first try to unshare it */
+ ret = __pkvm_host_unshare_hyp(pfn);
+ if (ret == -EBUSY) {
+ hyp_unpin_shared_mem(virt, virt + PAGE_SIZE);
+ ret = __pkvm_host_unshare_hyp(pfn);
+ if (ret == -EBUSY)
+ return 0;
+
+ WARN_ON(ret);
+ }
+
+ memset(&entries[i], 0, sizeof(struct dte_entry));
+ its->empty_entry = i;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int track_pfn(struct its_priv_state *its, u32 device_id, u64 pfn, int num_pages,
+ bool remove)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < num_pages; i++) {
+ if (remove)
+ ret = track_pfn_remove(its, device_id, pfn + i);
+ else
+ ret = track_pfn_add(its, device_id, pfn + i);
+
+ if (ret)
+ goto err_track_pfn;
+ }
+
+ return 0;
+err_track_pfn:
+ for (i = i - 1; i >= 0; i--) {
+ if (remove)
+ WARN_ON(track_pfn_add(its, device_id, pfn + i));
+ else
+ WARN_ON(track_pfn_remove(its, device_id, pfn + i));
+ }
+ return ret;
+}
+
+static int process_its_mapd(struct its_priv_state *its, struct its_cmd_block *cmd, bool rollback)
+{
+ phys_addr_t itt_addr = cmd->raw_cmd[2] & GENMASK(51, 8);
+ bool remove = !(cmd->raw_cmd[2] & BIT(63));
+ u8 size = cmd->raw_cmd[1] & GENMASK(4, 0);
+ u32 device_id = cmd->raw_cmd[0] >> 32;
+ int num_pages, ret;
+ u64 itt_pfn;
+
+ if (rollback)
+ remove = !remove;
+
+ itt_pfn = hyp_phys_to_pfn(itt_addr);
+ num_pages = get_num_itt_pages(its, size);
+ if (num_pages < 0)
+ return num_pages;
+
+ ret = check_table_update(its, device_id, GITS_BASER_TYPE_DEVICE, rollback);
+ if (ret)
+ return ret;
+
+ return track_pfn(its, device_id, itt_pfn, num_pages, remove);
+}
+
static int process_cmd(struct its_priv_state *its, struct its_cmd_block *cmd,
bool rollback)
{
- /* Passthrough everything for now */
- return 0;
+ u8 req_type = cmd->raw_cmd[0] & GENMASK_ULL(7, 0);
+ int ret = 0;
+
+ switch (req_type) {
+ case GITS_CMD_MAPD:
+ ret = process_its_mapd(its, cmd, rollback);
+ break;
+ default:
+ /* Passthrough everything for now */
+ break;
+ }
+
+ return ret;
}
static void cwriter_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
@@ -459,6 +692,9 @@ int pkvm_its_emulate_setup(phys_addr_t dev_addr, struct its_host_state *host_sta
priv_state->base = (void __iomem *)__hyp_va(dev_addr);
priv_state->cmd_original = host_state->cmd_original;
priv_state->cmd_host_copy = host_state->cmd_host_copy;
+ priv_state->empty_entry = 0;
+ priv_state->num_tracked_entries = ((priv_num_pages << PAGE_SHIFT) -
+ offsetof(struct its_priv_state, tracked_entries)) / sizeof(struct dte_entry);
priv_state->cmd_offset = readq_relaxed(priv_state->base + GITS_CREADR) &
GITS_CREADR_OFFSET;
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 08/13] KVM: arm64: Trap & emulate the ITS MAPD command
2026-08-07 16:43 ` [PATCH v2 08/13] KVM: arm64: Trap & emulate the ITS MAPD command Sebastian Ene
@ 2026-08-07 16:57 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 16:57 UTC (permalink / raw)
To: Sebastian Ene; +Cc: kvmarm, Marc Zyngier, Oliver Upton
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] The rollback logic in check_table_update is dead code and allows host-driven hypervisor memory compromise.
- [Critical] get_num_itt_pages calculates ITT boundaries incorrectly, resulting in underpinning of unaligned ITT tail pages.
- [Critical] check_table_update does not enforce physical address alignment for Level 2 tables, allowing hardware ITS memory corruption.
- [High] track_pfn_remove skips clearing the tracking entry on early return, permanently leaking dte_entry slots.
- [High] Partial failure in process_its_mapd aborts command execution without rolling back EL2 metadata donations.
- [High] track_pfn_add leaks hyp_page refcounts when mapping duplicate commands, permanently stranding host memory.
- [High] Architecturally legal MAPD unmap commands fail with -EINVAL, permanently stalling the emulated ITS command queue.
- [Medium] Re-mapping an existing device to a new ITT leaks the old ITT pages.
--
--- Patch [8]: [PATCH v2 08/13] KVM: arm64: Trap & emulate the ITS MAPD command ---
commit d27a7f8a2aeffcb0dd24c8e126274036ef6c275e
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Trap & emulate the ITS MAPD command
Parse the MAPD command and extract the ITT address to sanitize it. When
the command has the valid bit set, share and pin the memory that holds
the ITT table with the hypervisor to prevent it from being given to
someone else (eg. a VM). Use the pinning as a mechanism to get a grip
to the page and to prevent other users of the pKVM API from sharing or
donating the page for something else.
> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> index 1ce2f9d8fcf9c..071a08d3602d7 100644
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
[ ... ]
> @@ -110,11 +118,236 @@ static int submit_single_cmd(struct its_priv_state *its, bool retry)
> return 0;
> }
>
> +static int get_num_itt_pages(struct its_priv_state *its, u8 num_bits)
> +{
> + u64 gits_typer, nr_ites;
> + size_t sz;
> +
> + gits_typer = readq_relaxed(its->base + GITS_TYPER);
> + if (num_bits > FIELD_GET(GITS_TYPER_IDBITS, gits_typer))
> + return -EINVAL;
> +
> + nr_ites = BIT_ULL(num_bits + 1);
> + sz = nr_ites * (FIELD_GET(GITS_TYPER_ITT_ENTRY_SIZE, gits_typer) + 1);
> + sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
> +
> + return PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +}
[Severity: Critical]
Does this calculation miss the intra-page offset of the itt_addr? If a host
provides an ITS-aligned but not page-aligned address, sz alone doesn't
account for the page boundary crossing. This could result in under-pinning
the tail page and allowing it to be mapped to a guest while the hardware ITS
writes to it.
[ ... ]
> +static int check_table_update(struct its_priv_state *its, u32 device_id, u64 type, bool rollback)
> +{
> + struct its_baser *table = get_table_from_snapshot(its->host_state, type);
> + size_t lvl2_entry_sz, lvl1_table_sz, num_lvl2_entries, num_lvl1_entries;
> + u64 *snapshot_table, *original_table;
> + u64 prev_entry, new_entry;
> + u32 new_entry_index;
> + int ret;
> +
[ ... ]
> + snapshot_table = kern_hyp_va(table->base_snapshot);
> + original_table = kern_hyp_va(table->base);
> +
> + /*
> + * Look at the host table copy and if the entry hasn't changed the valid
> + * bit compared to the original table used by the hardwre, don't update anything.
> + */
> + new_entry = snapshot_table[new_entry_index];
> + prev_entry = original_table[new_entry_index];
> + if (!((new_entry ^ prev_entry) & GITS_BASER_VALID))
> + return 0;
[Severity: Critical]
If a command fails and triggers a rollback, will this early return bypass
the rollback logic? Because the forward pass already updated original_table,
new_entry and prev_entry will match here. This leaves a window for the host
to manipulate snapshot_table and maliciously donate arbitrary hypervisor
pages.
> +
> + /*
> + * The host can play nasty tricks with read-modify-write after a
> + * rollback is triggered but we still hold on to the original tables
> + * which are hyp managed and we don't give back any other page to the
> + * host.
> + */
> + if (rollback)
> + new_entry = new_entry ^ GITS_BASER_VALID;
> +
> + if (new_entry & GITS_BASER_VALID)
> + ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK),
> + table->psz >> PAGE_SHIFT);
[Severity: Critical]
Does this pass the physical address to the hypervisor without enforcing the
architectural page size alignment (GITS_BASER.Page_Size)? If the host
provides an unaligned physical address, the hypervisor will donate based on
the unaligned PFN, but the hardware ITS masks the lower bits, potentially
accessing memory outside the donated range.
> + else
> + ret = __pkvm_hyp_donate_host(hyp_phys_to_pfn(prev_entry & PHYS_MASK),
> + table->psz >> PAGE_SHIFT);
> + if (ret)
> + return ret;
> +
> + original_table[new_entry_index] = new_entry;
> + return 0;
> +}
> +
> +static int track_pfn_add(struct its_priv_state *its, u32 device_id, u64 pfn)
> +{
> + void *virt = hyp_phys_to_virt(hyp_pfn_to_phys(pfn));
> + struct dte_entry *entries = &its->tracked_entries[0];
> + bool pfn_shared = false;
> + int ret;
> + int i;
> +
> + for (i = 0; i < its->num_tracked_entries; i++) {
> + if (entries[i].itt_pfn == pfn) {
> + if (entries[i].device_id != device_id) {
> + pfn_shared = true;
> + break;
> + } else {
> + return hyp_pin_shared_mem(virt, virt + PAGE_SIZE);
> + }
[Severity: High]
If the host issues multiple MAPD commands with Valid=1 for the same
device_id and ITT, can this leak hyp_page refcounts? It increments the pin
count for a duplicate mapping without adding a new tracking entry. When
unmapped, track_pfn_remove unpins only once and clears the tracking entry,
which would permanently strand the extra pins.
> + }
> + }
[ ... ]
> +static int track_pfn_remove(struct its_priv_state *its, u32 device_id, u64 pfn)
> +{
> + void *virt = hyp_phys_to_virt(hyp_pfn_to_phys(pfn));
> + struct dte_entry *entries = &its->tracked_entries[0];
> + int ret;
> + int i;
> +
> + for (i = 0; i < its->num_tracked_entries; i++) {
> + if (entries[i].itt_pfn != pfn || entries[i].device_id != device_id)
> + continue;
> +
> + /* To decrement the refcount, first try to unshare it */
> + ret = __pkvm_host_unshare_hyp(pfn);
> + if (ret == -EBUSY) {
> + hyp_unpin_shared_mem(virt, virt + PAGE_SIZE);
> + ret = __pkvm_host_unshare_hyp(pfn);
> + if (ret == -EBUSY)
> + return 0;
[Severity: High]
Does this early return skip clearing the tracking entry? If an ITT page is
shared by multiple devices and the second unshare returns -EBUSY, bypassing
the memset below will permanently leak the dte_entry slot for this device,
eventually leading to -ENOSPC.
> +
> + WARN_ON(ret);
> + }
> +
> + memset(&entries[i], 0, sizeof(struct dte_entry));
> + its->empty_entry = i;
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
[ ... ]
> +static int process_its_mapd(struct its_priv_state *its, struct its_cmd_block *cmd, bool rollback)
> +{
> + phys_addr_t itt_addr = cmd->raw_cmd[2] & GENMASK(51, 8);
> + bool remove = !(cmd->raw_cmd[2] & BIT(63));
> + u8 size = cmd->raw_cmd[1] & GENMASK(4, 0);
> + u32 device_id = cmd->raw_cmd[0] >> 32;
> + int num_pages, ret;
> + u64 itt_pfn;
> +
> + if (rollback)
> + remove = !remove;
> +
> + itt_pfn = hyp_phys_to_pfn(itt_addr);
> + num_pages = get_num_itt_pages(its, size);
[Severity: High]
The GICv3 specification dictates that when a MAPD command has Valid=0 (an
unmap), the ITT_addr and Size fields are IGNORED. If a driver provides zeros
for these ignored fields during an unmap, will extracting them here cause
track_pfn_remove to fail with -EINVAL, permanently stalling the emulated ITS
command queue?
> + if (num_pages < 0)
> + return num_pages;
> +
> + ret = check_table_update(its, device_id, GITS_BASER_TYPE_DEVICE, rollback);
> + if (ret)
> + return ret;
> +
> + return track_pfn(its, device_id, itt_pfn, num_pages, remove);
> +}
[Severity: High]
If track_pfn() fails here (for example, if the tracking array is full), the
error is returned directly. Will this leave the mutations and donations
performed by check_table_update() permanently applied without rolling them
back or executing the command in hardware?
[Severity: Medium]
When a MAPD command is issued with Valid=1 for a device_id that is already
mapped, but with a new ITT_addr, does this leak the old ITT pages? The new
PFNs are tracked, but there doesn't appear to be logic to look up and remove
the pre-existing mapping for this device_id.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=8
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 09/13] KVM: arm64: Trap & emulate the ITS MAPC command
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (7 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 08/13] KVM: arm64: Trap & emulate the ITS MAPD command Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:04 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 10/13] KVM: arm64: Restrict host updates to GITS_CTLR Sebastian Ene
` (3 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Parse the MAPC command and verify if we need to do any updates to the
shadow collection table.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
index 071a08d3602d..5629e2a070df 100644
--- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -332,6 +332,13 @@ static int process_its_mapd(struct its_priv_state *its, struct its_cmd_block *cm
return track_pfn(its, device_id, itt_pfn, num_pages, remove);
}
+static int process_its_mapc(struct its_priv_state *its, struct its_cmd_block *cmd, bool rollback)
+{
+ u32 icid = cmd->raw_cmd[2] & GENMASK(15, 0);
+
+ return check_table_update(its, icid, GITS_BASER_TYPE_COLLECTION, rollback);
+}
+
static int process_cmd(struct its_priv_state *its, struct its_cmd_block *cmd,
bool rollback)
{
@@ -342,6 +349,10 @@ static int process_cmd(struct its_priv_state *its, struct its_cmd_block *cmd,
case GITS_CMD_MAPD:
ret = process_its_mapd(its, cmd, rollback);
break;
+
+ case GITS_CMD_MAPC:
+ ret = process_its_mapc(its, cmd, rollback);
+ break;
default:
/* Passthrough everything for now */
break;
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 09/13] KVM: arm64: Trap & emulate the ITS MAPC command
2026-08-07 16:43 ` [PATCH v2 09/13] KVM: arm64: Trap & emulate the ITS MAPC command Sebastian Ene
@ 2026-08-07 17:04 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:04 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Oliver Upton, Marc Zyngier, kvmarm
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Double-fetch of host-shared memory allows bypassing EL2 physical page ownership verification.
- [Critical] `process_its_mapd` lacks alignment validation for `itt_addr`, allowing the physical ITS to access host-controlled memory.
- [Critical] `check_table_update` lacks alignment validation for the L2 table base address, leading to a desynchronization between donated pages and hardware ITS accesses.
- [High] `process_its_mapc` fails to validate the `RDBase` (Target Address) field in the MAPC command, allowing the host to corrupt memory.
- [High] Rollback logic in `check_table_update` is unreachable, causing leaked page donations on ITS command submission failure.
- [High] `process_its_mapc` unconditionally checks the Collection table, breaking ITS command processing if the hardware implements internal collections.
- [High] Updates to the shadow L1 table in `check_table_update` are not flushed to the Point of Coherency.
--
--- Patch [9]: [PATCH v2 09/13] KVM: arm64: Trap & emulate the ITS MAPC command ---
commit 6bcf78893239baf62c5e67a46e844a52707cefb5
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Trap & emulate the ITS MAPC command
This patch parses the MAPC command to verify if any updates to the shadow
collection table are needed. It adds the process_its_mapc handler and wires
it into the main process_cmd switch statement.
> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> index 071a08d3602d7..5629e2a070df5 100644
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> @@ -332,6 +332,13 @@ static int process_its_mapd(struct its_priv_state *its, struct its_cmd_block *cm
> return track_pfn(its, device_id, itt_pfn, num_pages, remove);
> }
>
> +static int process_its_mapc(struct its_priv_state *its, struct its_cmd_block *cmd, bool rollback)
> +{
> + u32 icid = cmd->raw_cmd[2] & GENMASK(15, 0);
> +
[Severity: High]
Does this handler need to extract and validate the Target Address from
cmd->raw_cmd[2]?
If the physical ITS uses Physical Target Addresses (GITS_TYPER.PTA == 1), the
RDBase field in the MAPC command specifies the physical address of the target
Redistributor.
Since this field appears to be ignored here, could a malicious host provide an
arbitrary hypervisor or guest physical address? When the ITS hardware
processes an interrupt for this collection, it might perform a write to the
unvalidated physical address, bypassing SMMU stage-2 protections and
corrupting isolated memory.
> + return check_table_update(its, icid, GITS_BASER_TYPE_COLLECTION, rollback);
> +}
[Severity: High]
Does this code break ITS command processing if the hardware implements
internal collections?
According to the GICv3 architecture, if the ITS supports internal hardware
collections (GITS_TYPER.HCC > 0), the GITS_BASER register for the Collection
table is optional. If not implemented, get_table_from_snapshot() in
check_table_update() would legitimately return NULL, which causes an
-EINVAL return.
This error would propagate to cwriter_write(), which immediately returns
without advancing its->cmd_offset. The MAPC command would be permanently
rejected and the host ITS queue could stall.
[Severity: Critical]
Is there a double-fetch risk when reading from the host-shared snapshot table
in check_table_update() called here?
Looking at check_table_update(), EL2 reads the host-controlled L1 table entry
via a bare pointer dereference:
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
new_entry = snapshot_table[new_entry_index];
prev_entry = original_table[new_entry_index];
...
}
Since READ_ONCE() is not used, could the compiler satisfy multiple uses of
new_entry by reloading it from host memory? If the compiler emits multiple
load instructions, a host could race to overwrite the entry after it has been
verified and the PFN donated, but before it is written to the hypervisor's
original_table.
This could allow the host to install an unverified PFN into the hardware ITS,
potentially leading to arbitrary memory corruption by the ITS.
[Severity: High]
Can the rollback logic in check_table_update() actually be reached?
When an ITS command fails to submit to hardware, KVM attempts to revert the
state by calling process_cmd() with rollback=true, which calls into
check_table_update().
However, during the initial forward pass, original_table[new_entry_index]
was already updated to match the VALID bit of new_entry.
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
if (!((new_entry ^ prev_entry) & GITS_BASER_VALID))
return 0;
if (rollback)
new_entry = new_entry ^ GITS_BASER_VALID;
...
}
During rollback, wouldn't the early return condition evaluate to true
since the VALID bits now match? The function would return 0 immediately,
and the intended rollback logic is never reached, permanently leaving the
Level 2 table page donated to the hypervisor.
[Severity: Critical]
Does check_table_update() lack alignment validation for the L2 table base
address?
When the hypervisor donates the host's L2 table memory, it derives the PFN
using hyp_phys_to_pfn(new_entry & PHYS_MASK), but does not appear to validate
its alignment against the table page size (table->psz):
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
if (new_entry & GITS_BASER_VALID)
ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK),
table->psz >> PAGE_SHIFT);
...
}
If the host provides an unaligned address, KVM would donate pages starting
exactly at the unaligned PFN. However, the physical ITS hardware masks the
address to its strictly aligned boundary.
Could the ITS hardware then access pages preceding the KVM-donated range?
Since these preceding pages would still be owned by host EL1, this could allow
the host to directly read or modify active L2 table entries and bypass pKVM
isolation.
[Severity: High]
Do the updates to the shadow L1 table in check_table_update() need to be
flushed to the Point of Coherency?
When the new L1 entry is written to the shadow table:
arch/arm64/kvm/hyp/nvhe/its_emulate.c:check_table_update() {
...
original_table[new_entry_index] = new_entry;
return 0;
}
This update does not seem to be flushed. If the physical ITS is not cache
coherent with the CPUs (indicated by its->needs_flush), the hardware ITS
might fetch the stale L1 entry directly from RAM.
Does this require a gic_flush_dcache_to_poc() on the modified shadow table
entry to ensure the hardware ITS sees the updated pointer to the L2 table?
> +
> static int process_cmd(struct its_priv_state *its, struct its_cmd_block *cmd,
> bool rollback)
> {
> @@ -342,6 +349,10 @@ static int process_cmd(struct its_priv_state *its, struct its_cmd_block *cmd,
> case GITS_CMD_MAPD:
> ret = process_its_mapd(its, cmd, rollback);
> break;
[Severity: Critical]
Does process_its_mapd() validate the architectural alignment of itt_addr?
Looking at the implementation of process_its_mapd() called here:
arch/arm64/kvm/hyp/nvhe/its_emulate.c:process_its_mapd() {
...
phys_addr_t itt_addr = cmd->raw_cmd[2] & GENMASK(51, 8);
...
itt_pfn = hyp_phys_to_pfn(itt_addr);
num_pages = get_num_itt_pages(its, size);
...
}
itt_addr is converted to a PFN by shifting right by PAGE_SHIFT. The GIC
architecture only requires itt_addr to be 256-byte aligned.
If a host provides a sub-page aligned itt_addr near a page boundary, KVM
calculates num_pages and tracks those pages starting from the truncated PFN.
However, the physical ITS will access size bytes starting at the unaligned
itt_addr, potentially spilling into the next page.
If this next page is not pinned and remains owned by host EL1, could the host
donate it to a guest, allowing either to read or modify active ITEs and bypass
memory isolation?
> +
> + case GITS_CMD_MAPC:
> + ret = process_its_mapc(its, cmd, rollback);
> + break;
> default:
> /* Passthrough everything for now */
> break;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=9
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 10/13] KVM: arm64: Restrict host updates to GITS_CTLR
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (8 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 09/13] KVM: arm64: Trap & emulate the ITS MAPC command Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:02 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 11/13] KVM: arm64: Prevent the host from specifying a different command queue Sebastian Ene
` (2 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Prevent unpredictable hardware behavior when the host tries to enable
the ITS while it is not in quiescent state.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
index 5629e2a070df..b9b71aa18d48 100644
--- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -403,8 +403,35 @@ static void cwriter_read(struct pkvm_protected_reg *region, u64 offset, u64 *rea
*read = readq_relaxed(its->base + GITS_CWRITER);
}
+static void ctlr_read(struct pkvm_protected_reg *region, u64 offset, u64 *read)
+{
+ struct its_priv_state *its = region->priv;
+ *read = readl_relaxed(its->base + GITS_CTLR);
+}
+
+static void ctlr_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
+{
+ struct its_priv_state *its = region->priv;
+ bool is_quiescent, is_enabled;
+ u32 ctlr;
+
+ ctlr = readl_relaxed(its->base + GITS_CTLR);
+ is_quiescent = !!(ctlr & GITS_CTLR_QUIESCENT);
+ is_enabled = !!(ctlr & GITS_CTLR_ENABLE);
+
+ /*
+ * If it's disabled and not in quiescent state and it tries to enable
+ * it, bail out.
+ */
+ if (!is_enabled && (value & GITS_CTLR_ENABLE) && !is_quiescent)
+ return;
+
+ writel_relaxed(value, its->base + GITS_CTLR);
+}
+
static struct its_handler its_handlers[] = {
ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
+ ITS_HANDLER(GITS_CTLR, sizeof(u32), ctlr_write, ctlr_read),
{},
};
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 10/13] KVM: arm64: Restrict host updates to GITS_CTLR
2026-08-07 16:43 ` [PATCH v2 10/13] KVM: arm64: Restrict host updates to GITS_CTLR Sebastian Ene
@ 2026-08-07 17:02 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:02 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Oliver Upton, Marc Zyngier, kvmarm
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Missing interception of GITS_CBASER and GITS_BASER<n> allows the host to bypass ITS emulation and execute unvalidated commands.
- [High] Concurrent modification of the host's `snapshot_table` during ITS command rollback leads to arbitrary memory donation attempts.
- [High] Widespread use of `WARN_ON` for host-reachable error conditions in ITS emulation panics the hypervisor.
- [High] Incorrect sub-word MMIO handling in `ctlr_write` and `ctlr_read` leads to register corruption.
- [Medium] 64-bit accesses to GITS_CTLR are incorrectly dropped, causing reads to return uninitialized memory and writes to be silently ignored.
- [Medium] The handler fails to mask reserved (RES0) bits from the host-provided `value` before writing to the physical `GITS_CTLR` register.
--
--- Patch [10]: [PATCH v2 10/13] KVM: arm64: Restrict host updates to GITS_CTLR ---
commit 8547f0737e8d44ccf52c0d09c7367b0079543481
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Restrict host updates to GITS_CTLR
Prevent unpredictable hardware behavior when the host tries to enable
the ITS while it is not in quiescent state.
This commit adds emulation for the GITS_CTLR register to restrict the host
from enabling the ITS while it is not in a quiescent state. It adds
ctlr_read and ctlr_write handlers and registers them in the its_handlers
array.
> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> index 5629e2a070df5..b9b71aa18d48b 100644
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> @@ -403,8 +403,35 @@ static void cwriter_read(struct pkvm_protected_reg *region, u64 offset, u64 *rea
> *read = readq_relaxed(its->base + GITS_CWRITER);
> }
>
> +static void ctlr_read(struct pkvm_protected_reg *region, u64 offset, u64 *read)
> +{
> + struct its_priv_state *its = region->priv;
> + *read = readl_relaxed(its->base + GITS_CTLR);
> +}
[Severity: High]
Does this read handler account for the access offset? When the host performs
a partial sub-word read at a non-zero offset, it appears we return the full
32-bit register value instead of the requested bytes.
> +
> +static void ctlr_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
> +{
> + struct its_priv_state *its = region->priv;
> + bool is_quiescent, is_enabled;
> + u32 ctlr;
> +
> + ctlr = readl_relaxed(its->base + GITS_CTLR);
> + is_quiescent = !!(ctlr & GITS_CTLR_QUIESCENT);
> + is_enabled = !!(ctlr & GITS_CTLR_ENABLE);
> +
> + /*
> + * If it's disabled and not in quiescent state and it tries to enable
> + * it, bail out.
> + */
> + if (!is_enabled && (value & GITS_CTLR_ENABLE) && !is_quiescent)
> + return;
> +
> + writel_relaxed(value, its->base + GITS_CTLR);
[Severity: High]
Can a sub-word write corrupt the register state here? If the host performs a
partial 8-bit or 16-bit write at a non-zero offset, this writes the provided
value directly to offset 0 as a 32-bit word, which would zero-extend the value
and wipe out other bits in GITS_CTLR.
[Severity: Medium]
Should we mask out reserved (RES0) bits from the host-provided value before
writing it? The host could write non-zero values to RES0 bits, which the
architecture states can lead to unpredictable hardware behavior.
> +}
> +
> static struct its_handler its_handlers[] = {
> ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
> + ITS_HANDLER(GITS_CTLR, sizeof(u32), ctlr_write, ctlr_read),
> {},
> };
[Severity: Critical]
Does this registry need to intercept GITS_CBASER and GITS_BASER<n>? Because
they are omitted, writes to these registers fall through and are forwarded
directly to hardware. This could allow the host to bypass the ITS emulation
completely by configuring the hardware to read from host-controlled memory,
leading to a potential DMA attack against EL2 memory.
I also noticed a few other issues in the ITS emulation code that might need
attention.
[Severity: High]
In check_table_update(), could there be a TOCTOU race with the host?
snapshot_table = kern_hyp_va(table->base_snapshot);
...
new_entry = snapshot_table[new_entry_index];
...
if (rollback)
new_entry = new_entry ^ GITS_BASER_VALID;
if (new_entry & GITS_BASER_VALID)
ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK),
The snapshot_table is in host-shared memory. If the host concurrently
modifies the entry between the initial check and the rollback, couldn't it
force the rollback path to donate an arbitrary PFN to the hypervisor?
[Severity: High]
Can an untrusted host trigger the WARN_ON() in cwriter_write()?
if (submit_single_cmd(its, retry)) {
WARN_ON(process_cmd(its, &cmd, /* rollback */ true));
return;
}
If the host intentionally stalls the ITS to force a command timeout, this
rollback path is executed. If the rollback fails, the WARN_ON() will panic
the hypervisor in EL2 nVHE, bringing down the host and all protected VMs.
[Severity: Medium]
Are 64-bit accesses to GITS_CTLR incorrectly dropped by pkvm_its_emulate_handler()?
if (reg_handler->access_size < reg_size)
return;
Since GITS_CTLR is configured with an access_size of 4, a valid 64-bit access
from the host (which is permitted by the GICv3 architecture) would be silently
ignored, potentially returning uninitialized memory on reads.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=10
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 11/13] KVM: arm64: Prevent the host from specifying a different command queue
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (9 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 10/13] KVM: arm64: Restrict host updates to GITS_CTLR Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:28 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 12/13] KVM: arm64: Prevent the host from programming new GITS_BASER tables Sebastian Ene
2026-08-07 16:43 ` [PATCH v2 13/13] KVM: arm64: Implement HVC interface for ITS emulation setup Sebastian Ene
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Don't let a malicious host re-program the command queue register with a
different address and size to bypass the commands sanitization.
Prevent unpredictable hardware behavior and restrict updates to the
GITS_CBASER while the ITS is enabled or not in a quiescent state.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 32 +++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
index b9b71aa18d48..97cfa31d90d1 100644
--- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -429,9 +429,41 @@ static void ctlr_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
writel_relaxed(value, its->base + GITS_CTLR);
}
+static void cbaser_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
+{
+ struct its_priv_state *its = region->priv;
+ int num_pages;
+ u64 ctlr;
+
+ ctlr = readl_relaxed(its->base + GITS_CTLR);
+ if ((ctlr & GITS_CTLR_ENABLE) || !(ctlr & GITS_CTLR_QUIESCENT))
+ return;
+
+ num_pages = its->host_state->cmdq_len / SZ_4K;
+
+ /* Don't let the host program a different command queue */
+ value &= ~(GENMASK(7, 0) | GENMASK_ULL(51, 12));
+ value |= (num_pages - 1) & GENMASK(7, 0);
+ value |= __hyp_pa(its->cmd_original) & GENMASK_ULL(51, 12);
+ its->needs_flush = (value & GITS_CBASER_SHAREABILITY_MASK) != GITS_CBASER_InnerShareable;
+
+ writeq_relaxed(value, its->base + GITS_CBASER);
+
+ /* Restart the CMDQ to read from 0 */
+ its->cmd_offset = 0;
+ writeq_relaxed(0, its->base + GITS_CWRITER);
+}
+
+static void cbaser_read(struct pkvm_protected_reg *region, u64 offset, u64 *read)
+{
+ struct its_priv_state *its = region->priv;
+ *read = readq_relaxed(its->base + GITS_CBASER);
+}
+
static struct its_handler its_handlers[] = {
ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
ITS_HANDLER(GITS_CTLR, sizeof(u32), ctlr_write, ctlr_read),
+ ITS_HANDLER(GITS_CBASER, sizeof(u64), cbaser_write, cbaser_read),
{},
};
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 11/13] KVM: arm64: Prevent the host from specifying a different command queue
2026-08-07 16:43 ` [PATCH v2 11/13] KVM: arm64: Prevent the host from specifying a different command queue Sebastian Ene
@ 2026-08-07 17:28 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:28 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Marc Zyngier, kvmarm, Oliver Upton
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Critical] Malicious host can bypass ITS command sanitization by setting Page_Size to 64KB in GITS_CBASER, forcing the hardware to read unsanitized commands from host memory.
- [Critical] A malicious host can bypass ITS command sanitization by programming GITS_CBASER with Non-cacheable attributes, exploiting a flawed `needs_flush` calculation.
Pre-existing issues:
- [Critical] Missing interception of GITS_BASER registers allows a malicious host to direct hardware ITS tables to arbitrary physical memory, breaking isolation.
- [High] Infinite loop and out-of-bounds memory unsharing in the error handling path of `pkvm_setup_its_shadow_cmdq` due to an unsigned integer underflow.
--
--- Patch [11]: [PATCH v2 11/13] KVM: arm64: Prevent the host from specifying a different command queue ---
commit 3cac1c2e6af2a0edec0047b75fa76ac659338bc2
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Prevent the host from specifying a different command queue
This patch secures the command queue register (GITS_CBASER) from being
re-programmed by a malicious host to bypass command sanitization. It
restricts updates to the GITS_CBASER register while the ITS is enabled
or not in a quiescent state.
> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> index b9b71aa18d48b..97cfa31d90d11 100644
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> @@ -429,9 +429,41 @@ static void ctlr_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
> writel_relaxed(value, its->base + GITS_CTLR);
> }
>
> +static void cbaser_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
> +{
> + struct its_priv_state *its = region->priv;
> + int num_pages;
> + u64 ctlr;
> +
> + ctlr = readl_relaxed(its->base + GITS_CTLR);
> + if ((ctlr & GITS_CTLR_ENABLE) || !(ctlr & GITS_CTLR_QUIESCENT))
> + return;
> +
> + num_pages = its->host_state->cmdq_len / SZ_4K;
> +
> + /* Don't let the host program a different command queue */
> + value &= ~(GENMASK(7, 0) | GENMASK_ULL(51, 12));
> + value |= (num_pages - 1) & GENMASK(7, 0);
> + value |= __hyp_pa(its->cmd_original) & GENMASK_ULL(51, 12);
[Severity: Critical]
Does this masking logic allow the host to modify the Page_Size field?
By not clearing bits 9:8 (Page_Size) from the host's GITS_CBASER value,
could a malicious host pass a Page_Size of 64KB (0b10)?
According to the GICv3 architecture, when a 64KB page size is used, the
hardware treats bits 15:12 of the physical address as RES0, effectively
rounding down the base address to a 64KB boundary.
If the host donates a command queue buffer (cmd_original) that is 4KB-aligned
but not 64KB-aligned, the address truncation by the hardware would cause it
to fetch commands from an adjacent memory region still under host control,
bypassing the hypervisor's command sanitization entirely.
> + its->needs_flush = (value & GITS_CBASER_SHAREABILITY_MASK) != GITS_CBASER_InnerShareable;
[Severity: Critical]
Does this calculation safely account for cacheability attributes?
Since this only checks the Shareability mask (bits 11:10) of the raw host
value, could a malicious host request an Inner Cacheability of Non-cacheable
(0b000 in bits 61:59) while maintaining an InnerShareable state?
This would cause needs_flush to evaluate to false. If the hardware ITS
operates in Non-cacheable mode, it reads physical memory directly without
waiting for the hypervisor to flush the data cache.
A host could pre-seed the physical memory with malicious commands before
donating the page, forcing the hardware to execute those instead of the
sanitized commands.
> +
> + writeq_relaxed(value, its->base + GITS_CBASER);
> +
> + /* Restart the CMDQ to read from 0 */
> + its->cmd_offset = 0;
> + writeq_relaxed(0, its->base + GITS_CWRITER);
> +}
[ ... ]
> static struct its_handler its_handlers[] = {
> ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
> ITS_HANDLER(GITS_CTLR, sizeof(u32), ctlr_write, ctlr_read),
> + ITS_HANDLER(GITS_CBASER, sizeof(u64), cbaser_write, cbaser_read),
> {},
> };
[Severity: Critical]
This is a pre-existing issue, but does the omission of GITS_BASER from the
its_handlers array allow the host to bypass isolation?
Because GITS_BASER is missing here, any host writes to these registers fall
through to its_emulate_forward_req() and are directly written to the hardware
ITS.
Could a malicious host write an arbitrary physical address (such as hypervisor
private memory or guest memory) into GITS_BASER?
When the host subsequently issues a MAPD command, the hardware ITS would
execute it using the maliciously programmed address, allowing the host to
overwrite arbitrary physical memory.
[Severity: High]
This isn't a bug introduced by this patch, but is there an unsigned integer
underflow in the error handling path of pkvm_setup_its_shadow_cmdq()?
Looking at unshare_cmd_host:
arch/arm64/kvm/hyp/nvhe/its_emulate.c:pkvm_setup_its_shadow_cmdq() {
...
unshare_cmd_host:
if (i == 0)
return ret;
for (i = i - 1; i >= 0; i--)
__pkvm_host_unshare_hyp(start_pfn + i);
return ret;
...
}
Since the loop variable i is declared as an unsigned 64-bit integer (u64 i),
the condition i >= 0 is always true.
When i reaches 0, the i-- decrement underflows it to 0xFFFFFFFFFFFFFFFF,
which would cause an infinite loop sequentially calling
__pkvm_host_unshare_hyp() with invalid out-of-bounds page frame numbers.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=11
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 12/13] KVM: arm64: Prevent the host from programming new GITS_BASER tables
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (10 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 11/13] KVM: arm64: Prevent the host from specifying a different command queue Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:11 ` sashiko-bot
2026-08-07 16:43 ` [PATCH v2 13/13] KVM: arm64: Implement HVC interface for ITS emulation setup Sebastian Ene
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Don't allow the host to change the layout of the tables or to modify the
address programmed in the GITS_BASER registers to point to new tables
and bypass the sanitization.
Prevent the host from updating the ITS tables while the ITS is enabled
and the tables are set to prevent undefined behavior.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 44 +++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
index 97cfa31d90d1..82dc60dcde68 100644
--- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -42,18 +42,23 @@ void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool
struct its_handler {
u64 offset;
u8 access_size;
+ u8 num_registers;
void (*write)(struct pkvm_protected_reg *region, u64 offset, u64 value);
void (*read)(struct pkvm_protected_reg *region, u64 offset, u64 *read);
};
-#define ITS_HANDLER(off, sz, write_cb, read_cb) \
+#define ITS_HANDLER_REG_PAIR(off, sz, registers, write_cb, read_cb) \
{ \
.offset = (off), \
.access_size = (sz), \
+ .num_registers = (registers), \
.write = (write_cb), \
.read = (read_cb), \
}
+#define ITS_HANDLER(off, sz, write_cb, read_cb) \
+ ITS_HANDLER_REG_PAIR(off, sz, 1, write_cb, read_cb)
+
struct dte_entry {
u32 device_id;
u64 itt_pfn;
@@ -460,10 +465,42 @@ static void cbaser_read(struct pkvm_protected_reg *region, u64 offset, u64 *read
*read = readq_relaxed(its->base + GITS_CBASER);
}
+static void baser_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
+{
+ struct its_priv_state *its = region->priv;
+ u32 ctlr = readl_relaxed(its->base + GITS_CTLR);
+ int baser_idx;
+ u64 baser;
+
+ if ((ctlr & GITS_CTLR_ENABLE) || !(ctlr & GITS_CTLR_QUIESCENT))
+ return;
+
+ baser_idx = (offset - GITS_BASER) >> 3;
+ baser = its->host_state->tables[baser_idx].val;
+
+ /* Prevent if it tries to change from direct layout to indirect layout */
+ if ((value & GITS_BASER_INDIRECT) != (baser & GITS_BASER_INDIRECT))
+ return;
+
+ /* Don't allow the host to point to new tables or new attributes */
+ value &= ~(GENMASK_ULL(47, 12) | GENMASK_ULL(9, 0));
+ value |= (baser & GENMASK_ULL(47, 12)) | (baser & GENMASK_ULL(9, 0));
+
+ writeq_relaxed(value, its->base + offset);
+}
+
+static void baser_read(struct pkvm_protected_reg *region, u64 offset, u64 *read)
+{
+ struct its_priv_state *its = region->priv;
+ *read = readq_relaxed(its->base + offset);
+}
+
static struct its_handler its_handlers[] = {
ITS_HANDLER(GITS_CWRITER, sizeof(u64), cwriter_write, cwriter_read),
ITS_HANDLER(GITS_CTLR, sizeof(u32), ctlr_write, ctlr_read),
ITS_HANDLER(GITS_CBASER, sizeof(u64), cbaser_write, cbaser_read),
+
+ ITS_HANDLER_REG_PAIR(GITS_BASER, sizeof(u64), 8, baser_write, baser_read),
{},
};
@@ -472,13 +509,14 @@ void pkvm_its_emulate_handler(struct pkvm_protected_reg *region, u64 offset, boo
{
struct its_priv_state *priv = region->priv;
struct its_handler *reg_handler;
+ u64 end;
if (!priv || !IS_ALIGNED(offset, reg_size))
return;
for (reg_handler = its_handlers; reg_handler->access_size; reg_handler++) {
- if (reg_handler->offset > offset ||
- reg_handler->offset + reg_handler->access_size <= offset)
+ end = reg_handler->offset + reg_handler->access_size * reg_handler->num_registers;
+ if (reg_handler->offset > offset || end <= offset)
continue;
if (reg_handler->access_size < reg_size)
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 12/13] KVM: arm64: Prevent the host from programming new GITS_BASER tables
2026-08-07 16:43 ` [PATCH v2 12/13] KVM: arm64: Prevent the host from programming new GITS_BASER tables Sebastian Ene
@ 2026-08-07 17:11 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:11 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Marc Zyngier, kvmarm, Oliver Upton
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Sub-word (32-bit) MMIO writes to 64-bit ITS registers cause hypervisor crashes (Alignment Fault) and silent state corruption.
- [Critical] Incomplete attribute masking in baser_write and cbaser_write allows the host to alter ITS table Cacheability and Shareability.
- [Critical] The host can activate unvalidated ITS tables by toggling the GITS_BASER Valid bit (bit 63).
- [High] Incomplete physical address masking in baser_write on 52-bit PA systems (FEAT_LPA) allows physical address hijacking.
--
--- Patch [12]: [PATCH v2 12/13] KVM: arm64: Prevent the host from programming new GITS_BASER tables ---
commit ceb5f9bba1bc25bf01683842fe6461183f94c200
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Prevent the host from programming new GITS_BASER tables
Don't allow the host to change the layout of the tables or to modify the
address programmed in the GITS_BASER registers to point to new tables
and bypass the sanitization.
Prevent the host from updating the ITS tables while the ITS is enabled
and the tables are set to prevent undefined behavior.
> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> --- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
[ ... ]
> @@ -460,10 +465,42 @@ static void cbaser_read(struct pkvm_protected_reg *region, u64 offset, u64 *read
> *read = readq_relaxed(its->base + GITS_CBASER);
> }
>
> +static void baser_write(struct pkvm_protected_reg *region, u64 offset, u64 value)
> +{
> + struct its_priv_state *its = region->priv;
> + u32 ctlr = readl_relaxed(its->base + GITS_CTLR);
> + int baser_idx;
> + u64 baser;
> +
> + if ((ctlr & GITS_CTLR_ENABLE) || !(ctlr & GITS_CTLR_QUIESCENT))
> + return;
> +
> + baser_idx = (offset - GITS_BASER) >> 3;
> + baser = its->host_state->tables[baser_idx].val;
> +
> + /* Prevent if it tries to change from direct layout to indirect layout */
> + if ((value & GITS_BASER_INDIRECT) != (baser & GITS_BASER_INDIRECT))
> + return;
> +
> + /* Don't allow the host to point to new tables or new attributes */
> + value &= ~(GENMASK_ULL(47, 12) | GENMASK_ULL(9, 0));
> + value |= (baser & GENMASK_ULL(47, 12)) | (baser & GENMASK_ULL(9, 0));
[Severity: Critical]
Does this mask correctly restrict the cacheability and shareability attributes?
The mask only clears bits 47:12 and 9:0, which leaves Cacheability
(bits 61:53) and Shareability (bits 11:10) unmasked. This allows the host
to force the ITS hardware to bypass caches while EL2 accesses the same
memory via Cacheable mappings, potentially compromising memory coherency
and EL2 isolation.
The cbaser_write() function similarly fails to mask Cacheability and
Shareability attributes for the command queue base register.
[Severity: High]
Can this mask allow the host to hijack the physical address on 52-bit PA
systems?
For systems with FEAT_LPA, PA[51:48] are stored in bits 51:48 of GITS_BASER.
Since the mask only covers up to bit 47, the host can supply arbitrary
values in bits 51:48. This would redirect the ITS table physical address
by multiples of 256TB to untracked memory, bypassing pKVM stage-2 tracking.
[Severity: Critical]
Could this allow the host to activate unvalidated ITS tables by toggling the
Valid bit (bit 63)?
Because bit 63 is unmasked, a malicious host could boot with a fabricated PA
and VALID = 0, skipping KVM's validation and memory donation. If the host
subsequently writes to GITS_BASER with VALID = 1, this mask restores the
fabricated PA and writes VALID = 1 to the hardware, granting the ITS
hardware DMA access to arbitrary physical addresses.
> +
> + writeq_relaxed(value, its->base + offset);
[Severity: Critical]
Could a 32-bit MMIO write to a 64-bit ITS register cause a hypervisor crash
or state corruption here?
Looking at pkvm_its_emulate_handler() in this file, it permits sub-word
accesses to 64-bit registers:
if (reg_handler->access_size < reg_size)
return;
If a host issues a 32-bit write to an unaligned offset (e.g.,
GITS_BASER + 4), this writeq_relaxed() will perform a 64-bit device memory
access at an unaligned address, triggering an Alignment Fault at EL2. Even
if aligned, writing a 32-bit payload via a 64-bit writeq_relaxed() will
overwrite the upper 32 bits of the hardware register with zeros, wiping
critical fields.
Should the payload be adapted for a read-modify-write, or alignment
enforced? The cbaser_write() function appears to share this same behavior.
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=12
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 13/13] KVM: arm64: Implement HVC interface for ITS emulation setup
2026-08-07 16:43 [PATCH v2 00/13] KVM: ITS hardening for pKVM Sebastian Ene
` (11 preceding siblings ...)
2026-08-07 16:43 ` [PATCH v2 12/13] KVM: arm64: Prevent the host from programming new GITS_BASER tables Sebastian Ene
@ 2026-08-07 16:43 ` Sebastian Ene
2026-08-07 17:17 ` sashiko-bot
12 siblings, 1 reply; 27+ messages in thread
From: Sebastian Ene @ 2026-08-07 16:43 UTC (permalink / raw)
To: catalin.marinas, fuad.tabba, joey.gouly, mark.rutland, maz,
oupton, rananta, Sascha.Bischoff, suzuki.poulose, will
Cc: kvmarm, android-kvm, bgrzesik, linux-arm-kernel, linux-kernel,
nathan, perlarsen, sebastianene, seiden, smostafa, tglx,
vdonnefort, vladimir.murzin, yuzenghui, zenghui.yu
Introduce a new HVC to allow the host to trigger the ITS emulation
setup. Use the introduced API in the GIC ITS driver to call the driver
to lock the ITS before pKVM finalize and to prepare for emulation setup.
On the return path from the pKVM finalize, call into the driver to
release the ITS locks which performs a switch in the driver to use a
different command queue and a different set of level-1 indirect tables.
Allocate memory that will be used by the emulation to track the internal
state and send the snapshot state from the driver.
Replace the initial "trap-and-forward" MMIO handler with a full-featured
emulation handler.
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/include/asm/kvm_asm.h | 1 +
arch/arm64/include/asm/kvm_pkvm.h | 4 ++--
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 16 ++++++++++++++++
arch/arm64/kvm/hyp/nvhe/its_emulate.c | 4 ++--
arch/arm64/kvm/pkvm.c | 26 ++++++++++++++++++++++++--
5 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 043495f7fc78..fcb2871b8a86 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -114,6 +114,7 @@ enum __kvm_host_smccc_func {
__KVM_HOST_SMCCC_FUNC___pkvm_vcpu_load,
__KVM_HOST_SMCCC_FUNC___pkvm_vcpu_put,
__KVM_HOST_SMCCC_FUNC___pkvm_tlb_flush_vmid,
+ __KVM_HOST_SMCCC_FUNC___pkvm_its_emulate_setup,
MARKER(__KVM_HOST_SMCCC_FUNC_MAX)
};
diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
index 78597210a53c..cc89e2bde468 100644
--- a/arch/arm64/include/asm/kvm_pkvm.h
+++ b/arch/arm64/include/asm/kvm_pkvm.h
@@ -32,8 +32,8 @@ struct pkvm_protected_reg {
extern struct pkvm_protected_reg kvm_nvhe_sym(pkvm_protected_regs)[];
extern unsigned int kvm_nvhe_sym(num_protected_reg);
-extern void kvm_nvhe_sym(its_emulate_forward_req)(struct pkvm_protected_reg *region, u64 offset,
- bool write, u64 *reg, u8 reg_size);
+extern void kvm_nvhe_sym(pkvm_its_emulate_handler)(struct pkvm_protected_reg *region, u64 offset,
+ bool write, u64 *reg, u8 reg_size);
int pkvm_init_host_vm(struct kvm *kvm, unsigned long type);
int pkvm_create_hyp_vm(struct kvm *kvm);
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index d3df96ed8ba4..ad57b2076eee 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -16,6 +16,7 @@
#include <asm/kvm_mmu.h>
#include <nvhe/ffa.h>
+#include <nvhe/its_emulate.h>
#include <nvhe/mem_protect.h>
#include <nvhe/mm.h>
#include <nvhe/pkvm.h>
@@ -705,6 +706,20 @@ static void handle___vgic_v5_restore_vmcr_apr(struct kvm_cpu_context *host_ctxt)
__vgic_v5_restore_vmcr_apr(kern_hyp_va(cpu_if));
}
+static void handle___pkvm_its_emulate_setup(struct kvm_cpu_context *host_ctxt)
+{
+ DECLARE_REG(phys_addr_t, dev_addr, host_ctxt, 1);
+ DECLARE_REG(struct its_host_state *, host_state, host_ctxt, 2);
+ DECLARE_REG(void *, priv_state, host_ctxt, 3);
+ DECLARE_REG(size_t, priv_state_num_pages, host_ctxt, 4);
+
+ if (!is_protected_kvm_enabled())
+ return;
+
+ cpu_reg(host_ctxt, 1) = pkvm_its_emulate_setup(dev_addr, host_state, priv_state,
+ priv_state_num_pages);
+}
+
typedef void (*hcall_t)(struct kvm_cpu_context *);
#define HANDLE_FUNC(x) [__KVM_HOST_SMCCC_FUNC_##x] = (hcall_t)handle_##x
@@ -762,6 +777,7 @@ static const hcall_t host_hcall[] = {
HANDLE_FUNC(__pkvm_vcpu_load),
HANDLE_FUNC(__pkvm_vcpu_put),
HANDLE_FUNC(__pkvm_tlb_flush_vmid),
+ HANDLE_FUNC(__pkvm_its_emulate_setup),
};
static void handle_host_hcall(struct kvm_cpu_context *host_ctxt)
diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
index 82dc60dcde68..8c8acaee4d2b 100644
--- a/arch/arm64/kvm/hyp/nvhe/its_emulate.c
+++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
@@ -6,8 +6,8 @@
#include <linux/irqchip/arm-gic-v3.h>
-void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool write, u64 *reg,
- u8 reg_size)
+static void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool write,
+ u64 *reg, u8 reg_size)
{
void __iomem *addr = __hyp_va(PFN_PHYS(region->pfn) + offset);
diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
index 4bfffbedac4c..a9ceb9ffe6a4 100644
--- a/arch/arm64/kvm/pkvm.c
+++ b/arch/arm64/kvm/pkvm.c
@@ -71,7 +71,7 @@ static int __init register_its_emulated_region(void)
*/
kvm_nvhe_sym(pkvm_protected_regs)[i].pfn = PHYS_PFN(res.start);
kvm_nvhe_sym(pkvm_protected_regs)[i].cb =
- lm_alias(&kvm_nvhe_sym(its_emulate_forward_req));
+ lm_alias(&kvm_nvhe_sym(pkvm_its_emulate_handler));
kvm_nvhe_sym(pkvm_protected_regs)[i].nr_pages =
PFN_DOWN(min_t(u64, resource_size(&res), PAGE_ALIGN_DOWN(GITS_TRANSLATER)));
@@ -312,8 +312,28 @@ static void __init _kvm_host_prot_finalize(void *arg)
WRITE_ONCE(*err, -EINVAL);
}
+#define ITS_PAGES (2UL)
+
+static int pkvm_init_its_emulation(phys_addr_t dev_addr, struct its_host_state *host)
+{
+ size_t priv_state_sz = ITS_PAGES << PAGE_SHIFT;
+ void *priv_state;
+ int ret;
+
+ priv_state = alloc_pages_exact(priv_state_sz, GFP_ATOMIC);
+ if (!priv_state)
+ return -ENOMEM;
+
+ ret = kvm_call_hyp_nvhe(__pkvm_its_emulate_setup, dev_addr, host, priv_state, ITS_PAGES);
+ if (ret)
+ free_pages_exact(priv_state, priv_state_sz);
+
+ return ret;
+}
+
static int __init pkvm_drop_host_privileges(void)
{
+ unsigned long its_flags;
int ret = 0;
/*
@@ -321,8 +341,10 @@ static int __init pkvm_drop_host_privileges(void)
* once the host stage 2 is installed.
*/
static_branch_enable(&kvm_protected_mode_initialized);
+
+ its_emulate_acquire_locks(&its_flags);
on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
- return ret;
+ return its_emulate_release_locks(ret, &its_flags, pkvm_init_its_emulation);
}
static int __init finalize_pkvm(void)
--
2.55.0.654.g21b8a5bc05-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 13/13] KVM: arm64: Implement HVC interface for ITS emulation setup
2026-08-07 16:43 ` [PATCH v2 13/13] KVM: arm64: Implement HVC interface for ITS emulation setup Sebastian Ene
@ 2026-08-07 17:17 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-07 17:17 UTC (permalink / raw)
To: Sebastian Ene; +Cc: Marc Zyngier, kvmarm, Oliver Upton
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The `psz` (page size) field read from the untrusted `host_state` is not validated, allowing a malicious host to bypass ownership transfer of Level 2 ITS tables.
- [High] Calling `on_each_cpu()` with local interrupts disabled causes a system-wide deadlock and triggers a kernel warning.
- [High] Allocating memory via `alloc_pages_exact(..., GFP_ATOMIC)` while holding a `raw_spin_lock` violates locking hierarchy and PREEMPT_RT constraints.
- [High] The error path in `pkvm_its_emulate_setup()` rolls back the `priv_state` donation using a hardcoded size of 1 page instead of `priv_num_pages`, leaking hypervisor memory.
- [High] An error rollback loop in `pkvm_setup_its_shadow_cmdq()` uses an unsigned 64-bit integer, resulting in an infinite loop.
--
--- Patch [13]: [PATCH v2 13/13] KVM: arm64: Implement HVC interface for ITS emulation setup ---
commit b5e04e3693da3938b3ebc14d5487e70eddc3c941
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Implement HVC interface for ITS emulation setup
This patch introduces a new HVC to allow the host to trigger the ITS emulation
setup. It uses the API in the GIC ITS driver to lock the ITS before pKVM
finalize, switches to a different command queue, and allocates memory to track
internal state. The initial MMIO handler is replaced with a full-featured
emulation handler.
> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
> index 5e80da18f3120..9cd562349f249 100644
> --- a/arch/arm64/kvm/pkvm.c
> +++ b/arch/arm64/kvm/pkvm.c
[ ... ]
> @@ -312,8 +312,28 @@ static void __init _kvm_host_prot_finalize(void *arg)
[ ... ]
> +static int pkvm_init_its_emulation(phys_addr_t dev_addr, struct its_host_state *host)
> +{
> + size_t priv_state_sz = ITS_PAGES << PAGE_SHIFT;
> + void *priv_state;
> + int ret;
> +
> + priv_state = alloc_pages_exact(priv_state_sz, GFP_ATOMIC);
[Severity: High]
Does this allocation violate locking hierarchy constraints?
This function is invoked as a callback from its_emulate_release_locks() in
drivers/irqchip/irq-gic-v3-its.c, which iterates over ITS nodes and calls this
while holding both its_lock and the per-node its->lock raw spinlocks.
On PREEMPT_RT kernels, internal page allocator spinlocks are sleeping
rt_mutexes. Furthermore, GFP_ATOMIC contains __GFP_KSWAPD_RECLAIM, which can
wake kswapd and acquire scheduler runqueue locks while holding a true raw
spinlock.
> + if (!priv_state)
> + return -ENOMEM;
[ ... ]
> static int __init pkvm_drop_host_privileges(void)
> {
> + unsigned long its_flags;
> int ret = 0;
>
> /*
> @@ -321,8 +341,10 @@ static int __init pkvm_drop_host_privileges(void)
> * once the host stage 2 is installed.
> */
> static_branch_enable(&kvm_protected_mode_initialized);
> +
> + its_emulate_acquire_locks(&its_flags);
> on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
[Severity: High]
Can this sequence cause a system-wide deadlock?
its_emulate_acquire_locks() explicitly disables local hardware interrupts via
raw_spin_lock_irqsave(). Immediately after, on_each_cpu() is called, which
relies on smp_call_function_many() to broadcast an IPI and synchronously wait
for completion.
Waiting for an IPI while local IRQs are disabled is an AB-BA deadlock
scenario.
> - return ret;
> + return its_emulate_release_locks(ret, &its_flags, pkvm_init_its_emulation);
> }
[ ... ]
For the items identified in arch/arm64/kvm/hyp/nvhe/its_emulate.c, the
following observations were made based on the underlying emulation setup:
arch/arm64/kvm/hyp/nvhe/its_emulate.c:pkvm_setup_its_shadow_cmdq() {
...
for (i = i - 1; i >= 0; i--)
__pkvm_host_unshare_hyp(start_pfn + i);
}
[Severity: High]
Can this error rollback trigger an infinite loop?
If the loop counter i is declared as an unsigned 64-bit integer, the condition
i >= 0 is universally true. When i wraps around to 0xFFFFFFFFFFFFFFFF, it will
continually execute out-of-bounds __pkvm_host_unshare_hyp() calls instead of
terminating.
arch/arm64/kvm/hyp/nvhe/its_emulate.c:pkvm_host_unmap_last_level() {
...
ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(table_addr), psz >> PAGE_SHIFT);
}
[Severity: Critical]
Does this bypass pKVM isolation guarantees by trusting the host-provided page
size?
The psz field appears to be read directly from the untrusted host_state
without validation. If a malicious host sets psz to a value strictly less than
PAGE_SIZE (e.g., 0), psz >> PAGE_SHIFT evaluates to 0. The donation will
silently succeed without actually transferring page ownership, allowing the
host to retain ownership and concurrently modify the tables.
arch/arm64/kvm/hyp/nvhe/its_emulate.c:pkvm_its_emulate_setup() {
...
err_with_priv:
WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(priv_state), 1));
}
[Severity: High]
Does this error path leak hypervisor memory?
The setup successfully donates priv_num_pages from the host to EL2, but if a
subsequent operation fails, this rollback hardcodes 1 instead of using
priv_num_pages. If priv_num_pages is 2 for ITS setup, the remaining pages are
permanently left in the HYP-owned state.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807164322.2970811-2-sebastianene@google.com?part=13
^ permalink raw reply [flat|nested] 27+ messages in thread