[2/2] kvm: x86: Disallow invalid exceptions from userspace
diff mbox series

Message ID 20181012180429.259916-2-jmattson@google.com
State New
Headers show
Series
  • [1/2] kvm: x86: Bounds-check argument to x86_exception_has_error_code
Related show

Commit Message

Jim Mattson Oct. 12, 2018, 6:04 p.m. UTC
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(+)

Comments

Marc Orr Oct. 15, 2018, 1:46 p.m. UTC | #1
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>
Jim Mattson Oct. 15, 2018, 4:11 p.m. UTC | #2
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.

Patch
diff mbox series

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;
 }