[PATCH 0/3] Misc fixes for 8.2

[PATCH 0/3] Misc fixes for 8.2

[David Woodhouse]

Fix a use-after-free (or double-free) due to net_cleanup() freeing NICs
that don't belong to it, fix a newly-introduced launch failure with a
documented command line, and clean up code to avoid a Coverity warning.

David Woodhouse (3):
      net: do not delete nics in net_cleanup()
      vl: disable default serial when xen-console is enabled
      hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive

 hw/block/xen-block.c | 24 +++++++++++++++++++++---
 net/net.c            | 28 ++++++++++++++++++++++------
 system/vl.c          |  1 +
 3 files changed, 44 insertions(+), 9 deletions(-)

[PATCH 1/3] net: do not delete nics in net_cleanup()

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

In net_cleanup() we only need to delete the netdevs, as those may have
state which outlives Qemu when it exits, and thus may actually need to
be cleaned up on exit.

The nics, on the other hand, are owned by the device which created them.
Most devices don't bother to clean up on exit because they don't have
any state which will outlive Qemu... but XenBus devices do need to clean
up their nodes in XenStore, and do have an exit handler to delete them.

When the XenBus exit handler destroys the xen-net-device, it attempts
to delete its nic after net_cleanup() had already done so. And crashes.

Fix this by only deleting netdevs as we walk the list. As the comment
notes, we can't use QTAILQ_FOREACH_SAFE() as each deletion may remove
*multiple* entries, including the "safely" saved 'next' pointer. But
we can store the *previous* entry, since nics are safe.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
---
 net/net.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/net/net.c b/net/net.c
