[02/21] net/virtio: Fix failover error handling crash bugs
diff mbox series

Message ID 20191130194240.10517-3-armbru@redhat.com
State New
Headers show
Series
  • Error handling fixes, may contain 4.2 material
Related show

Commit Message

Markus Armbruster Nov. 30, 2019, 7:42 p.m. UTC
Functions that take an Error ** parameter to pass an error to the
caller expect the parameter to point to null.
failover_replug_primary() violates this precondition in several
places:

* After qemu_opts_from_qdict() failed, *errp is no longer null.
  Passing it to error_setg() is wrong, and will trip the assertion in
  error_setv().  Messed up in commit 150ab54aa6 "net/virtio: fix
  re-plugging of primary device".  Simply drop the error_setg().

* Passing @errp to qemu_opt_set_bool(), hotplug_handler_pre_plug(),
  and hotplug_handler_plug() is wrong.  If one of the first two fails,
  *errp is no longer null.  Risks tripping the same assertion.
  Moreover, continuing after such errors is unsafe.  Messed up in
  commit 9711cd0dfc "net/virtio: add failover support".  Fix by
  handling each error properly.

failover_replug_primary() crashes when passed a null @errp.  Also
messed up in commit 9711cd0dfc.  This bug can't bite as no caller
actually passes null.  Fix it anyway.

Fixes: 9711cd0dfc3fa414f7f64935713c07134ae67971
Fixes: 150ab54aa6934583180f88a2bd540bc6fc4fbff3
Cc: Jens Freimann <jfreimann@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/net/virtio-net.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

Jens Freimann Dec. 2, 2019, 9:53 a.m. UTC | #1
On Sat, Nov 30, 2019 at 08:42:21PM +0100, Markus Armbruster wrote:
>Functions that take an Error ** parameter to pass an error to the
>caller expect the parameter to point to null.
>failover_replug_primary() violates this precondition in several
>places:
>
>* After qemu_opts_from_qdict() failed, *errp is no longer null.
>  Passing it to error_setg() is wrong, and will trip the assertion in
>  error_setv().  Messed up in commit 150ab54aa6 "net/virtio: fix
>  re-plugging of primary device".  Simply drop the error_setg().
>
>* Passing @errp to qemu_opt_set_bool(), hotplug_handler_pre_plug(),
>  and hotplug_handler_plug() is wrong.  If one of the first two fails,
>  *errp is no longer null.  Risks tripping the same assertion.
>  Moreover, continuing after such errors is unsafe.  Messed up in
>  commit 9711cd0dfc "net/virtio: add failover support".  Fix by
>  handling each error properly.
>
>failover_replug_primary() crashes when passed a null @errp.  Also
>messed up in commit 9711cd0dfc.  This bug can't bite as no caller
>actually passes null.  Fix it anyway.
>
>Fixes: 9711cd0dfc3fa414f7f64935713c07134ae67971
>Fixes: 150ab54aa6934583180f88a2bd540bc6fc4fbff3
>Cc: Jens Freimann <jfreimann@redhat.com>
>Cc: Michael S. Tsirkin <mst@redhat.com>
>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>---
> hw/net/virtio-net.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>

Thanks Markus!

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

Patch
diff mbox series

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 87088ba374..db3d7c38e6 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2795,6 +2795,7 @@  static bool failover_unplug_primary(VirtIONet *n)
 
 static bool failover_replug_primary(VirtIONet *n, Error **errp)
 {
+    Error *err = NULL;
     HotplugHandler *hotplug_ctrl;
     PCIDevice *pdev = PCI_DEVICE(n->primary_dev);
 
@@ -2806,27 +2807,33 @@  static bool failover_replug_primary(VirtIONet *n, Error **errp)
                 qemu_find_opts("device"),
                 n->primary_device_dict, errp);
         if (!n->primary_device_opts) {
-            error_setg(errp, "virtio_net: couldn't find primary device opts");
-            goto out;
+            return false;
         }
     }
     n->primary_bus = n->primary_dev->parent_bus;
     if (!n->primary_bus) {
         error_setg(errp, "virtio_net: couldn't find primary bus");
-        goto out;
+        return false;
     }
     qdev_set_parent_bus(n->primary_dev, n->primary_bus);
     n->primary_should_be_hidden = false;
     qemu_opt_set_bool(n->primary_device_opts,
-                      "partially_hotplugged", true, errp);
+                      "partially_hotplugged", true, &err);
+    if (err) {
+        goto out;
+    }
     hotplug_ctrl = qdev_get_hotplug_handler(n->primary_dev);
     if (hotplug_ctrl) {
-        hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
+        hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, &err);
+        if (err) {
+            goto out;
+        }
         hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
     }
 
 out:
-    return *errp == NULL;
+    error_propagate(errp, err);
+    return !err;
 }
 
 static void virtio_net_handle_migration_primary(VirtIONet *n,