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

[Fuad Tabba]

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)

> +        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)

> +                        } 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)

> +        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