From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Move VMX vpid allocation to fix vpid leak in vmm.
To: Dave Voutila <dv@sisu.io>
Cc: Theo Buehler <tb@theobuehler.org>, tech <tech@openbsd.org>, Mike Larkin <mlarkin@openbsd.org>, Hans-J?rg H?xer <Hans-Joerg_Hoexer@genua.de>
Date: Mon, 2 Sep 2024 23:15:48 +0200

On Mon, Sep 02, 2024 at 04:55:47PM -0400, Dave Voutila wrote:
> Updated diff below.

OK bluhm@

> diffstat refs/heads/master refs/heads/vpid-leak
>  M  sys/arch/amd64/amd64/vmm_machdep.c  |  19+  35-
> 
> 1 file changed, 19 insertions(+), 35 deletions(-)
> 
> diff refs/heads/master refs/heads/vpid-leak
> commit - 84b38932e34d2692ffaf821e4542142c343fa76f
> commit + 929c982aca50544a7e0770af67bfc71b0f2400d1
> blob - c84fbb3273c799116e4628a1097db4eb9f135955
> blob + 2fe7d8074968fdf3da08c179a9e8ee261de199f7
> --- sys/arch/amd64/amd64/vmm_machdep.c
> +++ sys/arch/amd64/amd64/vmm_machdep.c
> @@ -2253,7 +2253,7 @@ vcpu_reset_regs_vmx(struct vcpu *vcpu, struct vcpu_reg
>  	uint32_t pinbased, procbased, procbased2, exit, entry;
>  	uint32_t want1, want0;
>  	uint64_t ctrlval, cr3, msr_misc_enable;
> -	uint16_t ctrl, vpid;
> +	uint16_t ctrl;
>  	struct vmx_msr_store *msr_store;
> 
>  	rw_assert_wrlock(&vcpu->vc_lock);
> @@ -2516,30 +2516,12 @@ vcpu_reset_regs_vmx(struct vcpu *vcpu, struct vcpu_reg
>  	    IA32_VMX_ACTIVATE_SECONDARY_CONTROLS, 1)) {
>  		if (vcpu_vmx_check_cap(vcpu, IA32_VMX_PROCBASED2_CTLS,
>  		    IA32_VMX_ENABLE_VPID, 1)) {
> -
> -			/* We may sleep during allocation, so reload VMCS. */
> -			vcpu->vc_last_pcpu = curcpu();
> -			ret = vmm_alloc_vpid(&vpid);
> -			if (vcpu_reload_vmcs_vmx(vcpu)) {
> -				printf("%s: failed to reload vmcs\n", __func__);
> -				ret = EINVAL;
> -				goto exit;
> -			}
> -			if (ret) {
> -				DPRINTF("%s: could not allocate VPID\n",
> -				    __func__);
> -				ret = EINVAL;
> -				goto exit;
> -			}
> -
> -			if (vmwrite(VMCS_GUEST_VPID, vpid)) {
> +			if (vmwrite(VMCS_GUEST_VPID, vcpu->vc_vpid)) {
>  				DPRINTF("%s: error setting guest VPID\n",
>  				    __func__);
>  				ret = EINVAL;
>  				goto exit;
>  			}
> -
> -			vcpu->vc_vpid = vpid;
>  		}
>  	}
> 
> @@ -2832,13 +2814,19 @@ vcpu_init_vmx(struct vcpu *vcpu)
>  	uint32_t cr0, cr4;
>  	int ret = 0;
> 
> +	/* Allocate a VPID early to avoid km_alloc if we're out of VPIDs. */
> +	if (vmm_alloc_vpid(&vcpu->vc_vpid))
> +		return (ENOMEM);
> +
>  	/* Allocate VMCS VA */
>  	vcpu->vc_control_va = (vaddr_t)km_alloc(PAGE_SIZE, &kv_page, &kp_zero,
>  	    &kd_waitok);
>  	vcpu->vc_vmx_vmcs_state = VMCS_CLEARED;
> 
> -	if (!vcpu->vc_control_va)
> -		return (ENOMEM);
> +	if (!vcpu->vc_control_va) {
> +		ret = ENOMEM;
> +		goto exit;
> +	}
> 
>  	/* Compute VMCS PA */
>  	if (!pmap_extract(pmap_kernel(), vcpu->vc_control_va,
> @@ -3091,15 +3079,20 @@ vcpu_reset_regs(struct vcpu *vcpu, struct vcpu_reg_sta
>  int
>  vcpu_init_svm(struct vcpu *vcpu, struct vm_create_params *vcp)
>  {
> -	uint16_t asid;
>  	int ret = 0;
> 
> +	/* Allocate an ASID early to avoid km_alloc if we're out of ASIDs. */
> +	if (vmm_alloc_vpid(&vcpu->vc_vpid))
> +		return (ENOMEM);
> +
>  	/* Allocate VMCB VA */
>  	vcpu->vc_control_va = (vaddr_t)km_alloc(PAGE_SIZE, &kv_page, &kp_zero,
>  	    &kd_waitok);
> 
> -	if (!vcpu->vc_control_va)
> -		return (ENOMEM);
> +	if (!vcpu->vc_control_va) {
> +		ret = ENOMEM;
> +		goto exit;
> +	}
> 
>  	/* Compute VMCB PA */
>  	if (!pmap_extract(pmap_kernel(), vcpu->vc_control_va,
> @@ -3173,14 +3166,6 @@ vcpu_init_svm(struct vcpu *vcpu, struct vm_create_para
>  	    (uint64_t)vcpu->vc_svm_ioio_va,
>  	    (uint64_t)vcpu->vc_svm_ioio_pa);
> 
> -	/* Guest VCPU ASID */
> -	if (vmm_alloc_vpid(&asid)) {
> -		DPRINTF("%s: could not allocate asid\n", __func__);
> -		ret = EINVAL;
> -		goto exit;
> -	}
> -	vcpu->vc_vpid = asid;
> -
>  	/* Shall we enable SEV? */
>  	vcpu->vc_sev = vcp->vcp_sev;
> 
> @@ -3260,8 +3245,7 @@ vcpu_deinit_vmx(struct vcpu *vcpu)
>  	}
>  #endif
> 
> -	if (vcpu->vc_vmx_vpid_enabled)
> -		vmm_free_vpid(vcpu->vc_vpid);
> +	vmm_free_vpid(vcpu->vc_vpid);
>  }
> 
>  /*