1. Patchew
  2. Libvirt
  3. View series

[PATCH v2] interface: fix udev_device_get_sysattr_value return value check

Dmitry Frolov posted 1 patch 7 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20230908131528.135170-1-frolov@swemel.ru
There is a newer version of this series
src/interface/interface_backend_udev.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
[PATCH v2] interface: fix udev_device_get_sysattr_value return value check
Posted by Dmitry Frolov 7 months, 3 weeks ago 
Reviewing the code I found that return value of function
udev_device_get_sysattr_value() is dereferenced without a check.
udev_device_get_sysattr_value() may return NULL by number of reasons.

v2: VIR_DEBUG added, replaced STREQ(NULLSTR()) with STREQ_NULLABLE()

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 src/interface/interface_backend_udev.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
index a0485ddd21..df7066727e 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -23,6 +23,7 @@
 #include <dirent.h>
 #include <libudev.h>
 
+#include "virlog.h"
 #include "virerror.h"
 #include "virfile.h"
 #include "datatypes.h"
@@ -40,6 +41,8 @@
 
 #define VIR_FROM_THIS VIR_FROM_INTERFACE
 
+VIR_LOG_INIT("interface.interface_backend_udev");
+
 struct udev_iface_driver {
     struct udev *udev;
     /* pid file FD, ensures two copies of the driver can't use the same root */
@@ -355,10 +358,13 @@ udevConnectListAllInterfaces(virConnectPtr conn,
         g_autoptr(virInterfaceDef) def = NULL;
 
         path = udev_list_entry_get_name(dev_entry);
-        dev = udev_device_new_from_syspath(udev, path);
+        if (!(dev = udev_device_new_from_syspath(udev, path))) {
+            VIR_DEBUG("Skipping interface '%s'", NULLSTR(path));
+            continue;
+        }
         name = udev_device_get_sysname(dev);
         macaddr = udev_device_get_sysattr_value(dev, "address");
-        status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+        status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"), "up");
 
         def = udevGetMinimalDefForDevice(dev);
         if (!virConnectListAllInterfacesCheckACL(conn, def)) {
@@ -964,9 +970,9 @@ udevGetIfaceDef(struct udev *udev, const char *name)
 
     /* MTU */
     mtu_str = udev_device_get_sysattr_value(dev, "mtu");
-    if (virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) {
+    if (!mtu_str || virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                _("Could not parse MTU value '%1$s'"), mtu_str);
+                _("Could not parse MTU value '%1$s'"), NULLSTR(mtu_str));
         goto error;
     }
     ifacedef->mtu = mtu;
@@ -1089,7 +1095,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
        goto cleanup;
 
     /* Check if it's active or not */
-    status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+    status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"), "up");
 
     udev_device_unref(dev);
 
-- 
2.34.1
Re: [PATCH v2] interface: fix udev_device_get_sysattr_value return value check
Posted by Martin Kletzander 7 months, 2 weeks ago 
On Fri, Sep 08, 2023 at 04:15:29PM +0300, Dmitry Frolov wrote:
>Reviewing the code I found that return value of function
>udev_device_get_sysattr_value() is dereferenced without a check.
>udev_device_get_sysattr_value() may return NULL by number of reasons.
>
>v2: VIR_DEBUG added, replaced STREQ(NULLSTR()) with STREQ_NULLABLE()
>
>Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>---
> src/interface/interface_backend_udev.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
>diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
>index a0485ddd21..df7066727e 100644
>--- a/src/interface/interface_backend_udev.c
>+++ b/src/interface/interface_backend_udev.c
>@@ -23,6 +23,7 @@
> #include <dirent.h>
> #include <libudev.h>
>
>+#include "virlog.h"
> #include "virerror.h"
> #include "virfile.h"
> #include "datatypes.h"
>@@ -40,6 +41,8 @@
>
> #define VIR_FROM_THIS VIR_FROM_INTERFACE
>
>+VIR_LOG_INIT("interface.interface_backend_udev");
>+
> struct udev_iface_driver {
>     struct udev *udev;
>     /* pid file FD, ensures two copies of the driver can't use the same root */
>@@ -355,10 +358,13 @@ udevConnectListAllInterfaces(virConnectPtr conn,
>         g_autoptr(virInterfaceDef) def = NULL;
>
>         path = udev_list_entry_get_name(dev_entry);
>-        dev = udev_device_new_from_syspath(udev, path);
>+        if (!(dev = udev_device_new_from_syspath(udev, path))) {
>+            VIR_DEBUG("Skipping interface '%s'", NULLSTR(path));

Since you're at it, and path can be null (I guess), does it even make
sense to get the device from syspath when there is no path?  Shouldn't
we skip it right away?  And it would warrant even a VIR_DEBUG in such
case IMHO since the device is not made up, we got it from udev by
enumeration.

Also since there's going to be more VIR_DEBUG/WARN we should also
mention why the device is being skipped.

>+            continue;
>+        }
>         name = udev_device_get_sysname(dev);

As Peter pointed out this can be NULL and if this function is used for
getting the list of devices (not just the amount) this fails way later,
virGetInterface() sets an error message and we add NULL into the list
and happily carry on.  If we're checking that udev is not returning NULL
for devices that are returned by udev's enumeration then we should also
check the sysname existing if the caller wants it, something like:

         if (ifaces && !name) {
             VIR_WARN();
             continue;
         }

>         macaddr = udev_device_get_sysattr_value(dev, "address");
>-        status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
>+        status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"), "up");
>
>         def = udevGetMinimalDefForDevice(dev);
>         if (!virConnectListAllInterfacesCheckACL(conn, def)) {
>@@ -964,9 +970,9 @@ udevGetIfaceDef(struct udev *udev, const char *name)
>
>     /* MTU */
>     mtu_str = udev_device_get_sysattr_value(dev, "mtu");
>-    if (virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) {
>+    if (!mtu_str || virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) {
>         virReportError(VIR_ERR_INTERNAL_ERROR,
>-                _("Could not parse MTU value '%1$s'"), mtu_str);
>+                _("Could not parse MTU value '%1$s'"), NULLSTR(mtu_str));
>         goto error;
>     }
>     ifacedef->mtu = mtu;
>@@ -1089,7 +1095,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
>        goto cleanup;
>
>     /* Check if it's active or not */
>-    status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
>+    status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"), "up");
>
>     udev_device_unref(dev);
>
>-- 
>2.34.1
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.