Re: [PATCH v2 11/13] KVM: arm64: Prevent the host from specifying a different command queue

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

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