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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161027163457.GB11326@potion>
Date:   Thu, 27 Oct 2016 18:34:57 +0200
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     Bandan Das <bsd@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Laszlo Ersek <lersek@...hat.com>
Subject: Re: [PATCH 1/2] KVM: x86: emulate fxsave and fxrstor

2016-10-26 20:17-0400, Bandan Das:
> Radim Krčmář <rkrcmar@...hat.com> 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.