Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1222506 > unrolled thread

[PATCH V4 2/4] kvm: fix double free for fast mmio eventfd

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V4 2/4] kvm: fix double free for fast mmio eventfd Jason Wang <jasowang@redhat.com> - 2015-09-11 05:20 +0200
    Re: [PATCH V4 2/4] kvm: fix double free for fast mmio eventfd Cornelia Huck <cornelia.huck@de.ibm.com> - 2015-09-11 09:50 +0200
      Re: [PATCH V4 2/4] kvm: fix double free for fast mmio eventfd Jason Wang <jasowang@redhat.com> - 2015-09-11 11:30 +0200
        Re: [PATCH V4 2/4] kvm: fix double free for fast mmio eventfd Cornelia Huck <cornelia.huck@de.ibm.com> - 2015-09-11 12:20 +0200

#1222506 — [PATCH V4 2/4] kvm: fix double free for fast mmio eventfd

We register wildcard mmio eventfd on two buses, one for KVM_MMIO_BUS
and another is KVM_FAST_MMIO_BUS but with a single iodev
instance. This will lead an issue: kvm_io_bus_destroy() knows nothing
about the devices on two buses points to a single dev. Which will lead
double free[1] during exit. Fixing this by using allocate two
instances of iodevs then register one on KVM_MMIO_BUS and another on
KVM_FAST_MMIO_BUS.

CPU: 1 PID: 2894 Comm: qemu-system-x86 Not tainted 3.19.0-26-generic #28-Ubuntu
Hardware name: LENOVO 2356BG6/2356BG6, BIOS G7ET96WW (2.56 ) 09/12/2013
task: ffff88009ae0c4b0 ti: ffff88020e7f0000 task.ti: ffff88020e7f0000
RIP: 0010:[<ffffffffc07e25d8>]  [<ffffffffc07e25d8>] ioeventfd_release+0x28/0x60 [kvm]
RSP: 0018:ffff88020e7f3bc8  EFLAGS: 00010292
RAX: dead000000200200 RBX: ffff8801ec19c900 RCX: 000000018200016d
RDX: ffff8801ec19cf80 RSI: ffffea0008bf1d40 RDI: ffff8801ec19c900
RBP: ffff88020e7f3bd8 R08: 000000002fc75a01 R09: 000000018200016d
R10: ffffffffc07df6ae R11: ffff88022fc75a98 R12: ffff88021e7cc000
R13: ffff88021e7cca48 R14: ffff88021e7cca50 R15: ffff8801ec19c880
FS:  00007fc1ee3e6700(0000) GS:ffff88023e240000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f8f389d8000 CR3: 000000023dc13000 CR4: 00000000001427e0
Stack:
ffff88021e7cc000 0000000000000000 ffff88020e7f3be8 ffffffffc07e2622
ffff88020e7f3c38 ffffffffc07df69a ffff880232524160 ffff88020e792d80
 0000000000000000 ffff880219b78c00 0000000000000008 ffff8802321686a8
Call Trace:
[<ffffffffc07e2622>] ioeventfd_destructor+0x12/0x20 [kvm]
[<ffffffffc07df69a>] kvm_put_kvm+0xca/0x210 [kvm]
[<ffffffffc07df818>] kvm_vcpu_release+0x18/0x20 [kvm]
[<ffffffff811f69f7>] __fput+0xe7/0x250
[<ffffffff811f6bae>] ____fput+0xe/0x10
[<ffffffff81093f04>] task_work_run+0xd4/0xf0
[<ffffffff81079358>] do_exit+0x368/0xa50
[<ffffffff81082c8f>] ? recalc_sigpending+0x1f/0x60
[<ffffffff81079ad5>] do_group_exit+0x45/0xb0
[<ffffffff81085c71>] get_signal+0x291/0x750
[<ffffffff810144d8>] do_signal+0x28/0xab0
[<ffffffff810f3a3b>] ? do_futex+0xdb/0x5d0
[<ffffffff810b7028>] ? __wake_up_locked_key+0x18/0x20
[<ffffffff810f3fa6>] ? SyS_futex+0x76/0x170
[<ffffffff81014fc9>] do_notify_resume+0x69/0xb0
[<ffffffff817cb9af>] int_signal+0x12/0x17
Code: 5d c3 90 0f 1f 44 00 00 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 8b 7f 20 e8 06 d6 a5 c0 48 8b 43 08 48 8b 13 48 89 df 48 89 42 08 <48> 89 10 48 b8 00 01 10 00 00
 RIP  [<ffffffffc07e25d8>] ioeventfd_release+0x28/0x60 [kvm]
 RSP <ffff88020e7f3bc8>

Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 virt/kvm/eventfd.c | 111 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 59 insertions(+), 52 deletions(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 163258d..1a023ac 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -817,16 +817,6 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
 	if (ret < 0)
 		goto unlock_fail;
 
-	/* When length is ignored, MMIO is also put on a separate bus, for
-	 * faster lookups.
-	 */
-	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {
-		ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
-					      p->addr, 0, &p->dev);
-		if (ret < 0)
-			goto register_fail;
-	}
-
 	kvm->buses[bus_idx]->ioeventfd_count++;
 	list_add_tail(&p->list, &kvm->ioeventfds);
 
