[PATCH 0/3] kvm-s390: three kernel fixes

[PATCH 0/3] kvm-s390: three kernel fixes

[Christian Borntraeger]

Hello Avi,

here are small fixes for KVM on s390:

1. Fix printk on SIGP set arch
2. Fix problem state check for b2 intercepts
3. Fix SIGP set prefix ioctl

I dont think any of the fixes is critical, but patch 1 allows a malicious 
guest to flood the host dmesg. Therefore, I want to push patch 1 for 2.6.29 
and patches 2 + 3 for the next merge window. 

Christian

[PATCH 1/3] kvm-s390: Fix printk on SIGP set arch

[Christian Borntraeger]

From: Christian Borntraeger <borntraeger@de.ibm.com>

KVM on s390 does not support the ESA/390 architecture. We refuse to 
change the architecture mode and print a warning. While testing a
crashme for kvm, I spotted two problems with the printk:

o A malicious can flood host dmesg
o There was no newline at the end of the printk

This patch fixes both problems.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/sigp.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: kvm/arch/s390/kvm/sigp.c
===================================================================
--- kvm.orig/arch/s390/kvm/sigp.c
+++ kvm/arch/s390/kvm/sigp.c
@@ -153,8 +153,9 @@ static int __sigp_set_arch(struct kvm_vc
 
 	switch (parameter & 0xff) {
 	case 0:
-		printk(KERN_WARNING "kvm: request to switch to ESA/390 mode"
-							" not supported");
+		if (printk_ratelimit())
+			printk(KERN_WARNING "kvm: request to switch to ESA/390"
+					    " mode not supported\n");
 		rc = 3; /* not operational */
 		break;
 	case 1:

Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch

[Heiko Carstens]

On Thu, 22 Jan 2009 10:27:38 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> KVM on s390 does not support the ESA/390 architecture. We refuse to 
> change the architecture mode and print a warning. While testing a
> crashme for kvm, I spotted two problems with the printk:
> 
> o A malicious can flood host dmesg
> o There was no newline at the end of the printk
> 
> This patch fixes both problems.
[...]
> -		printk(KERN_WARNING "kvm: request to switch to ESA/390 mode"
> -							" not supported");
> +		if (printk_ratelimit())
> +			printk(KERN_WARNING "kvm: request to switch to ESA/390"
> +					    " mode not supported\n");

Why don't you just remove the printk? IMHO it's rather pointless.

Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch

[Carsten Otte]

Am Thu, 22 Jan 2009 12:17:07 +0100
schrieb heicars2@linux.vnet.ibm.com:
> Why don't you just remove the printk? IMHO it's rather pointless.
It is'nt: It infoms the user why his guest is going to crash
even though it has performed an operation that is perfectly
legal according to the spec.

Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch

[Heiko Carstens]

On Thu, 22 Jan 2009 12:26:11 +0100
Carsten Otte <cotte@de.ibm.com> wrote:

> Am Thu, 22 Jan 2009 12:17:07 +0100
> schrieb heicars2@linux.vnet.ibm.com:
> > Why don't you just remove the printk? IMHO it's rather pointless.
> It is'nt: It infoms the user why his guest is going to crash
> even though it has performed an operation that is perfectly
> legal according to the spec.

If you have hundreds of guests running, how do you get the connection
from this message to a specific user process?

Informing a user process that it did something that isn't allowed or
supported is usually done by returning an appropriate return code.

Also, if you have one "evil" process and hit the prink_limit the
messages for all other processes will likely be lost anyway.

Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch

[Avi Kivity]

Heiko Carstens wrote:
> On Thu, 22 Jan 2009 12:26:11 +0100
> Carsten Otte <cotte@de.ibm.com> wrote:
>
>   
>> Am Thu, 22 Jan 2009 12:17:07 +0100
>> schrieb heicars2@linux.vnet.ibm.com:
>>     
>>> Why don't you just remove the printk? IMHO it's rather pointless.
>>>       
>> It is'nt: It infoms the user why his guest is going to crash
>> even though it has performed an operation that is perfectly
>> legal according to the spec.
>>     
>
> If you have hundreds of guests running, how do you get the connection
> from this message to a specific user process?
>
> Informing a user process that it did something that isn't allowed or
> supported is usually done by returning an appropriate return code.
>   

Right, either inject an exception to the guest (if appropriate for the 
arch), or return -ESOMETHING from ioctl(KVM_RUN).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch

[Carsten Otte]

Am Thu, 22 Jan 2009 13:58:57 +0200
schrieb Avi Kivity <avi@redhat.com>:
> Right, either inject an exception to the guest (if appropriate for the 
> arch), or return -ESOMETHING from ioctl(KVM_RUN).
Yea that's what we do. We let userland handle the intercept,
and there we let the guest die gracefully with a message.

so long,
Carsten

[PATCH 1/3 v2] kvm-s390: Fix printk on SIGP set arch

[Christian Borntraeger]

Am Thursday 22 January 2009 12:58:57 schrieb Avi Kivity:
> Right, either inject an exception to the guest (if appropriate for the
> arch), or return -ESOMETHING from ioctl(KVM_RUN).

Ok. What about:

[PATCH] kvm-s390: fix printk on SIGP set arch

From: Christian Borntraeger <borntraeger@de.ibm.com>
Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>

KVM on s390 does not support the ESA/390 architecture. We refuse to 
change the architecture mode and print a warning. This patch removes
the printk for several reasons:

o A malicious can flood host dmesg
o The old message had no newline
o there is no connection between the message and the failing guest 

This patch simply removes the printk. We already set the condition
code to 3 - the guest knows that something went wrong.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/sigp.c |    2 --
 1 file changed, 2 deletions(-)

Index: kvm/arch/s390/kvm/sigp.c
===================================================================
--- kvm.orig/arch/s390/kvm/sigp.c
+++ kvm/arch/s390/kvm/sigp.c
@@ -153,8 +153,6 @@ static int __sigp_set_arch(struct kvm_vc
 
 	switch (parameter & 0xff) {
 	case 0:
-		printk(KERN_WARNING "kvm: request to switch to ESA/390 mode"
-							" not supported");
 		rc = 3; /* not operational */
 		break;
 	case 1:

Re: [PATCH 1/3 v2] kvm-s390: Fix printk on SIGP set arch

[Avi Kivity]

Christian Borntraeger wrote:
> Am Thursday 22 January 2009 12:58:57 schrieb Avi Kivity:
>   
>> Right, either inject an exception to the guest (if appropriate for the
>> arch), or return -ESOMETHING from ioctl(KVM_RUN).
>>     
>
> Ok. What about:
>
> [PATCH] kvm-s390: fix printk on SIGP set arch
>
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>
> KVM on s390 does not support the ESA/390 architecture. We refuse to 
> change the architecture mode and print a warning. This patch removes
> the printk for several reasons:
>
> o A malicious can flood host dmesg
> o The old message had no newline
> o there is no connection between the message and the failing guest 
>
> This patch simply removes the printk. We already set the condition
> code to 3 - the guest knows that something went wrong.
>   

Applied (with the other patches), thanks.  Please include a printk() 
patch in every s390 patchset so people can participate in the review and 
discussion.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch

[Amit Shah]

On (Thu) Jan 22 2009 [10:27:38], Christian Borntraeger wrote:
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> KVM on s390 does not support the ESA/390 architecture. We refuse to 
> change the architecture mode and print a warning. While testing a
> crashme for kvm, I spotted two problems with the printk:
> 
> o A malicious can flood host dmesg
> o There was no newline at the end of the printk
> 
> This patch fixes both problems.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/sigp.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: kvm/arch/s390/kvm/sigp.c
> ===================================================================
> --- kvm.orig/arch/s390/kvm/sigp.c
> +++ kvm/arch/s390/kvm/sigp.c
> @@ -153,8 +153,9 @@ static int __sigp_set_arch(struct kvm_vc
>  
>  	switch (parameter & 0xff) {
>  	case 0:
> -		printk(KERN_WARNING "kvm: request to switch to ESA/390 mode"
> -							" not supported");
> +		if (printk_ratelimit())
> +			printk(KERN_WARNING "kvm: request to switch to ESA/390"
> +					    " mode not supported\n");

Breaking printk lines isn't encouraged just to keep the column width at
80. It makes grepping the sources for the source of the message
difficult to find.

[PATCH 2/3] kvm-s390: Fix problem state check for b2 intercepts

[Christian Borntraeger]

From: Christian Borntraeger <borntraeger@de.ibm.com>

The kernel handles some priviledged instruction exits. While I was
unable to trigger such an exit from guest userspace, the code should
check for supervisor state before emulating a priviledged instruction.

I also renamed kvm_s390_handle_priv to kvm_s390_handle_b2. After all
there are non priviledged b2 instructions like stck (store clock).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/intercept.c |    2 +-
 arch/s390/kvm/kvm-s390.h  |    2 +-
 arch/s390/kvm/priv.c      |   18 +++++++++++++++---
 3 files changed, 17 insertions(+), 5 deletions(-)

Index: kvm/arch/s390/kvm/intercept.c
===================================================================
--- kvm.orig/arch/s390/kvm/intercept.c
+++ kvm/arch/s390/kvm/intercept.c
@@ -103,7 +103,7 @@ static int handle_lctl(struct kvm_vcpu *
 static intercept_handler_t instruction_handlers[256] = {
 	[0x83] = kvm_s390_handle_diag,
 	[0xae] = kvm_s390_handle_sigp,
-	[0xb2] = kvm_s390_handle_priv,
+	[0xb2] = kvm_s390_handle_b2,
 	[0xb7] = handle_lctl,
 	[0xeb] = handle_lctlg,
 };
Index: kvm/arch/s390/kvm/kvm-s390.h
===================================================================
--- kvm.orig/arch/s390/kvm/kvm-s390.h
+++ kvm/arch/s390/kvm/kvm-s390.h
@@ -50,7 +50,7 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu
 int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code);
 
 /* implemented in priv.c */
-int kvm_s390_handle_priv(struct kvm_vcpu *vcpu);
+int kvm_s390_handle_b2(struct kvm_vcpu *vcpu);
 
 /* implemented in sigp.c */
 int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
Index: kvm/arch/s390/kvm/priv.c
===================================================================
--- kvm.orig/arch/s390/kvm/priv.c
+++ kvm/arch/s390/kvm/priv.c
@@ -304,12 +304,24 @@ static intercept_handler_t priv_handlers
 	[0xb1] = handle_stfl,
 };
 
-int kvm_s390_handle_priv(struct kvm_vcpu *vcpu)
+int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
 {
 	intercept_handler_t handler;
 
+	/*
+	 * a lot of B2 instructions are priviledged. We first check for
+	 * the priviledges ones, that we can handle in the kernel. If the
+	 * kernel can handle this instruction, we check for the problem
+	 * state bit and (a) handle the instruction or (b) send a code 2
+	 * program check.
+	 * Anything else goes to userspace.*/
 	handler = priv_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
-	if (handler)
-		return handler(vcpu);
+	if (handler) {
+		if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+			return kvm_s390_inject_program_int(vcpu,
+						   PGM_PRIVILEGED_OPERATION);
+		else
+			return handler(vcpu);
+	}
 	return -ENOTSUPP;
 }

[PATCH 3/3] kvm-s390: Fix SIGP set prefix ioctl

[Christian Borntraeger]

From: Christian Borntraeger <borntraeger@de.ibm.com>

This patch fixes the SET PREFIX interrupt if triggered by userspace.
Until now, it was not necessary, but life migration will need it. In
addition, it helped me creating SMP support for my kvm_crashme tool
(lets kvm execute random guest memory content).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/interrupt.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: kvm/arch/s390/kvm/interrupt.c
===================================================================
--- kvm.orig/arch/s390/kvm/interrupt.c
+++ kvm/arch/s390/kvm/interrupt.c
@@ -555,9 +555,14 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu
 		VCPU_EVENT(vcpu, 3, "inject: program check %d (from user)",
 			   s390int->parm);
 		break;
+	case KVM_S390_SIGP_SET_PREFIX:
+		inti->prefix.address = s390int->parm;
+		inti->type = s390int->type;
+		VCPU_EVENT(vcpu, 3, "inject: set prefix to %x (from user)",
+			   s390int->parm);
+		break;
 	case KVM_S390_SIGP_STOP:
 	case KVM_S390_RESTART:
-	case KVM_S390_SIGP_SET_PREFIX:
 	case KVM_S390_INT_EMERGENCY:
 		VCPU_EVENT(vcpu, 3, "inject: type %x", s390int->type);
 		inti->type = s390int->type;