: Fix memory corruption in-kernel IOAPIC emulation

PATCH In review Chris Lalancette 2 participants 8 messages Jan 29, 2008 – Feb 5, 2008 5 messages not in the archive

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Chris Lalancette

author

2008-01-29 23:24 UTC #0 lore raw

PATCH 1/1 : Fix memory corruption in-kernel IOAPIC emulation

All,
Attached is a patch that fixes the first (of at least a couple) migration
problem that I am running into. Basically, using the setup I described in my last post, I was always getting "Disabling IRQ #11" once the guest reached the destination side, and then no further activity. Dumping the APIC on both the source and destination side revealed something interesting:

Source:
APIC 0x2
(pad is 0x0
IOAPIC state:

    base_address: 0xfec00000
    ioregsel:     0x2e
    id:           0x0
    irr:          0x0
    pad:          0x0

Destination:
APIC 0x2
(pad is 0x38)
IOAPIC state:

    base_address: 0xf2001000
    ioregsel:     0x2e
    id:           0x0
    irr:          0x78872f3d
    pad:          0x38

You'll notice that the base_address and irr are completely bogus on the destination side. Although KVM_CREATE_IRQCHIP does the right thing on the destination side when first creating the "incoming" guest, the base_address and other fields get blown away with bogus data during the restore. The attached patch fixes this by only restoring the bits that we know were saved on the source side (i.e. what's in qemu/hw/apic.c:ioapic_save()).

Signed-off-by Chris Lalancette <[email protected]>

arch/x86/kvm/x86.c +13 -3

index 8f94a0b..b07ea3a 100644
@@ -1314,6 +1314,9 @@ static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 {
 	int r;
+	int i;
+	struct kvm_ioapic *kioapic;
+	struct kvm_ioapic_state *uioapic;
 
 	r = 0;
 	switch (chip->chip_id) {

@@ -1328,9 +1331,16 @@ static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 			sizeof(struct kvm_pic_state));
 		break;
 	case KVM_IRQCHIP_IOAPIC:
-		memcpy(ioapic_irqchip(kvm),
-			&chip->chip.ioapic,
-			sizeof(struct kvm_ioapic_state));
+		kioapic = ioapic_irqchip(kvm);
+		uioapic = &chip->chip.ioapic;
+
+		kioapic->id = uioapic->id;
+		kioapic->ioregsel = uioapic->ioregsel;
+
+		for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+			kioapic->redirtbl[i].bits = uioapic->redirtbl[i].bits;
+		}
+
 		break;
 	default:
 		r = -EINVAL;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Avi Kivity

2008-01-30 10:04 UTC #2 lore raw

In reply to a message not in the archive

Quoted 9 lines · Chris Lalancette

All,
Attached is a patch that fixes the first (of at least a couple) migration
problem that I am running into. Basically, using the setup I described in my last post, I was always getting "Disabling IRQ #11" once the guest reached the destination side, and then no further activity. Dumping the APIC on both the source and destination side revealed something interesting:

[...]

Quoted 7 lines

You'll notice that the base_address and irr are completely bogus on the destination side. Although KVM_CREATE_IRQCHIP does the right thing on the destination side when first creating the "incoming" guest, the base_address and other fields get blown away with bogus data during the restore. The attached patch fixes this by only restoring the bits that we know were saved on the source side (i.e. what's in qemu/hw/apic.c:ioapic_save()).

Excellent catch, but the fix is wrong. Instead of partially restoring the ioapic state in the kernel, you should fully save it in qemu.

This is a trap that many fall into: considering kvm and qemu as one entity and making sure they work well together. We need to make sure that kvm and libkvm are useful for other userspace programs as well.

Signature · 7 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Chris Lalancette

author

2008-01-30 13:28 UTC #4 lore raw

In reply to a message not in the archive

Quoted 8 lines · Avi Kivity

Excellent catch, but the fix is wrong. Instead of partially restoring the ioapic state in the kernel, you should fully save it in qemu.
This is a trap that many fall into: considering kvm and qemu as one entity and making sure they work well together. We need to make sure that kvm and libkvm are useful for other userspace programs as well.

Ah. Yeah, that makes sense. OK, I'll work up another patch that does that. Thanks for the feedback.

