[PATCH] interface: fix udev_device_get_sysattr_value return value check

Dmitry Frolov posted 1 patch 2 weeks, 5 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20230908100642.78147-1-frolov@swemel.ru
There is a newer version of this series
src/interface/interface_backend_udev.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
[PATCH] interface: fix udev_device_get_sysattr_value return value check
Posted by Dmitry Frolov 2 weeks, 5 days 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.

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

diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
index a0485ddd21..c820b3ccdf 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -355,10 +355,11 @@ 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)))
+            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(NULLSTR(udev_device_get_sysattr_value(dev, "operstate")), "up");
 
         def = udevGetMinimalDefForDevice(dev);
         if (!virConnectListAllInterfacesCheckACL(conn, def)) {
@@ -964,9 +965,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 +1090,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(NULLSTR(udev_device_get_sysattr_value(dev, "operstate")), "up");
 
     udev_device_unref(dev);
 
-- 
2.34.1
Re: [PATCH] interface: fix udev_device_get_sysattr_value return value check
Posted by Peter Krempa 2 weeks, 5 days ago
On Fri, Sep 08, 2023 at 13:06:43 +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.
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>  src/interface/interface_backend_udev.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
> index a0485ddd21..c820b3ccdf 100644
> --- a/src/interface/interface_backend_udev.c
> +++ b/src/interface/interface_backend_udev.c
> @@ -355,10 +355,11 @@ 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)))
> +            continue;

IMO this warants at least a VIR_DEBUG stating that the returned
'dev' struct is NULL and thus we're skipping a whole interface.

>          name = udev_device_get_sysname(dev);

This is also documented as returning NULL on error, but virGetInterface
actually checks it before use.

>          macaddr = udev_device_get_sysattr_value(dev, "address");
> -        status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
> +        status = STREQ(NULLSTR(udev_device_get_sysattr_value(dev, "operstate")), "up");

For comparing strings which can be NULL we have STREQ_NULLABLE and
STRNEQ_NULLABLE.