diff mbox series

[PULL,v2,11/55] hw/qdev: Check machine_hotplug_handler in hotplug_unplug_allowed_common

Message ID 20250113173604.46931-7-philmd@linaro.org
State Accepted
Commit 937874a83d149a1b871a569d6abded3fdd302267
Headers show
Series None | expand

Commit Message

Philippe Mathieu-Daudé Jan. 13, 2025, 5:36 p.m. UTC 
From: Akihiko Odaki <akihiko.odaki@daynix.com>

Commit 03fcbd9dc508 ("qdev: Check for the availability of a hotplug
controller before adding a device") says:

 > The qdev_unplug() function contains a g_assert(hotplug_ctrl)
 > statement, so QEMU crashes when the user tries to device_add +
 > device_del a device that does not have a corresponding hotplug
 > controller.

 > The code in qdev_device_add() already checks whether the bus has a
 > proper hotplug controller, but for devices that do not have a
 > corresponding bus, here is no appropriate check available yet. In that
 > case we should check whether the machine itself provides a suitable
 > hotplug controller and refuse to plug the device if none is available.

However, it forgot to add the corresponding check to qdev_unplug().

Check the machine hotplug handler once in the common
qdev_hotplug_unplug_allowed_common() helper so both hotplug
and hot-unplug path are covered.

Fixes: 7716b8ca74 ("qdev: HotplugHandler: Add support for unplugging BUS-less devices")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
[PMD: Split from bigger patch, part 6/6]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20250110091908.64454-7-philmd@linaro.org>
---
 hw/core/qdev-hotplug.c | 10 ++++++++++
 system/qdev-monitor.c  | 14 +++-----------
 2 files changed, 13 insertions(+), 11 deletions(-)

Comments

Peter Maydell Feb. 4, 2025, 1:58 p.m. UTC | #1 
On Mon, 13 Jan 2025 at 17:38, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> Commit 03fcbd9dc508 ("qdev: Check for the availability of a hotplug
> controller before adding a device") says:
>
>  > The qdev_unplug() function contains a g_assert(hotplug_ctrl)
>  > statement, so QEMU crashes when the user tries to device_add +
>  > device_del a device that does not have a corresponding hotplug
>  > controller.
>
>  > The code in qdev_device_add() already checks whether the bus has a
>  > proper hotplug controller, but for devices that do not have a
>  > corresponding bus, here is no appropriate check available yet. In that
>  > case we should check whether the machine itself provides a suitable
>  > hotplug controller and refuse to plug the device if none is available.
>
> However, it forgot to add the corresponding check to qdev_unplug().
>
> Check the machine hotplug handler once in the common
> qdev_hotplug_unplug_allowed_common() helper so both hotplug
> and hot-unplug path are covered.

> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index e27d25c5859..861c25c855f 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -684,17 +684,9 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>      dev = qdev_new(driver);
>
>      /* Check whether the hotplug is allowed by the machine */
> -    if (phase_check(PHASE_MACHINE_READY)) {
> -        if (!qdev_hotplug_allowed(dev, bus, errp)) {
> -            goto err_del_dev;
> -        }
> -
> -        if (!bus && !qdev_get_machine_hotplug_handler(dev)) {
> -            /* No bus, no machine hotplug handler --> device is not hotpluggable */
> -            error_setg(errp, "Device '%s' can not be hotplugged on this machine",
> -                       driver);
> -            goto err_del_dev;
> -        }
> +    if (phase_check(PHASE_MACHINE_READY) &&
> +        !qdev_hotplug_allowed(dev, bus, errp)) {
> +        goto err_del_dev;
>      }
>

Hi; Coverity notices a NULL-check related issue as a
result of this change. Here we add a call to
qdev_hotplug_allowed() that assumes that dev is not NULL.
But in the code at err_del_dev we do check whether
dev is NULL. So either this code is wrong (and could be
dereferencing NULL) or the code at err_del_dev is
doing a pointless NULL check.

Since qdev_new() can't fail, it's the err_del_dev
code that's wrong, I think.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/core/qdev-hotplug.c b/hw/core/qdev-hotplug.c
index f6422cd0e4e..ff176dc1bb3 100644
--- a/hw/core/qdev-hotplug.c
+++ b/hw/core/qdev-hotplug.c
@@ -48,6 +48,16 @@  static bool qdev_hotplug_unplug_allowed_common(DeviceState *dev, BusState *bus,
                        bus->name);
             return false;
         }
+    } else {
+        if (!qdev_get_machine_hotplug_handler(dev)) {
+            /*
+             * No bus, no machine hotplug handler --> device is not hotpluggable
+             */
+            error_setg(errp,
+                       "Device '%s' can not be hotplugged on this machine",
+                       object_get_typename(OBJECT(dev)));
+            return false;
+        }
     }
 
     return true;
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index e27d25c5859..861c25c855f 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -684,17 +684,9 @@  DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     dev = qdev_new(driver);
 
     /* Check whether the hotplug is allowed by the machine */
-    if (phase_check(PHASE_MACHINE_READY)) {
-        if (!qdev_hotplug_allowed(dev, bus, errp)) {
-            goto err_del_dev;
-        }
-
-        if (!bus && !qdev_get_machine_hotplug_handler(dev)) {
-            /* No bus, no machine hotplug handler --> device is not hotpluggable */
-            error_setg(errp, "Device '%s' can not be hotplugged on this machine",
-                       driver);
-            goto err_del_dev;
-        }
+    if (phase_check(PHASE_MACHINE_READY) &&
+        !qdev_hotplug_allowed(dev, bus, errp)) {
+        goto err_del_dev;
     }
 
     /*