Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, 29 Aug 2018 at 23:42, Radim Krcmar 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 > > > 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, _bitmap_low, MIN(BITS_PER_LONG, > map->max_apic_id - min + 1)) Do it in v2. Regards, Wanpeng Li
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, 29 Aug 2018 at 23:42, Radim Krcmar 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 > > > 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, _bitmap_low, MIN(BITS_PER_LONG, > map->max_apic_id - min + 1)) Do it in v2. Regards, Wanpeng Li
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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 > > 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, _bitmap_low, MIN(BITS_PER_LONG, map->max_apic_id - min + 1))
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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 > > 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, _bitmap_low, MIN(BITS_PER_LONG, map->max_apic_id - min + 1))
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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, _bitmap_low, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, , 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, _bitmap_high, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, , 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, , NULL);
> + }
> }
>
> rcu_read_unlock();
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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, _bitmap_low, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, , 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, _bitmap_high, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, , 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, , NULL);
> + }
> }
>
> rcu_read_unlock();
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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, _bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , 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, _bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ for_each_set_bit(i, ipi_bitmap, ipi_bitmap_size) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
rcu_read_unlock();
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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, _bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , 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, _bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ for_each_set_bit(i, ipi_bitmap, ipi_bitmap_size) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
rcu_read_unlock();
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, Aug 29, 2018 at 06:42:42PM +0800, Wanpeng Li wrote: > On Wed, 29 Aug 2018 at 18:29, Dan Carpenter wrote: > > > > On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote: > > > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter > > > 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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, Aug 29, 2018 at 06:42:42PM +0800, Wanpeng Li wrote: > On Wed, 29 Aug 2018 at 18:29, Dan Carpenter wrote: > > > > On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote: > > > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter > > > 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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
> On 29 Aug 2018, at 13:29, Dan Carpenter wrote: > > On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote: >> On Wed, 29 Aug 2018 at 18:18, Dan Carpenter 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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
> On 29 Aug 2018, at 13:29, Dan Carpenter wrote: > > On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote: >> On Wed, 29 Aug 2018 at 18:18, Dan Carpenter 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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, 29 Aug 2018 at 18:29, Dan Carpenter wrote: > > On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote: > > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter > > 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 >
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, 29 Aug 2018 at 18:29, Dan Carpenter wrote: > > On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote: > > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter > > 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 >
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote: > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter 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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote: > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter 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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, 29 Aug 2018 at 18:18, Dan Carpenter 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; 0x is converted to int min = -1; Regards, Wanpeng Li
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
On Wed, 29 Aug 2018 at 18:18, Dan Carpenter 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; 0x is converted to int min = -1; Regards, Wanpeng Li
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
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
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
> On 29 Aug 2018, at 8:52, Wanpeng Li wrote:
>
> From: Wanpeng Li
>
> 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 $0x, %rbx
> mov $0x, %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
> Cc: Paolo Bonzini
> Cc: Radim Krčmář
> Cc: Liran Alon
> Cc: Dan Carpenter
> Signed-off-by: Wanpeng Li
> ---
> 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, _bitmap_low, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, , NULL);
> + if (map->phys_map[min + i]) {
> + vcpu = map->phys_map[min + i]->vcpu;
> + count += kvm_apic_set_irq(vcpu, , NULL);
> + }
> }
>
> min += cluster_size;
> + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
> + goto out;
> for_each_set_bit(i, _bitmap_high, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, , NULL);
> + if (map->phys_map[min + i]) {
> + vcpu = map->phys_map[min + i]->vcpu;
> + count += kvm_apic_set_irq(vcpu, , NULL);
> + }
> }
>
> +out:
> rcu_read_unlock();
> return count;
> }
> --
> 2.7.4
>
Reviewed-By: Liran Alon
Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
> On 29 Aug 2018, at 8:52, Wanpeng Li wrote:
>
> From: Wanpeng Li
>
> 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 $0x, %rbx
> mov $0x, %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
> Cc: Paolo Bonzini
> Cc: Radim Krčmář
> Cc: Liran Alon
> Cc: Dan Carpenter
> Signed-off-by: Wanpeng Li
> ---
> 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, _bitmap_low, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, , NULL);
> + if (map->phys_map[min + i]) {
> + vcpu = map->phys_map[min + i]->vcpu;
> + count += kvm_apic_set_irq(vcpu, , NULL);
> + }
> }
>
> min += cluster_size;
> + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
> + goto out;
> for_each_set_bit(i, _bitmap_high, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, , NULL);
> + if (map->phys_map[min + i]) {
> + vcpu = map->phys_map[min + i]->vcpu;
> + count += kvm_apic_set_irq(vcpu, , NULL);
> + }
> }
>
> +out:
> rcu_read_unlock();
> return count;
> }
> --
> 2.7.4
>
Reviewed-By: Liran Alon
[PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
From: Wanpeng Li
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 $0x, %rbx
mov $0x, %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
Cc: Paolo Bonzini
Cc: Radim Krčmář
Cc: Liran Alon
Cc: Dan Carpenter
Signed-off-by: Wanpeng Li
---
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, _bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
min += cluster_size;
+ if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
+ goto out;
for_each_set_bit(i, _bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
+out:
rcu_read_unlock();
return count;
}
--
2.7.4
[PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
From: Wanpeng Li
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 $0x, %rbx
mov $0x, %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
Cc: Paolo Bonzini
Cc: Radim Krčmář
Cc: Liran Alon
Cc: Dan Carpenter
Signed-off-by: Wanpeng Li
---
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, _bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
min += cluster_size;
+ if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
+ goto out;
for_each_set_bit(i, _bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
+out:
rcu_read_unlock();
return count;
}
--
2.7.4
[PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
From: Wanpeng Li
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 $0x, %rbx
mov $0x, %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
Cc: Paolo Bonzini
Cc: Radim Krčmář
Cc: Liran Alon
Cc: Dan Carpenter
Signed-off-by: Wanpeng Li
---
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, _bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
min += cluster_size;
+ if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
+ goto out;
for_each_set_bit(i, _bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
+out:
rcu_read_unlock();
return count;
}
--
2.7.4
[PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
From: Wanpeng Li
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 $0x, %rbx
mov $0x, %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
Cc: Paolo Bonzini
Cc: Radim Krčmář
Cc: Liran Alon
Cc: Dan Carpenter
Signed-off-by: Wanpeng Li
---
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, _bitmap_low, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
min += cluster_size;
+ if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
+ goto out;
for_each_set_bit(i, _bitmap_high, BITS_PER_LONG) {
- vcpu = map->phys_map[min + i]->vcpu;
- count += kvm_apic_set_irq(vcpu, , NULL);
+ if (map->phys_map[min + i]) {
+ vcpu = map->phys_map[min + i]->vcpu;
+ count += kvm_apic_set_irq(vcpu, , NULL);
+ }
}
+out:
rcu_read_unlock();
return count;
}
--
2.7.4
