| Message ID | 20110322163250.GA6426@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 22, 2011 at 10:25:06PM +0530, Amit Shah wrote: > On (Tue) 22 Mar 2011 [18:32:50], Michael S. Tsirkin wrote: > > Fix crash on invalid input in virtio-serial. > > Discovered by code review, untested. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > hw/virtio-serial-bus.c | 3 +++ > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > > index e0bf6c5..8807a2f 100644 > > --- a/hw/virtio-serial-bus.c > > +++ b/hw/virtio-serial-bus.c > > @@ -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 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.
On (Tue) 22 Mar 2011 [18:32:50], Michael S. Tsirkin wrote: > Fix crash on invalid input in virtio-serial. > Discovered by code review, untested. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Acked-by: Amit Shah <amit.shah@redhat.com> Amit
On (Wed) 23 Mar 2011 [11:56:57], Michael S. Tsirkin wrote: > On Tue, Mar 22, 2011 at 10:25:06PM +0530, Amit Shah wrote: > > On (Tue) 22 Mar 2011 [18:32:50], Michael S. Tsirkin wrote: > > > Fix crash on invalid input in virtio-serial. > > > Discovered by code review, untested. > > > @@ -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. > > 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
On Wed, Mar 23, 2011 at 09:39:46PM +0530, Amit Shah wrote: > On (Wed) 23 Mar 2011 [11:56:57], Michael S. Tsirkin wrote: > > On Tue, Mar 22, 2011 at 10:25:06PM +0530, Amit Shah wrote: > > > On (Tue) 22 Mar 2011 [18:32:50], Michael S. Tsirkin wrote: > > > > Fix crash on invalid input in virtio-serial. > > > > Discovered by code review, untested. > > > > > @@ -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. > > > > 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 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.
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index e0bf6c5..8807a2f 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -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);
Fix crash on invalid input in virtio-serial. Discovered by code review, untested. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/virtio-serial-bus.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)