diff mbox

[2/5] KVM: s390: Add support for machine checks.

Message ID 20121219094414.GA4996@osiris.de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Heiko Carstens Dec. 19, 2012, 9:44 a.m. UTC 
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?

Comments

Christian Borntraeger Dec. 19, 2012, 10:20 a.m. UTC | #1 
On 19/12/12 10:44, Heiko Carstens wrote:
> 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?

The whole guest memory access of KVM needs to be reworked to work properly
in those corner cases. I have this on my todo list as one of things for next
year with lots of open questions that I dont want to answer before xmas.
what about in kernel intercepts? (shall we then return EFAULT for the KVM_RUN
ioctl, shall we kill the guest?.....)

We actually need to test the address for validity via the memslots (and not
via return value of copy_from/to_user) all across the s390 code.

I really want to avoid mixing this effort with the virtio-ccw patches.
So my proposal is to apply your patch below and keep Conny's patch as is.
Ok?

Christian


> From db05454b6f3f49a7a10f5a1e546917303cf94532 Mon Sep 17 00:00:00 2001
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 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 <heiko.carstens@de.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>



> ---
>  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,
> 

--
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
Heiko Carstens Dec. 19, 2012, 1:07 p.m. UTC | #2 
On Wed, Dec 19, 2012 at 11:20:18AM +0100, Christian Borntraeger wrote:
> On 19/12/12 10:44, Heiko Carstens wrote:
> > 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?
> 
> The whole guest memory access of KVM needs to be reworked to work properly
> in those corner cases. I have this on my todo list as one of things for next
> year with lots of open questions that I dont want to answer before xmas.
> what about in kernel intercepts? (shall we then return EFAULT for the KVM_RUN
> ioctl, shall we kill the guest?.....)
> 
> We actually need to test the address for validity via the memslots (and not
> via return value of copy_from/to_user) all across the s390 code.
> 
> I really want to avoid mixing this effort with the virtio-ccw patches.
> So my proposal is to apply your patch below and keep Conny's patch as is.
> Ok?

Sure. :)

--
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
diff mbox

Patch

===================


From db05454b6f3f49a7a10f5a1e546917303cf94532 Mon Sep 17 00:00:00 2001
From: Heiko Carstens <heiko.carstens@de.ibm.com>
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 <heiko.carstens@de.ibm.com>
---
 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,