linux-kernel - Re: [PATCH v2 5/5] KVM: x86: allow defining return-0 static calls

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: <Ygvi5jr4V8S/bKSe@google.com>
Date:   Tue, 15 Feb 2022 17:29:10 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v2 5/5] KVM: x86: allow defining return-0 static calls

On Mon, Feb 14, 2022, Paolo Bonzini wrote:
> A few vendor callbacks are only used by VMX, but they return an integer
> or bool value.  Introduce KVM_X86_OP_RET0 for them: a NULL value in

s/KVM_X86_OP_RET0/KVM_X86_OP_OPTIONAL_RET0

And maybe "NULL func" instead of "NULL value", since some members of kvm_x86_ops
hold a value, not a func.

> struct kvm_x86_ops will be changed to __static_call_return0.

This implies kvm_x86_ops itself is changed, which is incorrect.  "will be patched
to __static_call_return0() when updating static calls" or so.

> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h | 20 +++++++++++++-------
>  arch/x86/include/asm/kvm_host.h    |  4 ++++
>  arch/x86/kvm/svm/avic.c            |  5 -----
>  arch/x86/kvm/svm/svm.c             | 26 --------------------------
>  arch/x86/kvm/x86.c                 |  2 +-
>  kernel/static_call.c               |  1 +
>  6 files changed, 19 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 0a074354aaf7..ad75ff5ac220 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -6,14 +6,19 @@ BUILD_BUG_ON(1)
>  /*
>   * KVM_X86_OP() and KVM_X86_OP_OPTIONAL() are used to help generate
>   * "static_call()"s. They are also intended for use when defining
> - * the vmx/svm kvm_x86_ops. KVM_X86_OP_OPTIONAL() can be used for those
> + * the vmx/svm kvm_x86_ops.
> + *
> + * KVM_X86_OP_OPTIONAL() can be used for those
>   * functions that can have a NULL definition, for example if
>   * "static_call_cond()" will be used at the call sites.
> + * KVM_X86_OP_OPTIONAL_RET0() can be used likewise to make
> + * a definition optional, but in this case the default will 

ERROR: trailing whitespace
#35: FILE: arch/x86/include/asm/kvm-x86-ops.h:15:
+ * a definition optional, but in this case the default will $

> + * be __static_call_return0.

Uber nit, __static_call_return0() to make it clear that that's a function, not a
magic return value (though arguably it's that too).

>   */
>  KVM_X86_OP(hardware_enable)
>  KVM_X86_OP(hardware_disable)
>  KVM_X86_OP(hardware_unsetup)
> -KVM_X86_OP(cpu_has_accelerated_tpr)
> +KVM_X86_OP_OPTIONAL_RET0(cpu_has_accelerated_tpr)

Can we instead just remove this helper entirely and return '1' unconditionally
from KVM_CAP_VAPIC?

The usage appears to be wrong, this will return '0' for VMX, '1' for SVM.

	case KVM_CAP_VAPIC:
		r = !static_call(kvm_x86_cpu_has_accelerated_tpr)();
		break;

Further more, our uapi says:

  /* Available with KVM_CAP_VAPIC */
  #define KVM_TPR_ACCESS_REPORTING  _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
  /* Available with KVM_CAP_VAPIC */
  #define KVM_SET_VAPIC_ADDR        _IOW(KVMIO,  0x93, struct kvm_vapic_addr)

But neither of those check cpu_has_accelerated_tpr().  QEMU doesn't check the
cap, and AFAICT neither does our VMM.