On Tue, Aug 18, 2020 at 2:16 PM Aaron Lewis <aaronlewis@xxxxxxxxxx> wrote:
>
> Add support for exiting to userspace on a rdmsr or wrmsr instruction if
> the MSR being read from or written to is in the user_exit_msrs list.
>
> Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx>
> ---
>
> v2 -> v3
>
> - Refactored commit based on Alexander Graf's changes in the first commit
> in this series. Changes made were:
> - Updated member 'inject_gp' to 'error' based on struct msr in kvm_run.
> - Move flag 'vcpu->kvm->arch.user_space_msr_enabled' out of
> kvm_msr_user_space() to allow it to work with both methods that bounce
> to userspace (msr list and #GP fallback). Updated caller functions
> to account for this change.
> - trace_kvm_msr has been moved up and combine with a previous call in
> complete_emulated_msr() based on the suggestion made by Alexander
> Graf <graf@xxxxxxxxxx>.
>
> ---
> @@ -1653,9 +1663,6 @@ static int kvm_msr_user_space(struct kvm_vcpu *vcpu, u32 index,
> u32 exit_reason, u64 data,
> int (*completion)(struct kvm_vcpu *vcpu))
> {
> - if (!vcpu->kvm->arch.user_space_msr_enabled)
> - return 0;
> -
> vcpu->run->exit_reason = exit_reason;
> vcpu->run->msr.error = 0;
> vcpu->run->msr.pad[0] = 0;
> @@ -1686,10 +1693,18 @@ int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu)
> u64 data;
> int r;
>
> + if (kvm_msr_user_exit(vcpu->kvm, ecx)) {
> + kvm_get_msr_user_space(vcpu, ecx);
> + /* Bounce to user space */
> + return 0;
> + }
> +
> +
> r = kvm_get_msr(vcpu, ecx, &data);
>
> /* MSR read failed? See if we should ask user space */
> - if (r && kvm_get_msr_user_space(vcpu, ecx)) {
> + if (r && vcpu->kvm->arch.user_space_msr_enabled) {
> + kvm_get_msr_user_space(vcpu, ecx);
> /* Bounce to user space */
> return 0;
> }
The before and after bounce to userspace is unfortunate. If we can
consolidate the allow/deny list checks at the top of kvm_get_msr(),
and we can tell why kvm_get_msr() failed (e.g. -EPERM=disallowed,
-ENOENT=unknown MSR, -EINVAL=illegal access), then we can eliminate
the first bounce to userspace above. -EPERM would always go to
userspace. -ENOENT would go to userspace if userspace asked to handle
unknown MSRs. -EINVAL would go to userspace if userspace asked to
handle all #GPs. (Yes; I'd still like to be able to distinguish
between "unknown MSR" and "illegal value." Otherwise, it seems
impossible for userspace to know how to proceed.)
(You may ask, "why would you get -EINVAL on a RDMSR?" This would be
the case if you tried to read a write-only MSR, like IA32_FLUSH_CMD.)
> @@ -1715,10 +1730,17 @@ int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
> u64 data = kvm_read_edx_eax(vcpu);
> int r;
>
> + if (kvm_msr_user_exit(vcpu->kvm, ecx)) {
> + kvm_set_msr_user_space(vcpu, ecx, data);
> + /* Bounce to user space */
> + return 0;
> + }
> +
> r = kvm_set_msr(vcpu, ecx, data);
>
> /* MSR write failed? See if we should ask user space */
> - if (r && kvm_set_msr_user_space(vcpu, ecx, data)) {
> + if (r && vcpu->kvm->arch.user_space_msr_enabled) {
> + kvm_set_msr_user_space(vcpu, ecx, data);
> /* Bounce to user space */
> return 0;
> }
Same idea as above.
> @@ -3606,6 +3628,25 @@ static int kvm_vm_ioctl_set_exit_msrs(struct kvm *kvm,
> return 0;
> }
>
> +bool kvm_msr_user_exit(struct kvm *kvm, u32 index)
> +{
> + struct kvm_msr_list *exit_msrs;
> + int i;
> +
> + exit_msrs = kvm->arch.user_exit_msrs;
> +
> + if (!exit_msrs)
> + return false;
> +
> + for (i = 0; i < exit_msrs->nmsrs; ++i) {
> + if (exit_msrs->indices[i] == index)
> + return true;
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(kvm_msr_user_exit);
I think this should probably be scrapped, along with Alexander's
allow-list check, in favor of a rule-based allow/deny list approach as
I outlined in an earlier message today.
