Re: [PATCH v8 07/25] KVM: arm64: iommu: Shadow host stage-2 page table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: arm64: pkvm: guest-triggerable hyp panic due to WARN_ON misuse
--

commit 68e26689446b14f6e5c91a79e4493c62586f9fb4
Author: Mostafa Saleh <smostafa@google.com>
Subject: KVM: arm64: iommu: Shadow host stage-2 page table

This commit creates a page-table for the IOMMU that shadows the host CPU
stage-2 to establish DMA isolation. An initial snapshot is created after
the driver init, and on permission changes, a callback updates the IOMMU
page table.

> diff --git a/arch/arm64/kvm/hyp/nvhe/iommu.c b/arch/arm64/kvm/hyp/nvhe/iommu.c
> index ef456eff42d29..3637b0327d9e8 100644
> --- a/arch/arm64/kvm/hyp/nvhe/iommu.c
> +++ b/arch/arm64/kvm/hyp/nvhe/iommu.c

[ ... ]

> @@ -4,16 +4,138 @@
>  int pkvm_iommu_init(void)
>  {
> +	int ret;
> +
>  	/* Keep DMA isolation optional. */
> -	if (!pkvm_iommu_ops || !pkvm_iommu_ops->init)
> +	if (!pkvm_iommu_ops || !pkvm_iommu_ops->init ||
> +	    !pkvm_iommu_ops->host_stage2_idmap)
>  		return 0;
>  
> -	return pkvm_iommu_ops->init();
> +	ret = pkvm_iommu_ops->init();
> +	if (ret)
> +		return ret;
> +
> +	return WARN_ON(pkvm_iommu_snapshot_host_stage2());

[Severity: High]
Does this unintentionally cause a hypervisor panic if the snapshot fails?

At EL2 (nVHE/pKVM), WARN_ON expands to a fatal BRK instruction rather than
a warn-and-continue. Wrapping a fallible operation like this triggers a
panic during initialization instead of returning the error.

> +}

[ ... ]

> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index c1a8fd811c0fa..da5014e4e96b6 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c

[ ... ]

> @@ -603,16 +604,15 @@ static int host_stage2_set_owner_metadata_locked(phys_addr_t addr, u64 size,
>  	ret = host_stage2_try(kvm_pgtable_stage2_annotate, &host_mmu.pgt,
>  			      addr, size, &host_s2_pool,
>  			      KVM_HOST_INVALID_PTE_TYPE_DONATION, annotation);
> -	if (!ret) {
> -		/*
> -		 * After stage2 maintenance has happened, but before the page
> -		 * owner has changed.
> -		 */
> -		pkvm_sme_dvmsync_fw_call();
> -		__host_update_page_state(addr, size, PKVM_NOPAGE);
> -	}
> -
> -	return ret;
> +	if (ret)
> +		return ret;
> +	/*
> +	 * After stage2 maintenance has happened, but before the page
> +	 * owner has changed.
> +	 */
> +	pkvm_sme_dvmsync_fw_call();
> +	__host_update_page_state(addr, size, PKVM_NOPAGE);
> +	return WARN_ON(pkvm_iommu_host_stage2_idmap(addr, addr + size, 0));

[Severity: High]
Does this create a dead-branch trap that skips rolling back the page state?

Since WARN_ON acts as a panic at EL2, any failure in the IOMMU mapping call
will immediately crash the hypervisor. It also leaves the earlier mutation
from __host_update_page_state without a rollback path on failure.

>  }
>  
>  int host_stage2_set_owner_locked(phys_addr_t addr, u64 size, u8 owner_id)
> @@ -625,8 +625,10 @@ int host_stage2_set_owner_locked(phys_addr_t addr, u64 size, u8 owner_id)
>  			return -EPERM;
>  
>  		ret = host_stage2_idmap_locked(addr, size, PKVM_HOST_MEM_PROT);
> -		if (!ret)
> -			__host_update_page_state(addr, size, PKVM_PAGE_OWNED);
> +		if (ret)
> +			break;
> +		__host_update_page_state(addr, size, PKVM_PAGE_OWNED);
> +		ret = WARN_ON(pkvm_iommu_host_stage2_idmap(addr, addr + size, PKVM_HOST_MEM_PROT));

[Severity: High]
Is it intentional that a failure here panics instead of returning the error?

Assigning WARN_ON to a variable suggests an expectation that it returns,
but at EL2 it triggers a fatal BRK instruction. This also leaves the prior
page state update without a rollback.

>  		break;

[ ... ]

> @@ -1029,6 +1031,7 @@ int __pkvm_guest_share_host(struct pkvm_hyp_vcpu *vcpu, u64 gfn)
>  				       pkvm_mkstate(KVM_PGTABLE_PROT_RWX, PKVM_PAGE_SHARED_OWNED),
>  				       &vcpu->vcpu.arch.pkvm_memcache, 0));
>  	WARN_ON(__host_set_page_state_range(phys, PAGE_SIZE, PKVM_PAGE_SHARED_BORROWED));
> +	WARN_ON(pkvm_iommu_host_stage2_idmap(phys, phys + PAGE_SIZE, PKVM_HOST_MEM_PROT));

[Severity: High]
Can an untrusted guest intentionally trigger a host denial of service here?

By triggering memory sharing hypercalls, a guest might exhaust the IOMMU
page pool, forcing an allocation failure in pkvm_iommu_host_stage2_idmap.
Wrapping this fallible outcome in WARN_ON will trigger a hypervisor panic.

>  unlock:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922131259.2975334-1-smostafa@google.com?part=7