qemu: Alter condition to avoid possible NULL deref

[libvirt] [PATCH] qemu: Alter condition to avoid possible NULL deref

Posted by John Ferlan 6 years, 2 months ago

Commit 'f0f2a5ec2' neglected to adjust the if condition to split
out the possibility that the @watchdog is NULL when altering the
message to add detail about the model.

Just split out the condition and use previous/original message, but
with the new message code.

Found by Coverity

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/qemu/qemu_hotplug.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c7bf25eee..3291ce613 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5159,11 +5159,16 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
     virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
     qemuDomainObjPrivatePtr priv = vm->privateData;
 
+    if (!watchdog) {
+        virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+                       _("watchdog device not present in domain configuration"));
+        return -1;
+    }
+
     /* While domains can have up to one watchdog, the one supplied by the user
      * doesn't necessarily match the one domain has. Refuse to detach in such
      * case. */
-    if (!(watchdog &&
-          watchdog->model == dev->model &&
+    if (!(watchdog->model == dev->model &&
           watchdog->action == dev->action &&
           virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) {
         virReportError(VIR_ERR_DEVICE_MISSING,
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Alter condition to avoid possible NULL deref

Posted by Chen Hanxiao 6 years, 2 months ago

At 2018-02-09 23:33:38, "John Ferlan" <jferlan@redhat.com> wrote:
>Commit 'f0f2a5ec2' neglected to adjust the if condition to split
>out the possibility that the @watchdog is NULL when altering the
>message to add detail about the model.
>
>Just split out the condition and use previous/original message, but
>with the new message code.
>
>Found by Coverity
>
>Signed-off-by: John Ferlan <jferlan@redhat.com>
>---
> src/qemu/qemu_hotplug.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index c7bf25eee..3291ce613 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -5159,11 +5159,16 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>     virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
>     qemuDomainObjPrivatePtr priv = vm->privateData;
> 
>+    if (!watchdog) {
>+        virReportError(VIR_ERR_DEVICE_MISSING, "%s",
>+                       _("watchdog device not present in domain configuration"));
>+        return -1;
>+    }
>+
>     /* While domains can have up to one watchdog, the one supplied by the user
>      * doesn't necessarily match the one domain has. Refuse to detach in such
>      * case. */
>-    if (!(watchdog &&
>-          watchdog->model == dev->model &&
>+    if (!(watchdog->model == dev->model &&
>           watchdog->action == dev->action &&
>           virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) {
>         virReportError(VIR_ERR_DEVICE_MISSING,
>-- 

Reviewed-by: Chen Hanxiao <chenhanxiao@gmail.com>

Regards,
- Chen

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Alter condition to avoid possible NULL deref

Posted by Michal Privoznik 6 years, 2 months ago

On 02/09/2018 04:33 PM, John Ferlan wrote:
> Commit 'f0f2a5ec2' neglected to adjust the if condition to split
> out the possibility that the @watchdog is NULL when altering the
> message to add detail about the model.
> 
> Just split out the condition and use previous/original message, but
> with the new message code.
> 
> Found by Coverity
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index c7bf25eee..3291ce613 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5159,11 +5159,16 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>      virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>  
> +    if (!watchdog) {
> +        virReportError(VIR_ERR_DEVICE_MISSING, "%s",
> +                       _("watchdog device not present in domain configuration"));
> +        return -1;
> +    }
> +
>      /* While domains can have up to one watchdog, the one supplied by the user
>       * doesn't necessarily match the one domain has. Refuse to detach in such
>       * case. */
> -    if (!(watchdog &&
> -          watchdog->model == dev->model &&
> +    if (!(watchdog->model == dev->model &&
>            watchdog->action == dev->action &&
>            virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) {
>          virReportError(VIR_ERR_DEVICE_MISSING,
> 

Oh I see now. The problem is not with the condition, but this
virReportError() uses watchdog->model (for better message) which is
dangerous in case @watchdog == NULL. I though that there's something bad
with the condition itself.

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list