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 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?
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.
>
> 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.
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 which
there are many, have multiple unions with fields of varying size, and all kinds
of subtle rules/logic for which field is used, how many bytes within a given field
are valid, etc.
It pains me a bit to say this, but I think we're best off leaving the emulator
as-is, and relying on things like fancy compiler features, UBSAN, and fuzzers 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;
};
};
