Patchwork virtio-serial: don't crash on invalid input

login
register
mail settings
Submitter Michael S. Tsirkin
Date March 22, 2011, 4:32 p.m.
Message ID <20110322163250.GA6426@redhat.com>
Download mbox | patch
Permalink /patch/87945/
State New
Headers show

Comments

Michael S. Tsirkin - March 22, 2011, 4:32 p.m.
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(-)
Michael S. Tsirkin - March 23, 2011, 9:56 a.m.
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.
Amit Shah - March 23, 2011, 4:07 p.m.
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
Amit Shah - March 23, 2011, 4:09 p.m.
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
Michael S. Tsirkin - March 23, 2011, 4:18 p.m.
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.

Patch

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);