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

[sashiko-bot@kernel.org]

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