* [Qemu-devel] [PATCH] virtio-serial: don't crash on invalid input
@ 2011-03-22 16:32 Michael S. Tsirkin
2011-03-22 16:55 ` [Qemu-devel] " Amit Shah
2011-03-23 16:07 ` Amit Shah
0 siblings, 2 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2011-03-22 16:32 UTC (permalink / raw)
To: qemu-devel, Amit Shah
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;
+ }
port->guest_connected = qemu_get_byte(f);
host_connected = qemu_get_byte(f);
--
1.7.3.2.91.g446ac
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] Re: [PATCH] virtio-serial: don't crash on invalid input
2011-03-22 16:32 [Qemu-devel] [PATCH] virtio-serial: don't crash on invalid input Michael S. Tsirkin
@ 2011-03-22 16:55 ` Amit Shah
2011-03-23 9:56 ` Michael S. Tsirkin
2011-03-23 16:07 ` Amit Shah
1 sibling, 1 reply; 6+ messages in thread
From: Amit Shah @ 2011-03-22 16:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [Qemu-devel] Re: [PATCH] virtio-serial: don't crash on invalid input
2011-03-22 16:55 ` [Qemu-devel] " Amit Shah
@ 2011-03-23 9:56 ` Michael S. Tsirkin
2011-03-23 16:09 ` Amit Shah
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2011-03-23 9:56 UTC (permalink / raw)
To: Amit Shah; +Cc: qemu-devel
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.
--
MST
^ permalink raw reply [flat|nested] 6+ messages in thread* [Qemu-devel] Re: [PATCH] virtio-serial: don't crash on invalid input
2011-03-23 9:56 ` Michael S. Tsirkin
@ 2011-03-23 16:09 ` Amit Shah
2011-03-23 16:18 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Amit Shah @ 2011-03-23 16:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [Qemu-devel] Re: [PATCH] virtio-serial: don't crash on invalid input
2011-03-23 16:09 ` Amit Shah
@ 2011-03-23 16:18 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2011-03-23 16:18 UTC (permalink / raw)
To: Amit Shah; +Cc: qemu-devel
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.
--
MST
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH] virtio-serial: don't crash on invalid input
2011-03-22 16:32 [Qemu-devel] [PATCH] virtio-serial: don't crash on invalid input Michael S. Tsirkin
2011-03-22 16:55 ` [Qemu-devel] " Amit Shah
@ 2011-03-23 16:07 ` Amit Shah
1 sibling, 0 replies; 6+ messages in thread
From: Amit Shah @ 2011-03-23 16:07 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-23 16:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-22 16:32 [Qemu-devel] [PATCH] virtio-serial: don't crash on invalid input Michael S. Tsirkin
2011-03-22 16:55 ` [Qemu-devel] " Amit Shah
2011-03-23 9:56 ` Michael S. Tsirkin
2011-03-23 16:09 ` Amit Shah
2011-03-23 16:18 ` Michael S. Tsirkin
2011-03-23 16:07 ` Amit Shah
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).