* [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
@ 2018-08-29 5:52 Wanpeng Li
2018-08-29 9:05 ` Liran Alon
0 siblings, 1 reply; 14+ messages in thread
From: Wanpeng Li @ 2018-08-29 5:52 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář, Liran Alon, Dan Carpenter
From: Wanpeng Li <wanpengli@tencent.com>
Dan Carpenter reported that the untrusted data returns from kvm_register_read()
results in the following static checker warning:
arch/x86/kvm/lapic.c:576 kvm_pv_send_ipi()
error: buffer underflow 'map->phys_map' 's32min-s32max'
KVM guest can easily trigger this by executing the following assembly sequence
in Ring0:
mov $10, %rax
mov $0xFFFFFFFF, %rbx
mov $0xFFFFFFFF, %rdx
mov $0, %rsi
vmcall
As this will cause KVM to execute the following code-path:
vmx_handle_exit() -> handle_vmcall() -> kvm_emulate_hypercall() -> kvm_pv_send_ipi()
which will reach out-of-bounds access.
This patch fixes it by adding a check to kvm_pv_send_ipi() against map->max_apic_id
and also checking whether or not map->phys_map[min + i] is NULL since the max_apic_id
is set according to the max apic id, however, some phys_map maybe NULL when apic id
is sparse, in addition, kvm also unconditionally set max_apic_id to 255 to reserve
enough space for any xAPIC ID.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Liran Alon <liran.alon@oracle.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
arch/x86/kvm/lapic.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0cefba2..86e933c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
rcu_read_lock();
map = rcu_dereference(kvm->arch.apic_map);
+ if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
+ goto out;
/* Bits above cluster_size are masked in the caller. */
for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ }
}
min += cluster_size;
+ if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
+ goto out;
for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ }
}
+out:
rcu_read_unlock();
return count;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 5:52 [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access Wanpeng Li
@ 2018-08-29 9:05 ` Liran Alon
2018-08-29 10:12 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Liran Alon @ 2018-08-29 9:05 UTC (permalink / raw)
To: Wanpeng Li
Cc: Linux Kernel Mailing List, kvm, Paolo Bonzini,
Radim Krčmář,
Dan Carpenter
> On 29 Aug 2018, at 8:52, Wanpeng Li <kernellwp@gmail.com> wrote:
>
> From: Wanpeng Li <wanpengli@tencent.com>
>
> Dan Carpenter reported that the untrusted data returns from kvm_register_read()
> results in the following static checker warning:
> arch/x86/kvm/lapic.c:576 kvm_pv_send_ipi()
> error: buffer underflow 'map->phys_map' 's32min-s32max'
>
> KVM guest can easily trigger this by executing the following assembly sequence
> in Ring0:
>
> mov $10, %rax
> mov $0xFFFFFFFF, %rbx
> mov $0xFFFFFFFF, %rdx
> mov $0, %rsi
> vmcall
>
> As this will cause KVM to execute the following code-path:
> vmx_handle_exit() -> handle_vmcall() -> kvm_emulate_hypercall() -> kvm_pv_send_ipi()
> which will reach out-of-bounds access.
>
> This patch fixes it by adding a check to kvm_pv_send_ipi() against map->max_apic_id
> and also checking whether or not map->phys_map[min + i] is NULL since the max_apic_id
> is set according to the max apic id, however, some phys_map maybe NULL when apic id
> is sparse, in addition, kvm also unconditionally set max_apic_id to 255 to reserve
> enough space for any xAPIC ID.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Liran Alon <liran.alon@oracle.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
> arch/x86/kvm/lapic.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 0cefba2..86e933c 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> rcu_read_lock();
> map = rcu_dereference(kvm->arch.apic_map);
>
> + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> + goto out;
I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
But that’s just a matter of taste :)
> /* Bits above cluster_size are masked in the caller. */
> for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, &irq, NULL);
> + if (map->phys_map[min + i]) {
> + vcpu = map->phys_map[min + i]->vcpu;
> + count += kvm_apic_set_irq(vcpu, &irq, NULL);
> + }
> }
>
> min += cluster_size;
> + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
> + goto out;
> for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, &irq, NULL);
> + if (map->phys_map[min + i]) {
> + vcpu = map->phys_map[min + i]->vcpu;
> + count += kvm_apic_set_irq(vcpu, &irq, NULL);
> + }
> }
>
> +out:
> rcu_read_unlock();
> return count;
> }
> --
> 2.7.4
>
Reviewed-By: Liran Alon <liran.alon@oracle.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 9:05 ` Liran Alon
@ 2018-08-29 10:12 ` Dan Carpenter
2018-08-29 10:18 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2018-08-29 10:12 UTC (permalink / raw)
To: Liran Alon
Cc: Wanpeng Li, Linux Kernel Mailing List, kvm, Paolo Bonzini,
Radim Krčmář
On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 0cefba2..86e933c 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > rcu_read_lock();
> > map = rcu_dereference(kvm->arch.apic_map);
> >
> > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > + goto out;
>
> I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> But that’s just a matter of taste :)
That's an integer overflow.
But I do prefer to put the variable on the left. The truth is that some
Smatch checks just ignore code which is backwards written because
otherwise you have to write duplicate code and the most code is written
with the variable on the left.
if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
Shouldn't this be >= instead? It looks off by one.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 10:12 ` Dan Carpenter
@ 2018-08-29 10:18 ` Dan Carpenter
2018-08-29 10:23 ` Wanpeng Li
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2018-08-29 10:18 UTC (permalink / raw)
To: Liran Alon
Cc: Wanpeng Li, Linux Kernel Mailing List, kvm, Paolo Bonzini,
Radim Krčmář
On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
> On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 0cefba2..86e933c 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > rcu_read_lock();
> > > map = rcu_dereference(kvm->arch.apic_map);
> > >
> > > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > > + goto out;
> >
> > I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> > But that’s just a matter of taste :)
>
> That's an integer overflow.
>
> But I do prefer to put the variable on the left. The truth is that some
> Smatch checks just ignore code which is backwards written because
> otherwise you have to write duplicate code and the most code is written
> with the variable on the left.
>
> if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
Wait, the (s32) cast doesn't make sense. We want negative min values to
be treated as invalid.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 10:18 ` Dan Carpenter
@ 2018-08-29 10:23 ` Wanpeng Li
2018-08-29 10:29 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Wanpeng Li @ 2018-08-29 10:23 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Liran Alon, LKML, kvm, Paolo Bonzini, Radim Krcmar
On Wed, 29 Aug 2018 at 18:18, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
> > On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > > > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > index 0cefba2..86e933c 100644
> > > > --- a/arch/x86/kvm/lapic.c
> > > > +++ b/arch/x86/kvm/lapic.c
> > > > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > > rcu_read_lock();
> > > > map = rcu_dereference(kvm->arch.apic_map);
> > > >
> > > > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > > > + goto out;
> > >
> > > I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> > > But that’s just a matter of taste :)
> >
> > That's an integer overflow.
> >
> > But I do prefer to put the variable on the left. The truth is that some
> > Smatch checks just ignore code which is backwards written because
> > otherwise you have to write duplicate code and the most code is written
> > with the variable on the left.
> >
> > if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
>
> Wait, the (s32) cast doesn't make sense. We want negative min values to
> be treated as invalid.
In v2, how about:
if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
map->max_apic_id))
goto out;
0xFFFFFFFF is converted to int min = -1;
Regards,
Wanpeng Li
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 10:23 ` Wanpeng Li
@ 2018-08-29 10:29 ` Dan Carpenter
2018-08-29 10:42 ` Wanpeng Li
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Dan Carpenter @ 2018-08-29 10:29 UTC (permalink / raw)
To: Wanpeng Li; +Cc: Liran Alon, LKML, kvm, Paolo Bonzini, Radim Krcmar
On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote:
> On Wed, 29 Aug 2018 at 18:18, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
> > > On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > > > > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > > index 0cefba2..86e933c 100644
> > > > > --- a/arch/x86/kvm/lapic.c
> > > > > +++ b/arch/x86/kvm/lapic.c
> > > > > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > > > rcu_read_lock();
> > > > > map = rcu_dereference(kvm->arch.apic_map);
> > > > >
> > > > > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > > > > + goto out;
> > > >
> > > > I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> > > > But that’s just a matter of taste :)
> > >
> > > That's an integer overflow.
> > >
> > > But I do prefer to put the variable on the left. The truth is that some
> > > Smatch checks just ignore code which is backwards written because
> > > otherwise you have to write duplicate code and the most code is written
> > > with the variable on the left.
> > >
> > > if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
> >
> > Wait, the (s32) cast doesn't make sense. We want negative min values to
> > be treated as invalid.
>
> In v2, how about:
>
> if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
> map->max_apic_id))
> goto out;
That works, too. It still has the off by one and we should set
"count = -KVM_EINVAL;".
Is the unlikely() really required? I don't know what the fast paths are
in KVM, so I don't know.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 10:29 ` Dan Carpenter
@ 2018-08-29 10:42 ` Wanpeng Li
2018-08-29 10:54 ` Dan Carpenter
2018-08-29 10:43 ` Liran Alon
2018-08-29 15:42 ` Radim Krcmar
2 siblings, 1 reply; 14+ messages in thread
From: Wanpeng Li @ 2018-08-29 10:42 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Liran Alon, LKML, kvm, Paolo Bonzini, Radim Krcmar
On Wed, 29 Aug 2018 at 18:29, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote:
> > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
> > > > On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > > > > > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > > > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > > > index 0cefba2..86e933c 100644
> > > > > > --- a/arch/x86/kvm/lapic.c
> > > > > > +++ b/arch/x86/kvm/lapic.c
> > > > > > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > > > > rcu_read_lock();
> > > > > > map = rcu_dereference(kvm->arch.apic_map);
> > > > > >
> > > > > > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > > > > > + goto out;
> > > > >
> > > > > I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> > > > > But that’s just a matter of taste :)
> > > >
> > > > That's an integer overflow.
> > > >
> > > > But I do prefer to put the variable on the left. The truth is that some
> > > > Smatch checks just ignore code which is backwards written because
> > > > otherwise you have to write duplicate code and the most code is written
> > > > with the variable on the left.
> > > >
> > > > if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
> > >
> > > Wait, the (s32) cast doesn't make sense. We want negative min values to
> > > be treated as invalid.
> >
> > In v2, how about:
> >
> > if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
> > map->max_apic_id))
> > goto out;
>
> That works, too. It still has the off by one and we should set
Sorry, why off by one?
> "count = -KVM_EINVAL;".
>
> Is the unlikely() really required? I don't know what the fast paths are
> in KVM, so I don't know.
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 10:42 ` Wanpeng Li
@ 2018-08-29 10:54 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2018-08-29 10:54 UTC (permalink / raw)
To: Wanpeng Li; +Cc: Liran Alon, LKML, kvm, Paolo Bonzini, Radim Krcmar
On Wed, Aug 29, 2018 at 06:42:42PM +0800, Wanpeng Li wrote:
> On Wed, 29 Aug 2018 at 18:29, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote:
> > > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
> > > > > On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > > > > > > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > > > > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > > > > index 0cefba2..86e933c 100644
> > > > > > > --- a/arch/x86/kvm/lapic.c
> > > > > > > +++ b/arch/x86/kvm/lapic.c
> > > > > > > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > > > > > rcu_read_lock();
> > > > > > > map = rcu_dereference(kvm->arch.apic_map);
> > > > > > >
> > > > > > > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > > > > > > + goto out;
> > > > > >
> > > > > > I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> > > > > > But that’s just a matter of taste :)
> > > > >
> > > > > That's an integer overflow.
> > > > >
> > > > > But I do prefer to put the variable on the left. The truth is that some
> > > > > Smatch checks just ignore code which is backwards written because
> > > > > otherwise you have to write duplicate code and the most code is written
> > > > > with the variable on the left.
> > > > >
> > > > > if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
> > > >
> > > > Wait, the (s32) cast doesn't make sense. We want negative min values to
> > > > be treated as invalid.
> > >
> > > In v2, how about:
> > >
> > > if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
> > > map->max_apic_id))
> > > goto out;
> >
> > That works, too. It still has the off by one and we should set
>
> Sorry, why off by one?
Sorry, my bad. I looked at the code and > is correct. (At first, I
thought it should be >= but I hadn't looked at the context).
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 10:29 ` Dan Carpenter
2018-08-29 10:42 ` Wanpeng Li
@ 2018-08-29 10:43 ` Liran Alon
2018-08-29 13:55 ` Radim Krcmar
2018-08-29 15:42 ` Radim Krcmar
2 siblings, 1 reply; 14+ messages in thread
From: Liran Alon @ 2018-08-29 10:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Wanpeng Li, LKML, kvm, Paolo Bonzini, Radim Krcmar
> On 29 Aug 2018, at 13:29, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote:
>> On Wed, 29 Aug 2018 at 18:18, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>
>>> On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
>>>> On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
>>>>>> arch/x86/kvm/lapic.c | 17 +++++++++++++----
>>>>>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>>>> index 0cefba2..86e933c 100644
>>>>>> --- a/arch/x86/kvm/lapic.c
>>>>>> +++ b/arch/x86/kvm/lapic.c
>>>>>> @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
>>>>>> rcu_read_lock();
>>>>>> map = rcu_dereference(kvm->arch.apic_map);
>>>>>>
>>>>>> + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
>>>>>> + goto out;
>>>>>
>>>>> I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
>>>>> But that’s just a matter of taste :)
>>>>
>>>> That's an integer overflow.
>>>>
>>>> But I do prefer to put the variable on the left. The truth is that some
>>>> Smatch checks just ignore code which is backwards written because
>>>> otherwise you have to write duplicate code and the most code is written
>>>> with the variable on the left.
>>>>
>>>> if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
>>>
>>> Wait, the (s32) cast doesn't make sense. We want negative min values to
>>> be treated as invalid.
>>
>> In v2, how about:
>>
>> if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
>> map->max_apic_id))
>> goto out;
>
> That works, too. It still has the off by one and we should set
> "count = -KVM_EINVAL;".
>
> Is the unlikely() really required? I don't know what the fast paths are
> in KVM, so I don't know.
>
> regards,
> dan carpenter
Why is “min” defined as “int” instead of “unsigned int”?
It represents the lowest APIC ID in bitmap so it can’t be negative…
"if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) > map->max_apic_id))”
should indeed be ok.
-Liran
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 10:43 ` Liran Alon
@ 2018-08-29 13:55 ` Radim Krcmar
2018-08-29 14:36 ` Radim Krcmar
0 siblings, 1 reply; 14+ messages in thread
From: Radim Krcmar @ 2018-08-29 13:55 UTC (permalink / raw)
To: Liran Alon; +Cc: Dan Carpenter, Wanpeng Li, LKML, kvm, Paolo Bonzini
2018-08-29 13:43+0300, Liran Alon:
> Why is “min” defined as “int” instead of “unsigned int”?
> It represents the lowest APIC ID in bitmap so it can’t be negative…
Right,
I think the code would look better as something like (untested):
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0cefba28c864..24fc84eb97d2 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -548,7 +548,7 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
}
int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
- unsigned long ipi_bitmap_high, int min,
+ unsigned long ipi_bitmap_high, u32 min,
unsigned long icr, int op_64_bit)
{
int i;
@@ -557,6 +557,7 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
struct kvm_lapic_irq irq = {0};
int cluster_size = op_64_bit ? 64 : 32;
int count = 0;
+ unsigned long ipi_bitmap[2] = {ipi_bitmap_low, ipi_bitmap_high};
irq.vector = icr & APIC_VECTOR_MASK;
irq.delivery_mode = icr & APIC_MODE_MASK;
@@ -571,16 +572,14 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
rcu_read_lock();
map = rcu_dereference(kvm->arch.apic_map);
- /* Bits above cluster_size are masked in the caller. */
- for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, &irq, NULL);
- }
+ if (min <= map->max_apic_id) {
+ size_t ipi_bitmap_size = MIN(sizeof(ipi_bitmap) * 8,
+ map->max_apic_id - min + 1);
- min += cluster_size;
- for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ for_each_set_bit(i, ipi_bitmap, ipi_bitmap_size) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ }
}
rcu_read_unlock();
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 13:55 ` Radim Krcmar
@ 2018-08-29 14:36 ` Radim Krcmar
0 siblings, 0 replies; 14+ messages in thread
From: Radim Krcmar @ 2018-08-29 14:36 UTC (permalink / raw)
To: Liran Alon; +Cc: Dan Carpenter, Wanpeng Li, LKML, kvm, Paolo Bonzini
2018-08-29 15:55+0200, Radim Krcmar:
> 2018-08-29 13:43+0300, Liran Alon:
> > Why is “min” defined as “int” instead of “unsigned int”?
> > It represents the lowest APIC ID in bitmap so it can’t be negative…
>
> Right,
>
> I think the code would look better as something like (untested):
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 0cefba28c864..24fc84eb97d2 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -548,7 +548,7 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> }
>
> int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> - unsigned long ipi_bitmap_high, int min,
> + unsigned long ipi_bitmap_high, u32 min,
> unsigned long icr, int op_64_bit)
> {
> int i;
> @@ -557,6 +557,7 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> struct kvm_lapic_irq irq = {0};
> int cluster_size = op_64_bit ? 64 : 32;
> int count = 0;
> + unsigned long ipi_bitmap[2] = {ipi_bitmap_low, ipi_bitmap_high};
The patch is wrong, I missed the 32/64 bit cluster size.
It's salvageable with something like
if (op_64_bit) {
ipi_bitmap[0] = ipi_bitmap_low;
ipi_bitmap[1] = ipi_bitmap_high;
ipi_bitmap_size = 128;
} else {
ipi_bitmap[0] = (u32)ipi_bitmap_low | ipi_bitmap_high << 32;
ipi_bitmap_size = 64;
}
>
> irq.vector = icr & APIC_VECTOR_MASK;
> irq.delivery_mode = icr & APIC_MODE_MASK;
> @@ -571,16 +572,14 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> rcu_read_lock();
> map = rcu_dereference(kvm->arch.apic_map);
>
> - /* Bits above cluster_size are masked in the caller. */
> - for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, &irq, NULL);
> - }
> + if (min <= map->max_apic_id) {
> + size_t ipi_bitmap_size = MIN(sizeof(ipi_bitmap) * 8,
> + map->max_apic_id - min + 1);
> - min += cluster_size;
> - for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, &irq, NULL);
> + for_each_set_bit(i, ipi_bitmap, ipi_bitmap_size) {
and
... MIN(ipi_bitmap_size, map->max_apic_id - min + 1) ...
Not good, but could still be nicer than the alternatives.
> + vcpu = map->phys_map[min + i]->vcpu;
> + count += kvm_apic_set_irq(vcpu, &irq, NULL);
> + }
> }
>
> rcu_read_unlock();
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 10:29 ` Dan Carpenter
2018-08-29 10:42 ` Wanpeng Li
2018-08-29 10:43 ` Liran Alon
@ 2018-08-29 15:42 ` Radim Krcmar
2018-08-30 2:15 ` Wanpeng Li
2 siblings, 1 reply; 14+ messages in thread
From: Radim Krcmar @ 2018-08-29 15:42 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Wanpeng Li, Liran Alon, LKML, kvm, Paolo Bonzini
2018-08-29 13:29+0300, Dan Carpenter:
> On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote:
> > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
> > > > On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > > > > > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > > > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > > > index 0cefba2..86e933c 100644
> > > > > > --- a/arch/x86/kvm/lapic.c
> > > > > > +++ b/arch/x86/kvm/lapic.c
> > > > > > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > > > > rcu_read_lock();
> > > > > > map = rcu_dereference(kvm->arch.apic_map);
> > > > > >
> > > > > > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > > > > > + goto out;
> > > > >
> > > > > I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> > > > > But that’s just a matter of taste :)
> > > >
> > > > That's an integer overflow.
> > > >
> > > > But I do prefer to put the variable on the left. The truth is that some
> > > > Smatch checks just ignore code which is backwards written because
> > > > otherwise you have to write duplicate code and the most code is written
> > > > with the variable on the left.
> > > >
> > > > if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
> > >
> > > Wait, the (s32) cast doesn't make sense. We want negative min values to
> > > be treated as invalid.
> >
> > In v2, how about:
> >
> > if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
> > map->max_apic_id))
> > goto out;
>
> That works, too. It still has the off by one and we should set
> "count = -KVM_EINVAL;".
I'd prefer to ignore destinations that are not present and deliver the
rest, possibly nothing, instead of returning an error.
(It's closer to how the real hardware behaves and we already return the
number of notified VCPUs, so the caller can tell whether something went
wrong.)
Either in the form that I have posted earlier, or as:
if (min > map->max_apic_id)
goto out;
for_each_set_bit(i, &ipi_bitmap_low, MIN(BITS_PER_LONG, map->max_apic_id - min + 1))
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
2018-08-29 15:42 ` Radim Krcmar
@ 2018-08-30 2:15 ` Wanpeng Li
0 siblings, 0 replies; 14+ messages in thread
From: Wanpeng Li @ 2018-08-30 2:15 UTC (permalink / raw)
To: Radim Krcmar; +Cc: Dan Carpenter, Liran Alon, LKML, kvm, Paolo Bonzini
On Wed, 29 Aug 2018 at 23:42, Radim Krcmar <rkrcmar@redhat.com> wrote:
>
> 2018-08-29 13:29+0300, Dan Carpenter:
> > On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote:
> > > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
> > > > > On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > > > > > > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > > > > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > > > > index 0cefba2..86e933c 100644
> > > > > > > --- a/arch/x86/kvm/lapic.c
> > > > > > > +++ b/arch/x86/kvm/lapic.c
> > > > > > > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > > > > > rcu_read_lock();
> > > > > > > map = rcu_dereference(kvm->arch.apic_map);
> > > > > > >
> > > > > > > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > > > > > > + goto out;
> > > > > >
> > > > > > I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> > > > > > But that’s just a matter of taste :)
> > > > >
> > > > > That's an integer overflow.
> > > > >
> > > > > But I do prefer to put the variable on the left. The truth is that some
> > > > > Smatch checks just ignore code which is backwards written because
> > > > > otherwise you have to write duplicate code and the most code is written
> > > > > with the variable on the left.
> > > > >
> > > > > if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
> > > >
> > > > Wait, the (s32) cast doesn't make sense. We want negative min values to
> > > > be treated as invalid.
> > >
> > > In v2, how about:
> > >
> > > if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
> > > map->max_apic_id))
> > > goto out;
> >
> > That works, too. It still has the off by one and we should set
> > "count = -KVM_EINVAL;".
>
> I'd prefer to ignore destinations that are not present and deliver the
> rest, possibly nothing, instead of returning an error.
> (It's closer to how the real hardware behaves and we already return the
> number of notified VCPUs, so the caller can tell whether something went
> wrong.)
>
> Either in the form that I have posted earlier, or as:
>
> if (min > map->max_apic_id)
> goto out;
>
> for_each_set_bit(i, &ipi_bitmap_low, MIN(BITS_PER_LONG, map->max_apic_id - min + 1))
Do it in v2.
Regards,
Wanpeng Li
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
@ 2018-08-29 5:51 Wanpeng Li
0 siblings, 0 replies; 14+ messages in thread
From: Wanpeng Li @ 2018-08-29 5:51 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář, Liran Alon, Dan Carpenter
From: Wanpeng Li <wanpengli@tencent.com>
Dan Carpenter reported that the untrusted data returns from kvm_register_read()
results in the following static checker warning:
arch/x86/kvm/lapic.c:576 kvm_pv_send_ipi()
error: buffer underflow 'map->phys_map' 's32min-s32max'
KVM guest can easily trigger this by executing the following assembly sequence
in Ring0:
mov $10, %rax
mov $0xFFFFFFFF, %rbx
mov $0xFFFFFFFF, %rdx
mov $0, %rsi
vmcall
As this will cause KVM to execute the following code-path:
vmx_handle_exit() -> handle_vmcall() -> kvm_emulate_hypercall() -> kvm_pv_send_ipi()
which will reach out-of-bounds access.
This patch fixes it by adding a check to kvm_pv_send_ipi() against map->max_apic_id
and also checking whether or not map->phys_map[min + i] is NULL since the max_apic_id
is set according to the max apic id, however, some phys_map maybe NULL when apic id
is sparse, in addition, kvm also unconditionally set max_apic_id to 255 to reserve
enough space for any xAPIC ID.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Liran Alon <liran.alon@oracle.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
arch/x86/kvm/lapic.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0cefba2..86e933c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
rcu_read_lock();
map = rcu_dereference(kvm->arch.apic_map);
+ if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
+ goto out;
/* Bits above cluster_size are masked in the caller. */
for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ }
}
min += cluster_size;
+ if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
+ goto out;
for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, &irq, NULL);
+ }
}
+out:
rcu_read_unlock();
return count;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-08-30 2:15 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-29 5:52 [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access Wanpeng Li
2018-08-29 9:05 ` Liran Alon
2018-08-29 10:12 ` Dan Carpenter
2018-08-29 10:18 ` Dan Carpenter
2018-08-29 10:23 ` Wanpeng Li
2018-08-29 10:29 ` Dan Carpenter
2018-08-29 10:42 ` Wanpeng Li
2018-08-29 10:54 ` Dan Carpenter
2018-08-29 10:43 ` Liran Alon
2018-08-29 13:55 ` Radim Krcmar
2018-08-29 14:36 ` Radim Krcmar
2018-08-29 15:42 ` Radim Krcmar
2018-08-30 2:15 ` Wanpeng Li
-- strict thread matches above, loose matches on Subject: below --
2018-08-29 5:51 Wanpeng Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.