From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37895) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGPGC-0003vx-Kt for qemu-devel@nongnu.org; Tue, 25 Sep 2012 03:08:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TGPG6-0006UF-Fu for qemu-devel@nongnu.org; Tue, 25 Sep 2012 03:08:48 -0400 Date: Tue, 25 Sep 2012 17:05:50 +1000 From: David Gibson Message-ID: <20120925070550.GJ9800@truffula.fritz.box> References: <1348124922-24263-1-git-send-email-david@gibson.dropbear.id.au> <1348124922-24263-2-git-send-email-david@gibson.dropbear.id.au> <84A7A0D1-BC0F-46E1-A4DC-44A39B6A1D4F@suse.de> <20120920115332.GJ24695@truffula.fritz.box> <20120921002215.GL24695@truffula.fritz.box> <90A6149A-3F77-45D3-B4B1-AC3E7D4E17A3@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <90A6149A-3F77-45D3-B4B1-AC3E7D4E17A3@suse.de> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] pseries: Synchronize qemu and KVM state on hypercalls List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, qemu-stable@nongnu.org On Mon, Sep 24, 2012 at 04:27:20PM +0200, Alexander Graf wrote: > > On 21.09.2012, at 02:22, David Gibson wrote: > > > On Thu, Sep 20, 2012 at 02:44:26PM +0200, Alexander Graf wrote: > >> > >> On 20.09.2012, at 13:53, David Gibson wrote: > >> > >>> On Thu, Sep 20, 2012 at 09:38:58AM +0200, Alexander Graf wrote: > >>>> > >>>> On 20.09.2012, at 09:08, David Gibson wrote: > >>>> > >>>>> Currently the KVM exit path for PAPR hypercalls does not synchronize the > >>>>> qemu cpu state with the KVM state. Mostly this works, because the actual > >>>>> hypercall arguments and return values are explicitly passed through the > >>>>> kvm_run structure. However, the hypercall path includes a privilege check, > >>>>> to ensure that only the guest kernel can invoke hypercalls, not the guest > >>>>> userspace. Because of the lack of sync, this privilege check will use an > >>>>> out of date copy of the MSR, which could lead either to guest userspace > >>>>> being able to invoke hypercalls (a security hole for the guest) or to the > >>>>> guest kernel being incorrectly refused privilege leading to various other > >>>>> failures. > >>>>> > >>>>> This patch fixes the bug by forcing a synchronization on the hypercall exit > >>>>> path. This does mean we have a potentially quite expensive get and set of > >>>>> the state, however performance critical hypercalls are generally already > >>>>> implemented inside KVM so this probably won't matter. If it is a > >>>>> performance problem we can optimize it later by having the kernel perform > >>>>> the privilege check. That will need a new capability, however, since qemu > >>>>> will still need the privilege check for older kernels. > >>>>> > >>>>> Signed-off-by: David Gibson > >>>> > >>>> I would actually prefer to see that one fixed in kernel space. > >>> > >>> That's a better fix, but we can't fix it purely in the kernel, because > >>> there are existing released kernels that don't do the privilege check. > >> > >> There are security flaws fixed through -stable updates in the kernel > >> any day, why should this one be handled differently? > > > > From the kernel's point of view, this is not obviously a security bug > > - it passes a hypercall it doesn't know how to handle to qemu, qemu > > handles it incorrectly. > > > > And in any case, even if you do consider it a kernel security bug, > > there's no reason that qemu should just allow that bug to appear when > > it's capable of working around buggy kernels in a way that closes the > > security hole. > > This is the code in the HV kernel side: > > case BOOK3S_INTERRUPT_SYSCALL: > { > /* hcall - punt to userspace */ > int i; > > if (vcpu->arch.shregs.msr & MSR_PR) { > /* sc 1 from userspace - reflect to guest syscall */ > kvmppc_book3s_queue_irqprio(vcpu, BOOK3S_INTERRUPT_SYSCALL); > r = RESUME_GUEST; > break; > } > run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3); > for (i = 0; i < 9; ++i) > run->papr_hcall.args[i] = kvmppc_get_gpr(vcpu, 4 + i); > run->exit_reason = KVM_EXIT_PAPR_HCALL; > vcpu->arch.hcall_needed = 1; > r = RESUME_HOST; > break; > } > > So it already handles hypercalls in user space and deflects them > back. Everyone's happy :). Ah, so it does. I was mistaken. > The only outstanding bug is that QEMU shouldn't interpret env->msr > when handling hypercalls from KVM, since these are already > guaranteed to be checked and MSR in QEMU does not reflect the > current MSR in the vcpu, so we might end up rejecting hypercalls by > accident. I've written a suitable patch, just needs a little more testing and I'll send it out. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson