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()
v3: More checks added, to skip earlier. More verbose VIR_DEBUG.
Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
src/interface/interface_backend_udev.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
index a0485ddd21..fb6799ed94 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 */
@@ -354,11 +357,20 @@ udevConnectListAllInterfaces(virConnectPtr conn,
const char *macaddr;
g_autoptr(virInterfaceDef) def = NULL;
- path = udev_list_entry_get_name(dev_entry);
- dev = udev_device_new_from_syspath(udev, path);
- name = udev_device_get_sysname(dev);
+ if (!(path = udev_list_entry_get_name(dev_entry))) {
+ VIR_DEBUG("Skipping interface, path == NULL");
+ continue;
+ }
+ if (!(dev = udev_device_new_from_syspath(udev, path))) {
+ VIR_DEBUG("Skipping interface '%s', dev == NULL", path);
+ continue;
+ }
+ if (!(name = udev_device_get_sysname(dev))) {
+ VIR_DEBUG("Skipping interface '%s', name == NULL", path);
+ 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 +976,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 +1101,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
On Tue, Sep 12, 2023 at 03:56:47PM +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() >v3: More checks added, to skip earlier. More verbose VIR_DEBUG. > >Signed-off-by: Dmitry Frolov <frolov@swemel.ru> Reviewed-by: Martin Kletzander <mkletzan@redhat.com> >--- > src/interface/interface_backend_udev.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > >diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c >index a0485ddd21..fb6799ed94 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 */ >@@ -354,11 +357,20 @@ udevConnectListAllInterfaces(virConnectPtr conn, > const char *macaddr; > g_autoptr(virInterfaceDef) def = NULL; > >- path = udev_list_entry_get_name(dev_entry); >- dev = udev_device_new_from_syspath(udev, path); >- name = udev_device_get_sysname(dev); >+ if (!(path = udev_list_entry_get_name(dev_entry))) { >+ VIR_DEBUG("Skipping interface, path == NULL"); >+ continue; >+ } >+ if (!(dev = udev_device_new_from_syspath(udev, path))) { >+ VIR_DEBUG("Skipping interface '%s', dev == NULL", path); >+ continue; >+ } >+ if (!(name = udev_device_get_sysname(dev))) { >+ VIR_DEBUG("Skipping interface '%s', name == NULL", path); >+ 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 +976,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 +1101,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 >
© 2016 - 2024 Red Hat, Inc.