Re: [PATCH v2 08/13] KVM: arm64: Trap & emulate the ITS MAPD command

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 <[email protected]>

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/[email protected]?part=8