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

On Sun, Sep 01, 2024 at 09:46:16PM -0400, Dave Voutila wrote:
> I noticed this while Hans-Joerg was working on SEV support for
> vmm(4). Since vpid allocation is in the reset register ioctl handler,
> and we don't free any previous vpid, a program on an Intel host that
> keeps resetting a vcpu via VMM_IOC_RESETCPU will exhaust vpids.
> 
> Instead of a check & free dance, this just moves the allocation to the
> vcpu initialization since vmm frees vpids/asids in vcpu de-init. (The
> SEV changes moved the SVM ASID stuff similarly.)
> 
> It also removes the DPRINTF (also on the SVM side) since the allocation
> function already has a printf on failure.
> 
> Plus, since we're de-coupling allocation from configuration, the VMCS
> reload can be avoided, making this a net-negative diff.
> 
> ok?
> 

Not sure if that matters, but previously vmm_init_vpx()'s ENOMEM was
always mapped to EINVAL. Now there's a bit of an asymmetry between 
vcpu_init_vmx() and vcpu_init_svm() in that regard.

> 
> diffstat refs/heads/master refs/heads/vpid-leak
>  M  sys/arch/amd64/amd64/vmm_machdep.c  |  7+  21-
> 
> 1 file changed, 7 insertions(+), 21 deletions(-)
> 
> diff refs/heads/master refs/heads/vpid-leak
> commit - 24056440d1d6e8d3aa8785990526387386042fb4
> commit + a01d423d1b92501a0e6b5cc4b9fe22a9e3983795
> blob - c84fbb3273c799116e4628a1097db4eb9f135955
> blob + a472790c5d18da3cc8c7954a2c5006b745009c53
> --- 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,6 +2814,11 @@ vcpu_init_vmx(struct vcpu *vcpu)
>  	uint32_t cr0, cr4;
>  	int ret = 0;
> 
> +	/* Allocate a VPID early because there's no cleanup to do on failure. */
> +	ret = vmm_alloc_vpid(&vcpu->vc_vpid);
> +	if (ret)
> +		return (ret);
> +
>  	/* Allocate VMCS VA */
>  	vcpu->vc_control_va = (vaddr_t)km_alloc(PAGE_SIZE, &kv_page, &kp_zero,
>  	    &kd_waitok);
> @@ -3175,7 +3162,6 @@ vcpu_init_svm(struct vcpu *vcpu, struct vm_create_para
> 
>  	/* Guest VCPU ASID */
>  	if (vmm_alloc_vpid(&asid)) {
> -		DPRINTF("%s: could not allocate asid\n", __func__);
>  		ret = EINVAL;
>  		goto exit;
>  	}
>