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