Re: [PATCH] hw/char/virtio-serial-bus: fix guest-triggerable OOM in control_out()

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 23, 2026 at 01:54:14PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 23, 2026 at 08:36:30AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 22, 2026 at 05:30:33PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Jun 22, 2026 at 06:11:44PM +0200, Laurent Vivier wrote:
> > > > A malicious guest can craft virtqueue descriptors with arbitrary lengths.
> > > > control_out() calls iov_size() on the guest-supplied scatter-gather list
> > > > and passes the result directly to g_malloc(), allowing a guest to force
> > > > QEMU to attempt multi-gigabyte allocations and crash the host process.
> > > > 
> > > > Fix this by copying at most sizeof(struct virtio_console_control) into a
> > > > stack-local variable instead of allocating a buffer sized by the guest.
> > > > handle_control_message() only accesses the fixed-size id, event, and
> > > > value fields, so no data beyond the struct was ever needed.
> > > 
> > > Does anyone have thoughts on whether we should treat guest initiated
> > > unbounded allocs as a security issue ?
> > > 
> > > IIUC, this flaw would require root in the guest OS in order to craft
> > > the malicious virtqueue descriptors.
> > > 
> > > A self-initiated crash triggered by root would not historically
> > > be enough justification for CVE. We would require it to be triggered
> > > by unprivileged user.
> > > 
> > > Nested virt with device assignment could change that equation though
> > > as the L2 guest could be considered an unpriv user from the L1 POV.
> > > 
> > > Also in theory the large alloc might be large enough to consume all
> > > host RAM but not large enough to trigger OOM kill of QEMU. This might
> > > impact operation of other co-located VMs on the same host.
> > 
> > Don't see why not. We can treat it as low priority and no embargo.
> 
> Is there any point in assigning "low" severity  CVEs ? I'm not
> convinced downstream vendors have capacity to deal with "low"
> CVEs anymore even if we published them.

Let's ask some downstreams)

qemu security policy really should spell out what is, or is not,
considered worth a CVE.

> > 
> > > Anyone think this is bad enough to justify a CVE ? Or should we treat
> > > these OOM scenarios maerely as "hardening" bugs, where they require
> > > 'root' in the L1 guest ?
> > 
> > 
> > > > Cc: qemu-stable@nongnu.org
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3585
> > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > ---
> > > >  hw/char/virtio-serial-bus.c | 34 +++++++---------------------------
> > > >  1 file changed, 7 insertions(+), 27 deletions(-)
> > > > 
> > > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > > > index cd234dc6db1d..c1973f0248fc 100644
> > > > --- a/hw/char/virtio-serial-bus.c
> > > > +++ b/hw/char/virtio-serial-bus.c
> > > > @@ -344,22 +344,16 @@ void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle)
> > > >  }
> > > >  
> > > >  /* Guest wants to notify us of some event */
> > > > -static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> > > > +static void handle_control_message(VirtIOSerial *vser,
> > > > +                                   struct virtio_console_control *gcpkt)
> > > >  {
> > > >      VirtIODevice *vdev = VIRTIO_DEVICE(vser);
> > > >      struct VirtIOSerialPort *port;
> > > >      VirtIOSerialPortClass *vsc;
> > > > -    struct virtio_console_control cpkt, *gcpkt;
> > > > +    struct virtio_console_control cpkt;
> > > >      uint8_t *buffer;
> > > >      size_t buffer_len;
> > > >  
> > > > -    gcpkt = buf;
> > > > -
> > > > -    if (len < sizeof(cpkt)) {
> > > > -        /* The guest sent an invalid control packet */
> > > > -        return;
> > > > -    }
> > > > -
> > > >      cpkt.event = virtio_lduw_p(vdev, &gcpkt->event);
> > > >      cpkt.value = virtio_lduw_p(vdev, &gcpkt->value);
> > > >  
> > > > @@ -457,41 +451,27 @@ static void control_in(VirtIODevice *vdev, VirtQueue *vq)
> > > >  
> > > >  static void control_out(VirtIODevice *vdev, VirtQueue *vq)
> > > >  {
> > > > +    struct virtio_console_control cpkt;
> > > >      VirtQueueElement *elem;
> > > >      VirtIOSerial *vser;
> > > > -    uint8_t *buf;
> > > >      size_t len;
> > > >  
> > > >      vser = VIRTIO_SERIAL(vdev);
> > > >  
> > > > -    len = 0;
> > > > -    buf = NULL;
> > > >      for (;;) {
> > > > -        size_t cur_len;
> > > > -
> > > >          elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > > >          if (!elem) {
> > > >              break;
> > > >          }
> > > >  
> > > > -        cur_len = iov_size(elem->out_sg, elem->out_num);
> > > > -        /*
> > > > -         * Allocate a new buf only if we didn't have one previously or
> > > > -         * if the size of the buf differs
> > > > -         */
> > > > -        if (cur_len > len) {
> > > > -            g_free(buf);
> > > > -
> > > > -            buf = g_malloc(cur_len);
> > > > -            len = cur_len;
> > > > +        len = iov_to_buf(elem->out_sg, elem->out_num, 0, &cpkt, sizeof(cpkt));
> > > > +        if (len == sizeof(cpkt)) {
> > > > +            handle_control_message(vser, &cpkt);
> > > >          }
> > > > -        iov_to_buf(elem->out_sg, elem->out_num, 0, buf, cur_len);
> > > >  
> > > > -        handle_control_message(vser, buf, cur_len);
> > > >          virtqueue_push(vq, elem, 0);
> > > >          g_free(elem);
> > > >      }
> > > > -    g_free(buf);
> > > >      virtio_notify(vdev, vq);
> > > >  }
> > > >  
> > > > -- 
> > > > 2.54.0
> > > > 
> > > > 
> > > 
> > > With regards,
> > > Daniel
> > > -- 
> > > |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
> > > |: https://libvirt.org          ~~          https://entangle-photo.org :|
> > > |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
> > 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
> |: https://libvirt.org          ~~          https://entangle-photo.org :|
> |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|