@@ -834,8 +824,6 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
 
 	return 0;
 
-register_fail:
-	kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
 unlock_fail:
 	mutex_unlock(&kvm->slots_lock);
 
@@ -847,41 +835,6 @@ fail:
 }
 
 static int
-kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
-{
-	enum kvm_bus              bus_idx;
-
-	bus_idx = ioeventfd_bus_from_flags(args->flags);
-	/* must be natural-word sized, or 0 to ignore length */
-	switch (args->len) {
-	case 0:
-	case 1:
-	case 2:
-	case 4:
-	case 8:
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	/* check for range overflow */
-	if (args->addr + args->len < args->addr)
-		return -EINVAL;
-
-	/* check for extra flags that we don't understand */
-	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
-		return -EINVAL;
-
-	/* ioeventfd with no length can't be combined with DATAMATCH */
-	if (!args->len &&
-	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
-			   KVM_IOEVENTFD_FLAG_DATAMATCH))
-		return -EINVAL;
-
-	return kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
-}
-
-static int
 kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
 			   struct kvm_ioeventfd *args)
 {
@@ -909,10 +862,6 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
 			continue;
 
 		kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
-		if (!p->length) {
-			kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
-						  &p->dev);
-		}
 		kvm->buses[bus_idx]->ioeventfd_count--;
 		ioeventfd_release(p);
 		ret = 0;
@@ -929,8 +878,66 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
 static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
+	int ret = kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
+
+	if (!args->len)
+		kvm_deassign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
+
+	return ret;
+}
 
-	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
+static int
+kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
+{
+	enum kvm_bus              bus_idx;
+	int ret;
+
+	bus_idx = ioeventfd_bus_from_flags(args->flags);
+	/* must be natural-word sized, or 0 to ignore length */
+	switch (args->len) {
+	case 0:
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* check for range overflow */
+	if (args->addr + args->len < args->addr)
+		return -EINVAL;
+
+	/* check for extra flags that we don't understand */
+	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
+		return -EINVAL;
+
+	/* ioeventfd with no length can't be combined with DATAMATCH */
+	if (!args->len &&
+	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
+			   KVM_IOEVENTFD_FLAG_DATAMATCH))
+		return -EINVAL;
+
+	ret = kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
+	if (ret)
+		goto fail;
+
+	/* When length is ignored, MMIO is also put on a separate bus, for
+	 * faster lookups.
+	 */
+	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {
+		ret = kvm_assign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
+		if (ret < 0)
+			goto fast_fail;
+	}
+
+	return 0;
+
+fast_fail:
+	kvm_deassign_ioeventfd(kvm, args);
+fail:
+	return ret;
 }
 
 int
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1222591

On Fri, 11 Sep 2015 11:17:35 +0800
Jason Wang <jasowang@redhat.com> wrote:

> We register wildcard mmio eventfd on two buses, one for KVM_MMIO_BUS
> and another is KVM_FAST_MMIO_BUS but with a single iodev
> instance. This will lead an issue: kvm_io_bus_destroy() knows nothing
> about the devices on two buses points to a single dev. Which will lead

s/points/pointing/

> double free[1] during exit. Fixing this by using allocate two

s/using allocate/allocating/

> instances of iodevs then register one on KVM_MMIO_BUS and another on
> KVM_FAST_MMIO_BUS.
> 
(...)

> @@ -929,8 +878,66 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
>  static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  {
>  	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
> +	int ret = kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
> +
> +	if (!args->len)
> +		kvm_deassign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);

I think it would be good to explicitly check for bus_idx ==
KVM_MMIO_BUS here.