Chris Lalancette

author

2008-01-30 16:39 UTC #5 lore raw

In reply to a message not in the archive

Quoted 8 lines · Avi Kivity

Excellent catch, but the fix is wrong. Instead of partially restoring the ioapic state in the kernel, you should fully save it in qemu.
This is a trap that many fall into: considering kvm and qemu as one entity and making sure they work well together. We need to make sure that kvm and libkvm are useful for other userspace programs as well.

Actually, let me ask a question here. It seems to me that there are two ways I could go about this:

1) Change the save protocol so that it saves the relevant information (i.e. base_address and irr), and then the restore protocol so it actually pulls this stuff off the wire. This solution seems more "right" to me, but it has the downside that we are changing the over-the-wire information, which will break migration between old and new versions of qemu.

2) Just change the restore protocol so that we properly fill in the missing fields with real, hard-coded values instead of just random memory. This has the benefit that it doesn't change the protocol, but has the downside that we won't reflect changes to base_address or irr 100% properly. In practice, this doesn't seem like a big deal since there doesn't currently seem to be a way to change base_address anyway, and losing the irr doesn't seem to be catastrophic (although I'm not 100% certain about that).

The attached patch implements 2); does anyone have an opinion on which way to go here?

Chris Lalancette

qemu/hw/apic.c +5 -1

index c26a18d..6b41d7a 100644
@@ -60,6 +60,7 @@
 
 /* FIXME: it's now hard coded to be equal with KVM_IOAPIC_NUM_PINS */
 #define IOAPIC_NUM_PINS			0x18
+#define IOAPIC_DEFAULT_BASE_ADDRESS  0xfec00000
 
 #define ESR_ILLEGAL_ADDRESS (1 << 7)

@@ -1143,8 +1144,11 @@ static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
     chip.chip_id = KVM_IRQCHIP_IOAPIC;
     kioapic = &chip.chip.ioapic;
 
-    kioapic->id = s->id;
+    kioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
     kioapic->ioregsel = s->ioregsel;
+    kioapic->id = s->id;
+    kioapic->irr = 0;
+    kioapic->pad = 0;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         kioapic->redirtbl[i].bits = s->ioredtbl[i];
     }

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Avi Kivity

2008-01-30 16:54 UTC #7 lore raw

In reply to a message not in the archive

Quoted 14 lines · Chris Lalancette

Quoted 8 lines · Avi Kivity
Excellent catch, but the fix is wrong. Instead of partially restoring the ioapic state in the kernel, you should fully save it in qemu.
This is a trap that many fall into: considering kvm and qemu as one entity and making sure they work well together. We need to make sure that kvm and libkvm are useful for other userspace programs as well.
Actually, let me ask a question here. It seems to me that there are two ways I could go about this:

Quoted 8 lines

1) Change the save protocol so that it saves the relevant information (i.e. base_address and irr), and then the restore protocol so it actually pulls this stuff off the wire. This solution seems more "right" to me, but it has the downside that we are changing the over-the-wire information, which will break migration between old and new versions of qemu.

qemu supports backwards compatible changes to the protocol via the version field.

Quoted 9 lines

2) Just change the restore protocol so that we properly fill in the missing fields with real, hard-coded values instead of just random memory. This has the benefit that it doesn't change the protocol, but has the downside that we won't reflect changes to base_address or irr 100% properly. In practice, this doesn't seem like a big deal since there doesn't currently seem to be a way to change base_address anyway, and losing the irr doesn't seem to be catastrophic (although I'm not 100% certain about that).

Quoted 3 lines

The attached patch implements 2); does anyone have an opinion on which way to go here?

I prefer doing a full save/restore, since we don't know what guests depend on.

Signature · 7 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Chris Lalancette

author

2008-01-30 21:03 UTC #8 lore raw

In reply to a message not in the archive

Another version of the patch, done by changing the on-the-wire protocol as Avi suggested. I've tested this with:

old -> old - Migration works, but runs into the bug I'm trying to fix old -> new - Migration works, but runs into the bug I'm trying to fix new -> old - Migration fails gracefully with ioapic version mismatch new -> new - Migration works, and doesn't run into this particular bug

