Messages in this thread | | | From | Julian Stecklina <> | Subject | Re: [PATCH 1/2] KVM: x86: Fix partially uninitialized integer in emulate_pop | Date | Thu, 5 Oct 2023 13:48:30 +0000 |
| |
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
| |