[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1b0252b29c19cc08c41e1b58b26fbcf1f3fb06e4.camel@cyberus-technology.de>
Date: Thu, 5 Oct 2023 13:48:30 +0000
From: Julian Stecklina <julian.stecklina@...erus-technology.de>
To: "seanjc@...gle.com" <seanjc@...gle.com>
CC: "x86@...nel.org" <x86@...nel.org>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"hpa@...or.com" <hpa@...or.com>,
"mingo@...hat.com" <mingo@...hat.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"bp@...en8.de" <bp@...en8.de>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] KVM: x86: Fix partially uninitialized integer in
emulate_pop
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 in
> > emulate_pop. len is usually the word width of the guest.
> >
> > 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.
> >
> > 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.
> >
> > Fix this by explicitly requiring a unsigned long pointer and
> > initializing it with zero in all cases.
>
> NAK, this will break em_leave() as it will zero RBP regardless of how many
> bytes
> are actually supposed to be written. Specifically, KVM would incorrectly
> clobber
> RBP[31:16] if LEAVE is executed with a 16-bit stack.
Thanks, Sean! Great catch. I didn't see this. Is there already a test suite for
this?
> I generally like defense-in-depth approaches, but zeroing data that the caller
> did not ask to be written is not a net positive.
I'll rewrite the patch to just initialize variables where they are currently
not. This should be a bit more conservative and have less risk of breaking
anything.
Julian
Powered by blists - more mailing lists