Signed-off-by Chris Lalancette <[email protected]>

qemu/hw/apic.c +23 -5

index c26a18d..7f18e38 100644
@@ -94,6 +94,7 @@ typedef struct APICState {
 struct IOAPICState {
     uint8_t id;
     uint8_t ioregsel;
+    uint64_t base_address;
 
     uint32_t irr;
     uint64_t ioredtbl[IOAPIC_NUM_PINS];

@@ -1125,24 +1126,36 @@ static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
     kvm_get_irqchip(kvm_context, &chip);
     kioapic = &chip.chip.ioapic;
 
-    s->id = kioapic->id;
+    s->base_address = kioapic->base_address;
     s->ioregsel = kioapic->ioregsel;
+    s->id = kioapic->id;
+    s->irr = kioapic->irr;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         s->ioredtbl[i] = kioapic->redirtbl[i].bits;
     }
 #endif
 }
 
-static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
+static int kvm_kernel_ioapic_load_from_user(IOAPICState *s, int version_id)
 {
 #if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
     struct kvm_irqchip chip;
     struct kvm_ioapic_state *kioapic;
     int i;
 
+    if (version_id > 2) {
+        return -EINVAL;
+    }
+
     chip.chip_id = KVM_IRQCHIP_IOAPIC;
     kioapic = &chip.chip.ioapic;
 
+    if (version_id == 2) {
+        kioapic->base_address = s->base_address;
+        kioapic->irr = s->irr;
+    }
+
+    /* fields saved since version 1 */
     kioapic->id = s->id;
     kioapic->ioregsel = s->ioregsel;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {

@@ -1151,6 +1164,8 @@ static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
 
     kvm_set_irqchip(kvm_context, &chip);
 #endif
+
+    return 0;
 }
 
 static void ioapic_save(QEMUFile *f, void *opaque)

@@ -1173,8 +1188,9 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
 {
     IOAPICState *s = opaque;
     int i;
+    int ret;
 
-    if (version_id != 1)
+    if (version_id > 2)
         return -EINVAL;
 
     qemu_get_8s(f, &s->id);

@@ -1184,7 +1200,9 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
-        kvm_kernel_ioapic_load_from_user(s);
+        ret = kvm_kernel_ioapic_load_from_user(s, version_id);
+        if (ret < 0)
+	    return ret;
     }
 
     return 0;

@@ -1227,7 +1245,7 @@ IOAPICState *ioapic_init(void)
                                        ioapic_mem_write, s);
     cpu_register_physical_memory(0xfec00000, 0x1000, io_memory);
 
-    register_savevm("ioapic", 0, 1, ioapic_save, ioapic_load, s);
+    register_savevm("ioapic", 0, 2, ioapic_save, ioapic_load, s);
     qemu_register_reset(ioapic_reset, s);
 
     return s;

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Avi Kivity

2008-01-31 07:24 UTC #10 lore raw

In reply to a message not in the archive

Quoted 21 lines · Chris Lalancette

