| Message ID | 20230727182939.2050744-1-hca@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: fix sthyi error handling | expand |
Am 27.07.23 um 20:29 schrieb Heiko Carstens: > Commit 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info") > added cache handling for store hypervisor info. This also changed the > possible return code for sthyi_fill(). > > Instead of only returning a condition code like the sthyi instruction would > do, it can now also return a negative error value (-ENOMEM). handle_styhi() > was not changed accordingly. In case of an error, the negative error value > would incorrectly injected into the guest PSW. > > Add proper error handling to prevent this, and update the comment which > describes the possible return values of sthyi_fill(). To me it looks like this can only happen if page allocation fails? This should not happen in normal cases (and return -ENOMEM would likely kill the guest as QEMU would stop). But if it happens we better stop. Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> > > Fixes: 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info") > Signed-off-by: Heiko Carstens <hca@linux.ibm.com> > --- > arch/s390/kernel/sthyi.c | 6 +++--- > arch/s390/kvm/intercept.c | 9 ++++++--- > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c > index 4d141e2c132e..2ea7f208f0e7 100644 > --- a/arch/s390/kernel/sthyi.c > +++ b/arch/s390/kernel/sthyi.c > @@ -459,9 +459,9 @@ static int sthyi_update_cache(u64 *rc) > * > * Fills the destination with system information returned by the STHYI > * instruction. The data is generated by emulation or execution of STHYI, > - * if available. The return value is the condition code that would be > - * returned, the rc parameter is the return code which is passed in > - * register R2 + 1. > + * if available. The return value is either a negative error value or > + * the condition code that would be returned, the rc parameter is the > + * return code which is passed in register R2 + 1. > */ > int sthyi_fill(void *dst, u64 *rc) > { > diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c > index 954d39adf85c..341abafb96e4 100644 > --- a/arch/s390/kvm/intercept.c > +++ b/arch/s390/kvm/intercept.c > @@ -389,8 +389,8 @@ static int handle_partial_execution(struct kvm_vcpu *vcpu) > */ > int handle_sthyi(struct kvm_vcpu *vcpu) > { > - int reg1, reg2, r = 0; > - u64 code, addr, cc = 0, rc = 0; > + int reg1, reg2, cc = 0, r = 0; > + u64 code, addr, rc = 0; > struct sthyi_sctns *sctns = NULL; > > if (!test_kvm_facility(vcpu->kvm, 74)) > @@ -421,7 +421,10 @@ int handle_sthyi(struct kvm_vcpu *vcpu) > return -ENOMEM; > > cc = sthyi_fill(sctns, &rc); > - > + if (cc < 0) { > + free_page((unsigned long)sctns); > + return cc; > + } > out: > if (!cc) { > if (kvm_s390_pv_cpu_is_protected(vcpu)) {
On Fri, Jul 28, 2023 at 09:28:58AM +0200, Christian Borntraeger wrote: > Am 27.07.23 um 20:29 schrieb Heiko Carstens: > > Commit 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info") > > added cache handling for store hypervisor info. This also changed the > > possible return code for sthyi_fill(). > > > > Instead of only returning a condition code like the sthyi instruction would > > do, it can now also return a negative error value (-ENOMEM). handle_styhi() > > was not changed accordingly. In case of an error, the negative error value > > would incorrectly injected into the guest PSW. > > > > Add proper error handling to prevent this, and update the comment which > > describes the possible return values of sthyi_fill(). > > To me it looks like this can only happen if page allocation fails? This should > not happen in normal cases (and return -ENOMEM would likely kill the guest as > QEMU would stop). > But if it happens we better stop. Yes, no reason for any stable backports. But things might change in the future, so we better have correct error handling in place.
Am 28.07.23 um 11:14 schrieb Heiko Carstens: > On Fri, Jul 28, 2023 at 09:28:58AM +0200, Christian Borntraeger wrote: >> Am 27.07.23 um 20:29 schrieb Heiko Carstens: >>> Commit 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info") >>> added cache handling for store hypervisor info. This also changed the >>> possible return code for sthyi_fill(). >>> >>> Instead of only returning a condition code like the sthyi instruction would >>> do, it can now also return a negative error value (-ENOMEM). handle_styhi() >>> was not changed accordingly. In case of an error, the negative error value >>> would incorrectly injected into the guest PSW. >>> >>> Add proper error handling to prevent this, and update the comment which >>> describes the possible return values of sthyi_fill(). >> >> To me it looks like this can only happen if page allocation fails? This should >> not happen in normal cases (and return -ENOMEM would likely kill the guest as >> QEMU would stop). >> But if it happens we better stop. > > Yes, no reason for any stable backports. But things might change in the > future, so we better have correct error handling in place. Feel free to carry via the s390 tree.
diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c index 4d141e2c132e..2ea7f208f0e7 100644 --- a/arch/s390/kernel/sthyi.c +++ b/arch/s390/kernel/sthyi.c @@ -459,9 +459,9 @@ static int sthyi_update_cache(u64 *rc) * * Fills the destination with system information returned by the STHYI * instruction. The data is generated by emulation or execution of STHYI, - * if available. The return value is the condition code that would be - * returned, the rc parameter is the return code which is passed in - * register R2 + 1. + * if available. The return value is either a negative error value or + * the condition code that would be returned, the rc parameter is the + * return code which is passed in register R2 + 1. */ int sthyi_fill(void *dst, u64 *rc) { diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c index 954d39adf85c..341abafb96e4 100644 --- a/arch/s390/kvm/intercept.c +++ b/arch/s390/kvm/intercept.c @@ -389,8 +389,8 @@ static int handle_partial_execution(struct kvm_vcpu *vcpu) */ int handle_sthyi(struct kvm_vcpu *vcpu) { - int reg1, reg2, r = 0; - u64 code, addr, cc = 0, rc = 0; + int reg1, reg2, cc = 0, r = 0; + u64 code, addr, rc = 0; struct sthyi_sctns *sctns = NULL; if (!test_kvm_facility(vcpu->kvm, 74)) @@ -421,7 +421,10 @@ int handle_sthyi(struct kvm_vcpu *vcpu) return -ENOMEM; cc = sthyi_fill(sctns, &rc); - + if (cc < 0) { + free_page((unsigned long)sctns); + return cc; + } out: if (!cc) { if (kvm_s390_pv_cpu_is_protected(vcpu)) {
Commit 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info") added cache handling for store hypervisor info. This also changed the possible return code for sthyi_fill(). Instead of only returning a condition code like the sthyi instruction would do, it can now also return a negative error value (-ENOMEM). handle_styhi() was not changed accordingly. In case of an error, the negative error value would incorrectly injected into the guest PSW. Add proper error handling to prevent this, and update the comment which describes the possible return values of sthyi_fill(). Fixes: 9fb6c9b3fea1 ("s390/sthyi: add cache to store hypervisor info") Signed-off-by: Heiko Carstens <hca@linux.ibm.com> --- arch/s390/kernel/sthyi.c | 6 +++--- arch/s390/kvm/intercept.c | 9 ++++++--- 2 files changed, 9 insertions(+), 6 deletions(-)