Skip to contentSkip to conversation

virtio-serial: don't crash on invalid input

PATCH In review Michael S. Tsirkin 2 participants 6 messages Mar 22, 2011 – Mar 23, 2011

lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

PATCH 1/1 virtio-serial: don't crash on invalid input

Fix crash on invalid input in virtio-serial.
Discovered by code review, untested.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/virtio-serial-bus.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
hw/virtio-serial-bus.c +3 -0
index e0bf6c5..8807a2f 100644
@@ -654,6 +654,9 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
+        if (!port) {
+            return -EINVAL;
+        }
 
         port->guest_connected = qemu_get_byte(f);
         host_connected = qemu_get_byte(f);
Signature · 1 line
1.7.3.2.91.g446ac
#1 lore raw
Quoted 13 lines · Michael S. Tsirkin

Fix crash on invalid input in virtio-serial.
Discovered by code review, untested.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/virtio-serial-bus.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
hw/virtio-serial-bus.c +0 -0
index e0bf6c5..8807a2f 100644

hw/virtio-serial-bus.c static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) · line 659

Quoted 6 lines
hw/virtio-serial-bus.c +3 -0
@@ -654,6 +654,9 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
+        if (!port) {
+            return -EINVAL;
+        }

Just before this, we matched the ports_map which would bail out if the corresponding port isn't avl. in the destination, so this check is made redundant.

Amit

author
#2 lore raw
Quoted 25 lines · Amit Shah
Quoted 13 lines · Michael S. Tsirkin

Fix crash on invalid input in virtio-serial.
Discovered by code review, untested.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/virtio-serial-bus.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
hw/virtio-serial-bus.c +0 -0
index e0bf6c5..8807a2f 100644

hw/virtio-serial-bus.c static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) · line 659

Quoted 6 lines
hw/virtio-serial-bus.c +3 -0
@@ -654,6 +654,9 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
+        if (!port) {
+            return -EINVAL;
+        }

Just before this, we matched the ports_map which would bail out if the corresponding port isn't avl. in the destination, so this check is made redundant.

Quoted 1 line

Amit

You are trusting the remote here, this is a security problem. A malicious remote will always be able to create arbitrary guest state, but it should not be able to corrupt the host.

Signature · 1 line
MST
#3 lore raw
In reply to Michael S. Tsirkin
Quoted 17 lines · Michael S. Tsirkin
Quoted 6 lines · Amit Shah
Quoted 5 lines · Michael S. Tsirkin

Fix crash on invalid input in virtio-serial.
Discovered by code review, untested.

(quoted diff) +0 -0
@@ -654,6 +654,9 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
Quoted 9 lines

hw/virtio-serial-bus.c static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) · line 659

Quoted 5 lines
hw/virtio-serial-bus.c +3 -0
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
+        if (!port) {
+            return -EINVAL;
+        }

Just before this, we matched the ports_map which would bail out if the corresponding port isn't avl. in the destination, so this check is made redundant.

Quoted 3 lines

You are trusting the remote here, this is a security problem. A malicious remote will always be able to create arbitrary guest state, but it should not be able to corrupt the host.

I'm still unsure if we'll be able to achieve much if our primary defence goes down: we currently primarily rely on libvirt and selinux to ensure we're in a sane state and any incoming migration is from a properly-initialised qemu instance.

If we're receiving data from an untrusted qemu instance or some random sender, we're doomed anyway.

Amit

author
#4 lore raw
Quoted 27 lines · Amit Shah
Quoted 17 lines · Michael S. Tsirkin
Quoted 6 lines · Amit Shah
Quoted 5 lines · Michael S. Tsirkin

Fix crash on invalid input in virtio-serial.
Discovered by code review, untested.

(quoted diff) +0 -0
@@ -654,6 +654,9 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
Quoted 9 lines

hw/virtio-serial-bus.c static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) · line 659

Quoted 5 lines
hw/virtio-serial-bus.c +3 -0
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
+        if (!port) {
+            return -EINVAL;
+        }

Just before this, we matched the ports_map which would bail out if the corresponding port isn't avl. in the destination, so this check is made redundant.

Quoted 3 lines

You are trusting the remote here, this is a security problem. A malicious remote will always be able to create arbitrary guest state, but it should not be able to corrupt the host.

I'm still unsure if we'll be able to achieve much if our primary defence goes down: we currently primarily rely on libvirt and selinux to ensure we're in a sane state and any incoming migration is from a properly-initialised qemu instance.

Quoted 4 lines

If we're receiving data from an untrusted qemu instance or some random sender, we're doomed anyway.

Amit

I think we need defence in depth: qemu must validate all input to the point where remote can not crash qemu/host, selinux must restrict what qemu can do if that fails.

Signature · 1 line
MST