Another version of the patch, done by changing the on-the-wire protocol as Avi suggested. I've tested this with:
old -> old - Migration works, but runs into the bug I'm trying to fix old -> new - Migration works, but runs into the bug I'm trying to fix new -> old - Migration fails gracefully with ioapic version mismatch new -> new - Migration works, and doesn't run into this particular bug
qemu/hw/apic.c +3 -1
-static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
+static int kvm_kernel_ioapic_load_from_user(IOAPICState *s, int version_id)
 {
 #if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
     struct kvm_irqchip chip;
     struct kvm_ioapic_state *kioapic;
     int i;
 
+    if (version_id > 2) {
+        return -EINVAL;

qemu/hw/apic.c static void kvm_kernel_ioapic_save_to_user(IOAPICState *s) · line 1157

Quoted 10 lines

qemu/hw/apic.c +7 -0

+    }
+
     chip.chip_id = KVM_IRQCHIP_IOAPIC;
     kioapic = &chip.chip.ioapic;
 
+    if (version_id == 2) {
+        kioapic->base_address = s->base_address;
+        kioapic->irr = s->irr;
+    }
+

Preferably this function would always set all variables; the caller should make sure all the values are sane (by initializing them to sane values on ioapic creation, and not modifying them when a version 1 migration happens). It's best to limit protocol change handling to the actual qemu save/restore routines.

qemu/hw/apic.c static int ioapic_load(QEMUFile *f, void *opaque, int version_id) · line 1195

Quoted 11 lines

static void ioapic_save(QEMUFile *f, void *opaque)

qemu/hw/apic.c +2 -1

@@ -1173,8 +1188,9 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
 {
     IOAPICState *s = opaque;
     int i;
+    int ret;
 
-    if (version_id != 1)
+    if (version_id > 2)
         return -EINVAL;

This allows version_id == 0 to sneak in, where it couldn't before. It's not a problem in practice, but let's get it right.

Signature · 7 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Chris Lalancette

author

2008-02-05 15:58 UTC #12 lore raw

In reply to a message not in the archive

Another version of the patch, taking into account more of Avi's comments. This one was tested the same way as the previous one, by doing all the combinations of new and old QEMU versions; the results were the same as last time:

old -> old: Bug
old -> new: Sane values, but not transferred over the wire new -> old: Graceful fail, version mismatch
new -> new: Fixed values, taken from the wire

Signed-off-by Chris Lalancette <[email protected]>

qemu/hw/apic.c +20 -2

index a47c366..21e5790 100644
@@ -64,6 +64,7 @@ extern kvm_context_t kvm_context;
 
 /* FIXME: it's now hard coded to be equal with KVM_IOAPIC_NUM_PINS */
 #define IOAPIC_NUM_PINS			0x18
+#define IOAPIC_DEFAULT_BASE_ADDRESS  0xfec00000
 
 #define ESR_ILLEGAL_ADDRESS (1 << 7)

@@ -98,6 +99,7 @@ typedef struct APICState {
 struct IOAPICState {
     uint8_t id;
     uint8_t ioregsel;
+    uint64_t base_address;
 
     uint32_t irr;
     uint64_t ioredtbl[IOAPIC_NUM_PINS];

@@ -1145,6 +1147,8 @@ static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
 
     s->id = kioapic->id;
     s->ioregsel = kioapic->ioregsel;
+    s->base_address = kioapic->base_address;
+    s->irr = kioapic->irr;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         s->ioredtbl[i] = kioapic->redirtbl[i].bits;
     }

@@ -1163,6 +1167,8 @@ static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
 
     kioapic->id = s->id;
     kioapic->ioregsel = s->ioregsel;
+    kioapic->base_address = s->base_address;
+    kioapic->irr = s->irr;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         kioapic->redirtbl[i].bits = s->ioredtbl[i];
     }

@@ -1185,6 +1191,8 @@ static void ioapic_save(QEMUFile *f, void *opaque)
 
     qemu_put_8s(f, &s->id);
     qemu_put_8s(f, &s->ioregsel);
+    qemu_put_be64s(f, &s->base_address);
+    qemu_put_be32s(f, &s->irr);
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         qemu_put_be64s(f, &s->ioredtbl[i]);
     }

@@ -1195,11 +1203,21 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
     IOAPICState *s = opaque;
     int i;
 
-    if (version_id != 1)
+    if (version_id < 1 || version_id > 2)
         return -EINVAL;
 
     qemu_get_8s(f, &s->id);
     qemu_get_8s(f, &s->ioregsel);
+    if (version_id == 2) {
+      /* for version 2, we get this data off of the wire */
+      qemu_get_be64s(f, &s->base_address);
+      qemu_get_be32s(f, &s->irr);
+    }
+    else {
+      /* in case we are doing version 1, we just set these to sane values */
+      s->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
+      s->irr = 0;
+    }
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         qemu_get_be64s(f, &s->ioredtbl[i]);
     }

@@ -1250,7 +1268,7 @@ IOAPICState *ioapic_init(void)
                                        ioapic_mem_write, s);
     cpu_register_physical_memory(0xfec00000, 0x1000, io_memory);
 
-    register_savevm("ioapic", 0, 1, ioapic_save, ioapic_load, s);
+    register_savevm("ioapic", 0, 2, ioapic_save, ioapic_load, s);
     qemu_register_reset(ioapic_reset, s);
 
     return s;

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel