linux-kernel - Re: [PATCH] KVM: x86/xen: Make number of event channels defines less magical

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: <fde14caa0cf774b2b46f1124644a3b326a0a8f09.camel@infradead.org>
Date:   Mon, 14 Nov 2022 14:24:32 -0800
From:   David Woodhouse <dwmw2@...radead.org>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Paul Durrant <paul@....org>
Subject: Re: [PATCH] KVM: x86/xen: Make number of event channels defines
 less magical

On Mon, 2022-11-14 at 19:39 +0000, Sean Christopherson wrote:
> Ugh.  I worried that might be the case.  An alternative approach to help document
> things from a KVM perspective would be something like:
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 93c628d3e3a9..7769f3b98af0 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -1300,6 +1300,9 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>  
>  static inline int max_evtchn_port(struct kvm *kvm)
>  {
> +       BUILD_BUG_ON(EVTCHN_2L_NR_CHANNELS !=
> +                    (sizeof_field(struct shared_info, evtchn_pending) * BITS_PER_BYTE));
> +
>         if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode)
>                 return EVTCHN_2L_NR_CHANNELS;
>         else

Not really sure I see the point of that.

There are two main reasons for that kind of BUILD_BUG_ON(). I've added
a few of them asserting that the size of the structure and its compat
variant are identical, and thus documenting *why* the code lacks compat
handling. For example...

	/*
	 * Next, write the new runstate. This is in the *same* place
	 * for 32-bit and 64-bit guests, asserted here for paranoia.
	 */
	BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, state) !=
		     offsetof(struct compat_vcpu_runstate_info, state));

The second reason is to prevent accidental screwups where our local
definition of a structure varies from the official ABI. Like these:

	/* Paranoia checks on the 32-bit struct layout */
	BUILD_BUG_ON(offsetof(struct compat_shared_info, wc) != 0x900);
	BUILD_BUG_ON(offsetof(struct compat_shared_info, arch.wc_sec_hi) != 0x924);
	BUILD_BUG_ON(offsetof(struct pvclock_vcpu_time_info, version) != 0);

I don't really see the above fulfilling either of those use cases.
Given that the definition of the evtchn_pending field is:

        xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8];

It's fairly tautological that the number of event channels supported is
BITS_PER_ULONG * BITS_PER_ULONG. Which is sizeof(xen_ulong_t)² * 64 as
defined in the official Xen headers.

I don't know that we really need to add our own sanity check on the
headers we imported from Xen. It doesn't seem to add much.

No objection to cleaning up the COMPAT one though, as in your original
patch.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)