Re: [PATCH v2] KVM: s390: Test for bad access register and size at the start of S390_MEM_OP
On 29.08.19 14:25, Thomas Huth wrote: > If the KVM_S390_MEM_OP ioctl is called with an access register >= 16, > then there is certainly a bug in the calling userspace application. > We check for wrong access registers, but only if the vCPU was already > in the access register mode before (i.e. the SIE block has recorded > it). The check is also buried somewhere deep in the calling chain (in > the function ar_translation()), so this is somewhat hard to find. > > It's better to always report an error to the userspace in case this > field is set wrong, and it's safer in the KVM code if we block wrong > values here early instead of relying on a check somewhere deep down > the calling chain, so let's add another check to kvm_s390_guest_mem_op() > directly. > > We also should check that the "size" is non-zero here (thanks to Janosch > Frank for the hint!). If we do not check the size, we could call vmalloc() > with this 0 value, and this will cause a kernel warning. > > Signed-off-by: Thomas Huth > --- > v2: Check mop->size to be non-zero > > arch/s390/kvm/kvm-s390.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index f329dcb3f44c..49d779ae 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4255,7 +4255,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, > const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION > | KVM_S390_MEMOP_F_CHECK_ONLY; > > - if (mop->flags & ~supported_flags) > + if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size) > return -EINVAL; > > if (mop->size > MEM_OP_MAX_SIZE) > Reviewed-by: David Hildenbrand -- Thanks, David / dhildenb
Re: [PATCH v2] KVM: s390: Test for bad access register and size at the start of S390_MEM_OP
On 29.08.19 14:40, Cornelia Huck wrote: > On Thu, 29 Aug 2019 14:25:17 +0200 > Thomas Huth wrote: > >> If the KVM_S390_MEM_OP ioctl is called with an access register >= 16, >> then there is certainly a bug in the calling userspace application. >> We check for wrong access registers, but only if the vCPU was already >> in the access register mode before (i.e. the SIE block has recorded >> it). The check is also buried somewhere deep in the calling chain (in >> the function ar_translation()), so this is somewhat hard to find. >> >> It's better to always report an error to the userspace in case this >> field is set wrong, and it's safer in the KVM code if we block wrong >> values here early instead of relying on a check somewhere deep down >> the calling chain, so let's add another check to kvm_s390_guest_mem_op() >> directly. >> >> We also should check that the "size" is non-zero here (thanks to Janosch >> Frank for the hint!). If we do not check the size, we could call vmalloc() >> with this 0 value, and this will cause a kernel warning. >> >> Signed-off-by: Thomas Huth I will add cc stable. Thanks applied. >> --- >> v2: Check mop->size to be non-zero >> >> arch/s390/kvm/kvm-s390.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index f329dcb3f44c..49d779ae 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -4255,7 +4255,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu >> *vcpu, >> const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION >> | KVM_S390_MEMOP_F_CHECK_ONLY; >> >> -if (mop->flags & ~supported_flags) >> +if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size) >> return -EINVAL; >> >> if (mop->size > MEM_OP_MAX_SIZE) > > Reviewed-by: Cornelia Huck >
Re: [PATCH v2] KVM: s390: Test for bad access register and size at the start of S390_MEM_OP
On Thu, 29 Aug 2019 14:25:17 +0200 Thomas Huth wrote: > If the KVM_S390_MEM_OP ioctl is called with an access register >= 16, > then there is certainly a bug in the calling userspace application. > We check for wrong access registers, but only if the vCPU was already > in the access register mode before (i.e. the SIE block has recorded > it). The check is also buried somewhere deep in the calling chain (in > the function ar_translation()), so this is somewhat hard to find. > > It's better to always report an error to the userspace in case this > field is set wrong, and it's safer in the KVM code if we block wrong > values here early instead of relying on a check somewhere deep down > the calling chain, so let's add another check to kvm_s390_guest_mem_op() > directly. > > We also should check that the "size" is non-zero here (thanks to Janosch > Frank for the hint!). If we do not check the size, we could call vmalloc() > with this 0 value, and this will cause a kernel warning. > > Signed-off-by: Thomas Huth > --- > v2: Check mop->size to be non-zero > > arch/s390/kvm/kvm-s390.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index f329dcb3f44c..49d779ae 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4255,7 +4255,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, > const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION > | KVM_S390_MEMOP_F_CHECK_ONLY; > > - if (mop->flags & ~supported_flags) > + if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size) > return -EINVAL; > > if (mop->size > MEM_OP_MAX_SIZE) Reviewed-by: Cornelia Huck
[PATCH v2] KVM: s390: Test for bad access register and size at the start of S390_MEM_OP
If the KVM_S390_MEM_OP ioctl is called with an access register >= 16, then there is certainly a bug in the calling userspace application. We check for wrong access registers, but only if the vCPU was already in the access register mode before (i.e. the SIE block has recorded it). The check is also buried somewhere deep in the calling chain (in the function ar_translation()), so this is somewhat hard to find. It's better to always report an error to the userspace in case this field is set wrong, and it's safer in the KVM code if we block wrong values here early instead of relying on a check somewhere deep down the calling chain, so let's add another check to kvm_s390_guest_mem_op() directly. We also should check that the "size" is non-zero here (thanks to Janosch Frank for the hint!). If we do not check the size, we could call vmalloc() with this 0 value, and this will cause a kernel warning. Signed-off-by: Thomas Huth --- v2: Check mop->size to be non-zero arch/s390/kvm/kvm-s390.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index f329dcb3f44c..49d779ae 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4255,7 +4255,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION | KVM_S390_MEMOP_F_CHECK_ONLY; - if (mop->flags & ~supported_flags) + if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size) return -EINVAL; if (mop->size > MEM_OP_MAX_SIZE) -- 2.18.1
