linux-kernel - Re: [RESEND v4 2/2] KVM: VMX: Enable bus lock VM exit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201020221943.GB9031@linux.intel.com>
Date:   Tue, 20 Oct 2020 15:19:45 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Chenyi Qiang <chenyi.qiang@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Xiaoyao Li <xiaoyao.li@...el.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RESEND v4 2/2] KVM: VMX: Enable bus lock VM exit

On Mon, Oct 12, 2020 at 11:35:42AM +0800, Chenyi Qiang wrote:
> @@ -6138,6 +6149,26 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  	return 0;
>  }
>  
> +static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
> +{
> +	int ret = __vmx_handle_exit(vcpu, exit_fastpath);
> +
> +	/*
> +	 * Even when current exit reason is handled by KVM internally, we
> +	 * still need to exit to user space when bus lock detected to inform
> +	 * that there is a bus lock in guest.
> +	 */
> +	if (to_vmx(vcpu)->exit_reason.bus_lock_detected) {
> +		if (ret > 0)
> +			vcpu->run->exit_reason = KVM_EXIT_BUS_LOCK;
> +		else
> +			vcpu->run->flags |= KVM_RUN_BUS_LOCK;

This should always set flags.KVM_RUN_BUS_LOCK, e.g. so that userspace can
always check flags.KVM_RUN_BUS_LOCK instead of having to check both the flag
and the exit reason.  As is, it's really bad because the flag is undefined,
which could teach userspace to do the wrong thing.

> +		return 0;
> +	}
> +	vcpu->run->flags &= ~KVM_RUN_BUS_LOCK;

Hmm, I feel like explicitly clearing flags is should be unnecessary.  By
that, I mean that's it's necessary in the current patch, bit I think we should
figure out how to make that not be the case.  With the current approach, every
chunk of code that needs to set a flag also needs to clear it, which increases
the odds of missing a case and ending up with a flag in an undefined state.

The easiest way I can think of is to add another prep patch that zeros
run->flags at the beginning of kvm_arch_vcpu_ioctl_run(), and changes
post_kvm_run_save() to do:

	if (is_smm(vcpu))
		kvm_run->flags |= KVM_RUN_X86_SMM;

Then this patch can omit clearing KVM_RUN_BUS_LOCK, and doesn't have to touch
the SMM flag.

> +	return ret;
> +}
> +
>  /*
>   * Software based L1D cache flush which is used when microcode providing
>   * the cache control MSR is not loaded.