From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bandan Das Subject: Re: [PATCH 1/2] KVM: x86: emulate fxsave and fxrstor Date: Thu, 27 Oct 2016 23:29:42 -0400 Message-ID: References: <20161026205014.19801-1-rkrcmar@redhat.com> <20161026205014.19801-2-rkrcmar@redhat.com> <20161027163457.GB11326@potion> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Paolo Bonzini , Laszlo Ersek To: Radim =?utf-8?B?S3LEjW3DocWZ?= Return-path: In-Reply-To: <20161027163457.GB11326@potion> ("Radim \=\?utf-8\?B\?S3LEjW0\=\?\= \=\?utf-8\?B\?w6HFmSIncw\=\=\?\= message of "Thu, 27 Oct 2016 18:34:57 +0200") Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Radim Krčmář writes: > 2016-10-26 20:17-0400, Bandan Das: >> Radim Krčmář 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. Yeah, this was what I am wondering about. Whether, there are obvious ways for the guest to abuse the path, you know, common emulator code concerns. > 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. Ok. > Did you notice other problems? Rest looks fine to me even with the #ifdefs that you don't prefer but I will be sure to take another look when you post a v2. > Thanks. > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html