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

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 01:23:59PM +0000, sashiko-bot@kernel.org wrote:
> 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.

That is intentional as there is no roll-back beyond this point, this
pattern is commonly used with pKVM.

> 
> > +}
> 
> [ ... ]
> 
> > 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.

This is added for readability, not really a problem.

> 
> >  }
> >  
> >  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.

No need for rollback as it panics.

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

The IOMMU can't be exhasuted by design, the same WARN_ON() exist in the
line above for the CPU page table update also.

Thanks,
Mostafa

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