Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Fri, 6 Oct 2023 00:56:58 +0000
In-Reply-To: <1b0252b29c19cc08c41e1b58b26fbcf1f3fb06e4.camel@cyberus-technology.de>
Mime-Version: 1.0
References: <20231004133827.107-1-julian.stecklina@cyberus-technology.de>
 <ZR1_lizQd14pbXbg@google.com> <1b0252b29c19cc08c41e1b58b26fbcf1f3fb06e4.camel@cyberus-technology.de>
Message-ID: <ZR9bWv_Fogzx1zwv@google.com>
Subject: Re: [PATCH 1/2] KVM: x86: Fix partially uninitialized integer in emulate_pop
From:   Sean Christopherson <seanjc@google.com>
To:     Julian Stecklina <julian.stecklina@cyberus-technology.de>
Cc:     "x86@kernel.org" <x86@kernel.org>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "bp@alien8.de" <bp@alien8.de>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 05, 2023, Julian Stecklina wrote:
> On Wed, 2023-10-04 at 08:07 -0700, Sean Christopherson wrote:
> > On Wed, Oct 04, 2023, Julian Stecklina wrote:
> > > Most code gives a pointer to an uninitialized unsigned long as dest i=
n
> > > emulate_pop. len is usually the word width of the guest.
> > >=20
> > > If the guest runs in 16-bit or 32-bit modes, len will not cover the
> > > whole unsigned long and we end up with uninitialized data in dest.
> > >=20
> > > Looking through the callers of this function, the issue seems
> > > harmless, but given that none of this is performance critical, there
> > > should be no issue with just always initializing the whole value.
> > >=20
> > > Fix this by explicitly requiring a unsigned long pointer and
> > > initializing it with zero in all cases.
> >=20
> > NAK, this will break em_leave() as it will zero RBP regardless of how m=
any
> > bytes
> > are actually supposed to be written.=C2=A0 Specifically, KVM would inco=
rrectly
> > clobber
> > RBP[31:16] if LEAVE is executed with a 16-bit stack.
>=20
> Thanks, Sean! Great catch. I didn't see this. Is there already a test sui=
te for
> this?

No, I'm just excessively paranoid when it comes to the emulator :-)

> > I generally like defense-in-depth approaches, but zeroing data that the=
 caller
> > did not ask to be written is not a net positive.
>=20
> I'll rewrite the patch to just initialize variables where they are curren=
tly
> not. This should be a bit more conservative and have less risk of breakin=
g
> anything.

In all honesty, I wouldn't bother.  Trying to harden the emulator code for =
things
like this will be a never ending game of whack-a-mole.  The operands, of wh=
ich
there are many, have multiple unions with fields of varying size, and all k=
inds
of subtle rules/logic for which field is used, how many bytes within a give=
n field
are valid, etc.

It pains me a bit to say this, but I think we're best off leaving the emula=
tor
as-is, and relying on things like fancy compiler features, UBSAN, and fuzze=
rs to
detect any lurking bugs.

  struct operand {
	enum { OP_REG, OP_MEM, OP_MEM_STR, OP_IMM, OP_XMM, OP_MM, OP_NONE } type;
	unsigned int bytes;
	unsigned int count;
	union {
		unsigned long orig_val;
		u64 orig_val64;
	};
	union {
		unsigned long *reg;
		struct segmented_address {
			ulong ea;
			unsigned seg;
		} mem;
		unsigned xmm;
		unsigned mm;
	} addr;
	union {
		unsigned long val;
		u64 val64;
		char valptr[sizeof(sse128_t)];
		sse128_t vec_val;
		u64 mm_val;
		void *data;
	};
  };