Re: [PATCH 1/2] KVM: x86: emulate fxsave and fxrstor

[Radim KrÄmÃÅ]

2016-10-26 20:17-0400, Bandan Das:
> Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> writes:
> ...
>> +static int check_fxsr(struct x86_emulate_ctxt *ctxt)
>> +{
>> +	u32 eax = 1, ebx, ecx = 0, edx;
>> +
>> +	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
>> +	if (!(edx & FFL(FXSR)))
>> +		return emulate_ud(ctxt);
>> +
>> +	if (ctxt->ops->get_cr(ctxt, 0) & (X86_CR0_TS | X86_CR0_EM))
>> +		return emulate_nm(ctxt);
>> +
>> +	return X86EMUL_CONTINUE;
>> +}
>> +
>> +/*
>> + * FXSAVE and FXRSTOR have 3 different formats depending on execution mode,
>> + *  1) non-64-bit mode
>> + *  2) 64-bit mode with REX.W prefix
>> + *  3) 64-bit mode without REX.W prefix
>> + *
>> + * Emulation uses (3) for for (1) mode because only the number of XMM registers
>> + * is different.
>> + */
| [...]
>> +
>> +static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
>> +{
>> +	char fx_state[512] __aligned(16);
>> +	int rc;
>> +
>> +	rc = check_fxsr(ctxt);
> 
> Is this check enough here ? What I mean is that is it possible that the memory
> image that is read from has data in an invalid format/corrupt or is that irrelevant ?

No, it is not enough, v2 will need testing on bare metal. :)

Nadav mentioned that MXCSR could thrown #GP when setting bits 16-32.

And there are actually 4 different formats: 16 bit mode has only 16 bit
FIP, and other 16 bits are reserved, but KVM's fxrstor would be loading
all 32 bits, so the reserved upper 16 should be cleared beforehand.
The structure has a lot of reserved fields, but they should just be
ignored by the CPU.

Did you notice other problems?

Thanks.