| Message ID | 20181012180429.259916-2-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] kvm: x86: Bounds-check argument to x86_exception_has_error_code | expand |
On Fri, Oct 12, 2018 at 11:05 AM Jim Mattson <jmattson@google.com> wrote: > > Userspace should not be able to inject an invalid exception into a kvm > guest via KVM_SET_VCPU_EVENTS, nor should it be able to convert a > valid pending exception to an invalid one via KVM_SET_SREGS. > > In particular, only certain exceptions deliver an error code in > protected mode, and no exception delivers an error code in > real-address mode. > > Reported-by: syzbot <syzkaller@googlegroups.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/x86.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 4ab569171ad1..e060c8254cc3 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3423,6 +3423,14 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, > vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) > return -EINVAL; > > + if (events->exception.injected) { > + bool has_error_code = is_protmode(vcpu) && > + x86_exception_has_error_code(events->exception.nr); > + > + if (!!events->exception.has_error_code != has_error_code) Is !! necessary here? It's not used below, when extracting has_error_code from vcpu->arch.exception. Also, it would be nice to order these two if statements consistently (i.e., put the local variable, has_error_code, on the left side or right side of the != in both places). > + return -EINVAL; > + } > + > process_nmi(vcpu); > vcpu->arch.exception.injected = false; > vcpu->arch.exception.pending = events->exception.injected; > @@ -8170,6 +8178,14 @@ static int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > return -EINVAL; > } > > + if (vcpu->arch.exception.injected || vcpu->arch.exception.pending) { > + bool has_error_code = (sregs->cr0 & X86_CR0_PE) && > + x86_exception_has_error_code(vcpu->arch.exception.nr); > + > + if (has_error_code != vcpu->arch.exception.has_error_code) > + return -EINVAL; > + } > + > return 0; > } > > -- > 2.19.1.331.ge82ca0e54c-goog > Reviewed-by: Marc Orr <marcorr@google.com>
On Mon, Oct 15, 2018 at 6:46 AM, Marc Orr <marcorr@google.com> wrote: > On Fri, Oct 12, 2018 at 11:05 AM Jim Mattson <jmattson@google.com> wrote: >> + if (events->exception.injected) { >> + bool has_error_code = is_protmode(vcpu) && >> + x86_exception_has_error_code(events->exception.nr); >> + >> + if (!!events->exception.has_error_code != has_error_code) > > Is !! necessary here? It's not used below, when extracting > has_error_code from vcpu->arch.exception. Also, it would be nice to > order these two if statements consistently (i.e., put the local > variable, has_error_code, on the left side or right side of the != in > both places). The difference is that events->exception.has_error_code is of type unsigned char, whereas vcpu->arch.exception.has_error_code is of type bool. Alternatively, this could be written as: if ((bool)events->exception.has_error_code != has_error_code) However, Linux convention appears to prefer !! over (bool). I'll reorder the comparisons in v2.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 4ab569171ad1..e060c8254cc3 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3423,6 +3423,14 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) return -EINVAL; + if (events->exception.injected) { + bool has_error_code = is_protmode(vcpu) && + x86_exception_has_error_code(events->exception.nr); + + if (!!events->exception.has_error_code != has_error_code) + return -EINVAL; + } + process_nmi(vcpu); vcpu->arch.exception.injected = false; vcpu->arch.exception.pending = events->exception.injected; @@ -8170,6 +8178,14 @@ static int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) return -EINVAL; } + if (vcpu->arch.exception.injected || vcpu->arch.exception.pending) { + bool has_error_code = (sregs->cr0 & X86_CR0_PE) && + x86_exception_has_error_code(vcpu->arch.exception.nr); + + if (has_error_code != vcpu->arch.exception.has_error_code) + return -EINVAL; + } + return 0; }
Userspace should not be able to inject an invalid exception into a kvm guest via KVM_SET_VCPU_EVENTS, nor should it be able to convert a valid pending exception to an invalid one via KVM_SET_SREGS. In particular, only certain exceptions deliver an error code in protected mode, and no exception delivers an error code in real-address mode. Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: Jim Mattson <jmattson@google.com> --- arch/x86/kvm/x86.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)