> +
> +	return ret;
> +}
> 
> -	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
> +static int
> +kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> +{
> +	enum kvm_bus              bus_idx;
> +	int ret;
> +
> +	bus_idx = ioeventfd_bus_from_flags(args->flags);
> +	/* must be natural-word sized, or 0 to ignore length */
> +	switch (args->len) {
> +	case 0:
> +	case 1:
> +	case 2:
> +	case 4:
> +	case 8:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* check for range overflow */
> +	if (args->addr + args->len < args->addr)
> +		return -EINVAL;
> +
> +	/* check for extra flags that we don't understand */
> +	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
> +		return -EINVAL;
> +
> +	/* ioeventfd with no length can't be combined with DATAMATCH */
> +	if (!args->len &&
> +	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
> +			   KVM_IOEVENTFD_FLAG_DATAMATCH))
> +		return -EINVAL;
> +
> +	ret = kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
> +	if (ret)
> +		goto fail;
> +
> +	/* When length is ignored, MMIO is also put on a separate bus, for
> +	 * faster lookups.
> +	 */
> +	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {

Dito on a positive check for bus_idx == KVM_MMIO_BUS.

> +		ret = kvm_assign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
> +		if (ret < 0)
> +			goto fast_fail;
> +	}
> +
> +	return 0;
> +
> +fast_fail:
> +	kvm_deassign_ioeventfd(kvm, args);

Shouldn't you use kvm_deassign_ioeventfd(kvm, bus_idx, args) here?

> +fail:
> +	return ret;
>  }
> 
>  int

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222645

On 09/11/2015 03:46 PM, Cornelia Huck wrote:
> On Fri, 11 Sep 2015 11:17:35 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> We register wildcard mmio eventfd on two buses, one for KVM_MMIO_BUS
>> and another is KVM_FAST_MMIO_BUS but with a single iodev
>> instance. This will lead an issue: kvm_io_bus_destroy() knows nothing
>> about the devices on two buses points to a single dev. Which will lead
> s/points/pointing/

Will fix this in V5.

>> double free[1] during exit. Fixing this by using allocate two
> s/using allocate/allocating/

Will fix this in V5.

>
>> instances of iodevs then register one on KVM_MMIO_BUS and another on
>> KVM_FAST_MMIO_BUS.
>>
> (...)
>
>> @@ -929,8 +878,66 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
>>  static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>>  {
>>  	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
>> +	int ret = kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
>> +
>> +	if (!args->len)
>> +		kvm_deassign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
> I think it would be good to explicitly check for bus_idx ==
> KVM_MMIO_BUS here.

Ok.

>
>> +
>> +	return ret;
>> +}
>>
>> -	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
>> +static int
>> +kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>> +{
>> +	enum kvm_bus              bus_idx;
>> +	int ret;
>> +
>> +	bus_idx = ioeventfd_bus_from_flags(args->flags);
>> +	/* must be natural-word sized, or 0 to ignore length */
>> +	switch (args->len) {
>> +	case 0:
>> +	case 1:
>> +	case 2:
>> +	case 4:
>> +	case 8:
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* check for range overflow */
>> +	if (args->addr + args->len < args->addr)
>> +		return -EINVAL;
>> +
>> +	/* check for extra flags that we don't understand */
>> +	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
>> +		return -EINVAL;
>> +
>> +	/* ioeventfd with no length can't be combined with DATAMATCH */
>> +	if (!args->len &&
>> +	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
>> +			   KVM_IOEVENTFD_FLAG_DATAMATCH))
>> +		return -EINVAL;
>> +
>> +	ret = kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
>> +	if (ret)
>> +		goto fail;
>> +
>> +	/* When length is ignored, MMIO is also put on a separate bus, for
>> +	 * faster lookups.
>> +	 */
>> +	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {
> Dito on a positive check for bus_idx == KVM_MMIO_BUS.

I was thinking maybe this should be done in a separate patch on top.
What's your opinion?

>> +		ret = kvm_assign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
>> +		if (ret < 0)
>> +			goto fast_fail;
>> +	}
>> +
>> +	return 0;
>> +
>> +fast_fail:
>> +	kvm_deassign_ioeventfd(kvm, args);
> Shouldn't you use kvm_deassign_ioeventfd(kvm, bus_idx, args) here?

Actually, it's the same. (the deassign of fast mmio will return -ENOENT
and will be ignored.) But I admit do what you suggested here is better.
Will do this.

Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222688

On Fri, 11 Sep 2015 17:25:45 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 09/11/2015 03:46 PM, Cornelia Huck wrote:
> > On Fri, 11 Sep 2015 11:17:35 +0800
> > Jason Wang <jasowang@redhat.com> wrote:

> >> +
> >> +	/* When length is ignored, MMIO is also put on a separate bus, for
> >> +	 * faster lookups.
> >> +	 */
> >> +	if (!args->len && !(args->flags & KVM_IOEVENTFD_FLAG_PIO)) {
> > Dito on a positive check for bus_idx == KVM_MMIO_BUS.
> 
> I was thinking maybe this should be done in a separate patch on top.
> What's your opinion?

The check is an independent issue, an extra patch is fine (current
usage does not trigger any problems).

> 
> >> +		ret = kvm_assign_ioeventfd_idx(kvm, KVM_FAST_MMIO_BUS, args);
> >> +		if (ret < 0)
> >> +			goto fast_fail;
> >> +	}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web