linux-kernel - Re: [PATCH 01/22] KVM: x86: Drop unnecessary and confusing KVM_X86_OP

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]

Date:   Fri, 28 Jan 2022 15:42:48 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Like Xu <like.xu.linux@...il.com>
Subject: Re: [PATCH 01/22] KVM: x86: Drop unnecessary and confusing
 KVM_X86_OP_NULL macro

On Fri, Jan 28, 2022, Paolo Bonzini wrote:
> On 1/28/22 01:51, Sean Christopherson wrote:
> > Drop KVM_X86_OP_NULL, which is superfluous and confusing.  The macro is
> > just a "pass-through" to KVM_X86_OP; it was added with the intent of
> > actually using it in the future, but that obviously never happened.  The
> > name is confusing because its intended use was to provide a way for
> > vendor implementations to specify a NULL pointer, and even if it were
> > used, wouldn't necessarily be synonymous with declaring a kvm_x86_op as
> > DEFINE_STATIC_CALL_NULL.
> > 
> > Lastly, actually using KVM_X86_OP_NULL as intended isn't a maintanable
> > approach, e.g. bleeds vendor details into common x86 code, and would
> > either be prone to bit rot or would require modifying common x86 code
> > when modifying a vendor implementation.
> 
> I have some patches that redefine KVM_X86_OP_NULL as "must be used with
> static_call_cond".  That's a more interesting definition, as it can be used
> to WARN if KVM_X86_OP is used with a NULL function pointer.

I'm skeptical that will actually work well and be maintainble.  E.g. sync_pir_to_ir()
must be explicitly check for NULL in apic_has_interrupt_for_ppr(), forcing that path
to do static_call_cond() will be odd.  Ditto for ops that are wired up to ioctl()s,
e.g. the confidential VM stuff, and for ops that are guarded by other stuff, e.g. the
hypervisor timer.

Actually, it won't just be odd, it will be impossible to disallow NULL a pointer
for KVM_X86_OP and require static_call_cond() for KVM_X86_OP_NULL.  static_call_cond()
forces the return to "void", so any path that returns a value needs to be manually
guarded and can't use static_call_cond(), e.g.

arch/x86/kvm/x86.c: In function ‘kvm_arch_vm_ioctl’:
arch/x86/kvm/x86.c:6450:19: error: void value not ignored as it ought to be
 6450 |                 r = static_call_cond(kvm_x86_mem_enc_ioctl)(kvm, argp);
      |                   ^