Re: [PATCH v2 08/13] KVM: arm64: Trap & emulate the ITS MAPD command
flat view
From: Fuad Tabba <fuad.tabba@linux.dev>
Date: 2026-09-15 15:28:12
Also in:
kvmarm, lkml
Hi Sebastian, The MAPD emulation is the right idea, but there's a fair bit to fix. The main ones: On Fri, Aug 07, 2026 at 04:43:18PM +0000, Sebastian Ene wrote: [...]
+static int get_num_itt_pages(struct its_priv_state *its, u8 num_bits)
+{[...]
+ return PAGE_ALIGN(sz) >> PAGE_SHIFT; +}
This needs the ITT's intra-page offset. itt_addr is only 256-byte aligned (GENMASK(51, 8)), so PAGE_ALIGN(sz) undercounts by a page when the ITT starts mid-page, and track_pfn() then leaves the tail page unpinned for the host to repurpose before a MAPTI. PAGE_ALIGN((itt_addr & ~PAGE_MASK) + sz) >> PAGE_SHIFT. (Sashiko)
quoted hunk ↗ jump to hunk
+ new_entry = snapshot_table[new_entry_index]; + prev_entry = original_table[new_entry_index]; + if (!((new_entry ^ prev_entry) & GITS_BASER_VALID)) + return 0; + + if (rollback) + new_entry = new_entry ^ GITS_BASER_VALID;
[...]
+ original_table[new_entry_index] = new_entry;
The rollback here never runs. The forward pass ends with original_table[idx] = new_entry, so on rollback the guard above sees snapshot and original with the same valid bit and returns before the flip. A failed MAPD then never gives the L2 back, and original stays mapped. (Sashiko)
+ if (new_entry & GITS_BASER_VALID) + ret = __pkvm_host_donate_hyp(hyp_phys_to_pfn(new_entry & PHYS_MASK), table->psz >> PAGE_SHIFT);
Could the L2 address be checked psz-aligned first? hyp_phys_to_pfn(new_entry & PHYS_MASK) rounds down to a page, but the ITS uses the full address copied into original, so an unaligned entry runs the ITS into the next, undonated page. (Sashiko)
quoted hunk ↗ jump to hunk
+ } else { + return hyp_pin_shared_mem(virt, virt + PAGE_SIZE); + }
A duplicate MAPD for the same device and pfn pins again here, so the refcount climbs with no new slot and track_pfn_remove()'s single unpin can't free the page. (Sashiko)
+ if (ret == -EBUSY) + return 0;
[...]
+ return -EINVAL;
Two things in track_pfn_remove(). The -EBUSY return skips the memset that frees the dte_entry slot, so the slot leaks. And the trailing -EINVAL, for a V=0 unmap of an ITT that isn't tracked, propagates up and stalls the queue, where a legal unmap shouldn't. (Sashiko)
quoted hunk ↗ jump to hunk
+ 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 track_pfn() fails after check_table_update() donated the L2, nothing undoes the donation: process_cmd()'s forward-failure path in cwriter_write() just returns, with no rollback. (Sashiko) The snapshot-L1 TOCTOU during rollback (another host CPU can rewrite it while this one holds its_lock in EL2) and the old-ITT leak on a re-MAPD are real too. (Sashiko) Cheers, /fuad