From patchwork Wed Dec 19 09:44:14 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Heiko Carstens X-Patchwork-Id: 1895071 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id A6B453FC66 for ; Wed, 19 Dec 2012 09:43:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751443Ab2LSJnA (ORCPT ); Wed, 19 Dec 2012 04:43:00 -0500 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:45587 "EHLO e06smtp12.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751387Ab2LSJm5 (ORCPT ); Wed, 19 Dec 2012 04:42:57 -0500 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 19 Dec 2012 09:42:29 -0000 Received: from b06cxnps4075.portsmouth.uk.ibm.com (9.149.109.197) by e06smtp12.uk.ibm.com (192.168.101.142) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 19 Dec 2012 09:42:28 -0000 Received: from d06av01.portsmouth.uk.ibm.com (d06av01.portsmouth.uk.ibm.com [9.149.37.212]) by b06cxnps4075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qBJ9gjxl59310148; Wed, 19 Dec 2012 09:42:46 GMT Received: from d06av01.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av01.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qBJ9grgL008944; Wed, 19 Dec 2012 02:42:53 -0700 Received: from localhost (dyn-9-152-212-113.boeblingen.de.ibm.com [9.152.212.113]) by d06av01.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id qBJ9gqMX008941; Wed, 19 Dec 2012 02:42:52 -0700 Date: Wed, 19 Dec 2012 10:44:14 +0100 From: Heiko Carstens To: Cornelia Huck Cc: Marcelo Tosatti , Gleb Natapov , KVM , linux-s390 , Avi Kivity , Christian Borntraeger , Carsten Otte , Alexander Graf , Martin Schwidefsky , Sebastian Ott Subject: Re: [PATCH 2/5] KVM: s390: Add support for machine checks. Message-ID: <20121219094414.GA4996@osiris.de.ibm.com> References: <1354883425-38531-1-git-send-email-cornelia.huck@de.ibm.com> <1354883425-38531-3-git-send-email-cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1354883425-38531-3-git-send-email-cornelia.huck@de.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) x-cbid: 12121909-8372-0000-0000-000004AA7710 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Fri, Dec 07, 2012 at 01:30:22PM +0100, Cornelia Huck wrote: > + rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic); > + if (rc == -EFAULT) > + exception = 1; > + > + rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW, > + &vcpu->arch.sie_block->gpsw, sizeof(psw_t)); > + if (rc == -EFAULT) > + exception = 1; Please don't add more explicit -EFAULT checks on guest access paths. Just make this like normal user space accesses. That is return code != 0 means an error occured: rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic); if (rc) exception = 1; In fact, with the current kvm gaccess code it's even broken, since on error the guest access functions may return also -ENOMEM instead of -EFAULT, which would be ignored by your code. I addressed that with a patch when trying to clean up the guest access functions. Maybe the patch below should be merged anyway. Christian? Reviewed-by: Christian Borntraeger =================== From db05454b6f3f49a7a10f5a1e546917303cf94532 Mon Sep 17 00:00:00 2001 From: Heiko Carstens Date: Mon, 10 Sep 2012 16:36:23 +0200 Subject: s390/kvm,gaccess: fix guest access return code handling Guest access functions like copy_to/from_guest() call __guestaddr_to_user() which in turn call gmap_fault() in order to translate a guest address to a user space address. In error case __guest_addr_to_user() returns either -EFAULT or -ENOMEM. The copy_to/from_guest functions just pass these return values down to the callers. The -ENOMEM case however is problematic since there are several places which access guest memory like: rc = copy_to_guest(...); if (rc == -EFAULT) error_handling(); So in case of -ENOMEM the code assumes that the guest memory access succeeded even though it failed. This can cause guest data or state corruption. If __guestaddr_to_user() returns -ENOMEM the meaning is that a valid user space mapping exists, but there was not enough memory available when trying to build the guest mapping. In other words an out-of-memory situation occured. For normal user space accesses an out-of-memory situation causes the page fault handler to map -ENOMEM to -EFAULT (see fixup code in do_no_context()). We need to do exactly the same for the kvm gaccess functions. So __guestaddr_to_user() should just map all error codes to -EFAULT. Signed-off-by: Heiko Carstens --- arch/s390/kvm/gaccess.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h index 4703f12..84d01dd 100644 --- a/arch/s390/kvm/gaccess.h +++ b/arch/s390/kvm/gaccess.h @@ -22,13 +22,16 @@ static inline void __user *__guestaddr_to_user(struct kvm_vcpu *vcpu, unsigned long guestaddr) { unsigned long prefix = vcpu->arch.sie_block->prefix; + unsigned long uaddress; if (guestaddr < 2 * PAGE_SIZE) guestaddr += prefix; else if ((guestaddr >= prefix) && (guestaddr < prefix + 2 * PAGE_SIZE)) guestaddr -= prefix; - - return (void __user *) gmap_fault(guestaddr, vcpu->arch.gmap); + uaddress = gmap_fault(guestaddr, vcpu->arch.gmap); + if (IS_ERR_VALUE(uaddress)) + uaddress = -EFAULT; + return (void __user *)uaddress; } static inline int get_guest_u64(struct kvm_vcpu *vcpu, unsigned long guestaddr,