From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:45058 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752348AbbJZPna (ORCPT ); Mon, 26 Oct 2015 11:43:30 -0400 Subject: Re: [PATCH] KVM: x86: fix RSM into 64-bit protected mode, round 2 To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= References: <1445636635-32260-1-git-send-email-lersek@redhat.com> <20151026153715.GA31158@potion.brq.redhat.com> Cc: kvm@vger.kernel.org, Paolo Bonzini , Jordan Justen , Michael Kinney , stable@vger.kernel.org From: Laszlo Ersek Message-ID: <562E4A1D.7000900@redhat.com> Date: Mon, 26 Oct 2015 16:43:25 +0100 MIME-Version: 1.0 In-Reply-To: <20151026153715.GA31158@potion.brq.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: On 10/26/15 16:37, Radim Krčmář wrote: > 2015-10-23 23:43+0200, Laszlo Ersek: >> Commit b10d92a54dac ("KVM: x86: fix RSM into 64-bit protected mode") >> reordered the rsm_load_seg_64() and rsm_enter_protected_mode() calls, >> relative to each other. The argument that said commit made was correct, >> however putting rsm_enter_protected_mode() first whole-sale violated the >> following (correct) invariant from em_rsm(): >> >> * Get back to real mode, to prepare a safe state in which to load >> * CR0/CR3/CR4/EFER. Also this will ensure that addresses passed >> * to read_std/write_std are not virtual. > > Nice catch. > >> Namely, rsm_enter_protected_mode() may re-enable paging, *after* which >> >> rsm_load_seg_64() >> GET_SMSTATE() >> read_std() >> >> will try to interpret the (smbase + offset) address as a virtual one. This >> will result in unexpected page faults being injected to the guest in >> response to the RSM instruction. > > I think this is a good time to introduce the read_phys helper, which we > wanted to avoid with that assumption. > >> Split rsm_load_seg_64() in two parts: >> >> - The first part, rsm_stash_seg_64(), shall call GET_SMSTATE() while in >> real mode, and save the relevant state off SMRAM into an array local to >> rsm_load_state_64(). >> >> - The second part, rsm_load_seg_64(), shall occur after entering protected >> mode, but the segment details shall come from the local array, not the >> guest's SMRAM. >> >> Fixes: b10d92a54dac25a6152f1aa1ffc95c12908035ce >> Cc: Paolo Bonzini >> Cc: Radim Krčmář >> Cc: Jordan Justen >> Cc: Michael Kinney >> Cc: stable@vger.kernel.org >> Signed-off-by: Laszlo Ersek >> --- > > The code would be cleaner if we had a different approach, but this works > too and is safer for stable. In case you prefer to leave the rewrite for > a future victim, It's hard to express how much I prefer that. > > Reviewed-by: Radim Krčmář > Thank you! Laszlo