index c0c0cbe99e..bbe33da176 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1499,18 +1499,34 @@ static void net_vm_change_state_handler(void *opaque, bool running,
 
 void net_cleanup(void)
 {
-    NetClientState *nc;
+    NetClientState *nc, **p = &QTAILQ_FIRST(&net_clients);
 
     /*cleanup colo compare module for COLO*/
     colo_compare_cleanup();
 
-    /* We may del multiple entries during qemu_del_net_client(),
-     * so QTAILQ_FOREACH_SAFE() is also not safe here.
+    /*
+     * Walk the net_clients list and remove the netdevs but *not* any
+     * NET_CLIENT_DRIVER_NIC entries. The latter are owned by the device
+     * model which created them, and in some cases (e.g. xen-net-device)
+     * the device itself may do cleanup at exit and will be upset if we
+     * just delete its NIC from underneath it.
+     *
+     * Since qemu_del_net_client() may delete multiple entries, using
+     * QTAILQ_FOREACH_SAFE() is not safe here. The only safe pointer
+     * to keep as a bookmark is a NET_CLIENT_DRIVER_NIC entry, so keep
+     * 'p' pointing to either the head of the list, or the 'next' field
+     * of the latest NET_CLIENT_DRIVER_NIC, and operate on *p as we walk
+     * the list.
+     *
+     * The 'nc' variable isn't part of the list traversal; it's purely
+     * for convenience as too much '(*p)->' has a tendency to make the
+     * readers' eyes bleed.
      */
-    while (!QTAILQ_EMPTY(&net_clients)) {
-        nc = QTAILQ_FIRST(&net_clients);
+    while (*p) {
+        nc = *p;
         if (nc->info->type == NET_CLIENT_DRIVER_NIC) {
-            qemu_del_nic(qemu_get_nic(nc));
+            /* Skip NET_CLIENT_DRIVER_NIC entries */
+            p = &QTAILQ_NEXT(nc, next);
         } else {
             qemu_del_net_client(nc);
         }
-- 
2.41.0

[PATCH 2/3] vl: disable default serial when xen-console is enabled

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

If a Xen console is configured on the command line, do not add a default
serial port.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 system/vl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/system/vl.c b/system/vl.c
index 5af7ced2a1..8109231834 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -198,6 +198,7 @@ static const struct {
     const char *driver;
     int *flag;
 } default_list[] = {
+    { .driver = "xen-console",          .flag = &default_serial    },
     { .driver = "isa-serial",           .flag = &default_serial    },
     { .driver = "isa-parallel",         .flag = &default_parallel  },
     { .driver = "isa-fdc",              .flag = &default_floppy    },
-- 
2.41.0

Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled

[Marc-André Lureau]

Hi

On Wed, Nov 15, 2023 at 9:28 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> If a Xen console is configured on the command line, do not add a default
> serial port.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  system/vl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/system/vl.c b/system/vl.c
> index 5af7ced2a1..8109231834 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -198,6 +198,7 @@ static const struct {
>      const char *driver;
>      int *flag;
>  } default_list[] = {
> +    { .driver = "xen-console",          .flag = &default_serial    },
>      { .driver = "isa-serial",           .flag = &default_serial    },
>      { .driver = "isa-parallel",         .flag = &default_parallel  },
>      { .driver = "isa-fdc",              .flag = &default_floppy    },

Consistent with the rest of the lines (no conditional compilation nor
driver #define..)
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

btw, while quickly testing this (do we have any test for xen-console?):

$ qemu --accel kvm,xen-version=0x40011,kernel-irqchip=split -device
xen-console,chardev=foo -chardev stdio,id=foo
(and close gtk window)

Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
387        if (nc->incoming_queue) {
(gdb) bt
#0  0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
#1  0x0000555555c11a14 in qemu_del_nic (nic=0x555558b6f930) at ../net/net.c:459
#2  0x00005555559e398b in xen_netdev_unrealize (xendev=0x555558b6b510)
at ../hw/net/xen_nic.c:550
#3  0x0000555555b6e22f in xen_device_unrealize (dev=0x555558b6b510) at
../hw/xen/xen-bus.c:973
#4  0x0000555555b6e351 in xen_device_exit (n=0x555558b6b5e0, data=0x0)
at ../hw/xen/xen-bus.c:1002
#5  0x00005555560bc3fc in notifier_list_notify (list=0x5555570b5fc0
<exit_notifiers>, data=0x0) at ../util/notify.c:39
#6  0x0000555555ba1d49 in qemu_run_exit_notifiers () at ../system/runstate.c:800



--
Marc-André Lureau

Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled

[Marc-André Lureau]

Hi

On Tue, Nov 21, 2023 at 2:57 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Wed, Nov 15, 2023 at 9:28 PM David Woodhouse <dwmw2@infradead.org> wrote:
> >
> > From: David Woodhouse <dwmw@amazon.co.uk>
> >
> > If a Xen console is configured on the command line, do not add a default
> > serial port.
> >
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >  system/vl.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/system/vl.c b/system/vl.c
> > index 5af7ced2a1..8109231834 100644
> > --- a/system/vl.c
> > +++ b/system/vl.c
> > @@ -198,6 +198,7 @@ static const struct {
> >      const char *driver;
> >      int *flag;
> >  } default_list[] = {
> > +    { .driver = "xen-console",          .flag = &default_serial    },
> >      { .driver = "isa-serial",           .flag = &default_serial    },
> >      { .driver = "isa-parallel",         .flag = &default_parallel  },
> >      { .driver = "isa-fdc",              .flag = &default_floppy    },
>
> Consistent with the rest of the lines (no conditional compilation nor
> driver #define..)
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> btw, while quickly testing this (do we have any test for xen-console?):
>
> $ qemu --accel kvm,xen-version=0x40011,kernel-irqchip=split -device
> xen-console,chardev=foo -chardev stdio,id=foo
> (and close gtk window)
>
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> 387        if (nc->incoming_queue) {
> (gdb) bt
> #0  0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> #1  0x0000555555c11a14 in qemu_del_nic (nic=0x555558b6f930) at ../net/net.c:459
> #2  0x00005555559e398b in xen_netdev_unrealize (xendev=0x555558b6b510)
> at ../hw/net/xen_nic.c:550
> #3  0x0000555555b6e22f in xen_device_unrealize (dev=0x555558b6b510) at
> ../hw/xen/xen-bus.c:973
> #4  0x0000555555b6e351 in xen_device_exit (n=0x555558b6b5e0, data=0x0)
> at ../hw/xen/xen-bus.c:1002
> #5  0x00005555560bc3fc in notifier_list_notify (list=0x5555570b5fc0
> <exit_notifiers>, data=0x0) at ../util/notify.c:39
> #6  0x0000555555ba1d49 in qemu_run_exit_notifiers () at ../system/runstate.c:800

Ok, I found related "[PATCH 1/3] net: do not delete nics in net_cleanup()"



-- 
Marc-André Lureau

Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On Tue, 2023-11-21 at 14:58 +0400, Marc-André Lureau wrote:
> 
> > Consistent with the rest of the lines (no conditional compilation nor
> > driver #define..)
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks.

> > btw, while quickly testing this (do we have any test for xen-console?):
> > 
> > $ qemu --accel kvm,xen-version=0x40011,kernel-irqchip=split -device
> > xen-console,chardev=foo -chardev stdio,id=foo
> > (and close gtk window)
> > 
> > Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> > 0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> > 387        if (nc->incoming_queue) {
> > (gdb) bt
> > #0  0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> > #1  0x0000555555c11a14 in qemu_del_nic (nic=0x555558b6f930) at ../net/net.c:459
> > #2  0x00005555559e398b in xen_netdev_unrealize (xendev=0x555558b6b510)
> > at ../hw/net/xen_nic.c:550
> > #3  0x0000555555b6e22f in xen_device_unrealize (dev=0x555558b6b510) at
> > ../hw/xen/xen-bus.c:973
> > #4  0x0000555555b6e351 in xen_device_exit (n=0x555558b6b5e0, data=0x0)
> > at ../hw/xen/xen-bus.c:1002
> > #5  0x00005555560bc3fc in notifier_list_notify (list=0x5555570b5fc0
> > <exit_notifiers>, data=0x0) at ../util/notify.c:39
> > #6  0x0000555555ba1d49 in qemu_run_exit_notifiers () at ../system/runstate.c:800
> 
> Ok, I found related "[PATCH 1/3] net: do not delete nics in net_cleanup()"

Yep, and I think I saw that go by in a pull request not many hours ago,
so it should be fixed by -rc1. Thanks for testing.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled

[Paul Durrant]

On 15/11/2023 17:24, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> If a Xen console is configured on the command line, do not add a default
> serial port.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   system/vl.c | 1 +
>   1 file changed, 1 insertion(+)
> 

Reviewed-by: Paul Durrant <paul@xen.org>

[PATCH 3/3] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Coverity couldn't see that nr_existing was always going to be zero when
qemu_xen_xs_directory() returned NULL in the ENOENT case (CID 1523906).

Perhaps more to the point, neither could Peter at first glance. Improve
the code to hopefully make it clearer to Coverity and human reviewers
alike.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/block/xen-block.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 6d64ede94f..aed1d5c330 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -91,9 +91,27 @@ static bool xen_block_find_free_vdev(XenBlockDevice *blockdev, Error **errp)
 
     existing_frontends = qemu_xen_xs_directory(xenbus->xsh, XBT_NULL, fe_path,
                                                &nr_existing);
-    if (!existing_frontends && errno != ENOENT) {
-        error_setg_errno(errp, errno, "cannot read %s", fe_path);
-        return false;
+    if (!existing_frontends) {
+        if (errno == ENOENT) {
+            /*
+             * If the frontend directory doesn't exist because there are
+             * no existing vbd devices, that's fine. Just ensure that we
+             * don't dereference the NULL existing_frontends pointer, by
+             * checking that nr_existing is zero so the loop below is not
+             * entered.
+             *
+             * In fact this is redundant since nr_existing is initialized
+             * to zero, but setting it again here makes it abundantly clear
+             * to Coverity, and to the human reader who doesn't know the
+             * semantics of qemu_xen_xs_directory() off the top of their
+             * head.
+             */
+            nr_existing = 0;
+        } else {
+            /* All other errors accessing the frontend directory are fatal. */
+            error_setg_errno(errp, errno, "cannot read %s", fe_path);
+            return false;
+        }
     }
 
     memset(used_devs, 0, sizeof(used_devs));
-- 
2.41.0

Re: [PATCH 3/3] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive

[Paul Durrant]

On 15/11/2023 12:24, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Coverity couldn't see that nr_existing was always going to be zero when
> qemu_xen_xs_directory() returned NULL in the ENOENT case (CID 1523906).
> 
> Perhaps more to the point, neither could Peter at first glance. Improve
> the code to hopefully make it clearer to Coverity and human reviewers
> alike.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/block/xen-block.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
> 

Reviewed-by: Paul Durrant <paul@xen.org>