From 6bf725c07c25b56937c494b6c7e83e6ca27ec54c Mon Sep 17 00:00:00 2001

Message-Id: <6bf725c07c25b56937c494b6c7e83e6ca27ec54c@dist-git>

From: Michal Privoznik <mprivozn@redhat.com>

Date: Mon, 8 Mar 2021 12:57:31 +0100

Subject: [PATCH] virdevmapper: Don't use libdevmapper to obtain dependencies

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

CVE-2020-14339

When building domain's private /dev in a namespace, libdevmapper

is consulted for getting full dependency tree of domain's disks.

The reason is that for a multipath devices all dependent devices

must be created in the namespace and allowed in CGroups.

However, this approach is very fragile as building of namespace

happens in the forked off child process, after mass close of FDs

and just before dropping privileges and execing QEMU. And it so

happens that when calling libdevmapper APIs, one of them opens

/dev/mapper/control and saves the FD into a global variable. The

FD is kept open until the lib is unlinked or dm_lib_release() is

called explicitly. We are doing neither.

However, the virDevMapperGetTargets() function is called also

from libvirtd (when setting up CGroups) and thus has to be thread

safe. Unfortunately, libdevmapper APIs are not thread safe (nor

async signal safe) and thus we can't use them. Reimplement what

libdevmapper would do using plain C (ioctl()-s, /proc/devices

parsing, /dev/mapper dirwalking, and so on).

Fixes: a30078cb832646177defd256e77c632905f1e6d0

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1858260

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

(cherry picked from commit 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e)

Conflicts:

- po/POTFILES.in: This file doesn't exist, only POTFILES does.

- src/util/virdevmapper.c: Couple of problems. Firstly,

  03c532cf9711dd6ad35380455a77141ef7d492ab isn't backported, so

  the header files include looks a bit different. Secondly,

  91d88aaf23994691ea94973dd988ca5825c1e1b1 isn't backported, so

  we have to stick with virAsprintf() instead of

  g_strdup_printf(). Thirdly, virStrncpy() behaves a bit

  differently: with the old code it returns pointer to copied

  string or NULL, with the new code (cherry-picked one) it

  already returns an integer, and on the top of that, specifying

  -1 for the source len means strlen() is used under the hood

  (see 6c0d0210cbcd5d647f0d882c07f077d444bc707d and

  7d70a63b947e9a654a4e3fffa0ffa355f5549ec7 - neither is

  backported).

In addition, I had to de-GLib the code. In RHEL-7 there is no

g_autofree, G_STATIC_ASSERT, g_new0, g_strdup, g_strdup_printf,

or g_steal_pointer. I had switch these to their old counterparts

we used to use.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1933557

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Message-Id: <3c7fd3071d8a9a58c8716f850880e3a0869243d3.1615203117.git.mprivozn@redhat.com>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

---

 po/POTFILES             |   1 +

 src/util/virdevmapper.c | 315 +++++++++++++++++++++++++++++-----------

 2 files changed, 228 insertions(+), 88 deletions(-)

diff --git a/po/POTFILES b/po/POTFILES

index be2874487c..82a08da04a 100644

--- a/po/POTFILES

+++ b/po/POTFILES

@@ -206,6 +206,7 @@ src/util/vircommand.c

 src/util/virconf.c

 src/util/vircrypto.c

 src/util/virdbus.c

+src/util/virdevmapper.c

 src/util/virdnsmasq.c

 src/util/virerror.c

 src/util/virerror.h

diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c

index 50d12ea0f1..ebf446d966 100644

--- a/src/util/virdevmapper.c

+++ b/src/util/virdevmapper.c

@@ -23,40 +23,67 @@

 #include <config.h>

-#ifdef MAJOR_IN_MKDEV

-# include <sys/mkdev.h>

-#elif MAJOR_IN_SYSMACROS

+#include "virdevmapper.h"

+#include "internal.h"

+

+#ifdef __linux__

 # include <sys/sysmacros.h>

-#endif

+# include <linux/dm-ioctl.h>

+# include <sys/ioctl.h>

+# include <sys/types.h>

+# include <sys/stat.h>

+# include <fcntl.h>

-#ifdef WITH_DEVMAPPER

-# include <libdevmapper.h>

-#endif

+# include "virthread.h"

+# include "viralloc.h"

+# include "virstring.h"

+# include "virfile.h"

+

+# define VIR_FROM_THIS VIR_FROM_STORAGE

+

+# define PROC_DEVICES "/proc/devices"

+# define DM_NAME "device-mapper"

+# define DEV_DM_DIR "/dev/" DM_DIR

+# define CONTROL_PATH DEV_DM_DIR "/" DM_CONTROL_NODE

+# define BUF_SIZE (16 * 1024)

+

+verify(BUF_SIZE > sizeof(struct dm_ioctl));

+

+static unsigned int virDMMajor;

-#include "virdevmapper.h"

-#include "internal.h"

-#include "virthread.h"

-#include "viralloc.h"

-#include "virstring.h"

-

-#ifdef WITH_DEVMAPPER

-static void

-virDevMapperDummyLogger(int level ATTRIBUTE_UNUSED,

-                        const char *file ATTRIBUTE_UNUSED,

-                        int line ATTRIBUTE_UNUSED,

-                        int dm_errno ATTRIBUTE_UNUSED,

-                        const char *fmt ATTRIBUTE_UNUSED,

-                        ...)

-{

-    return;

-}

 static int

 virDevMapperOnceInit(void)

 {

-    /* Ideally, we would not need this. But libdevmapper prints

-     * error messages to stderr by default. Sad but true. */

-    dm_log_with_errno_init(virDevMapperDummyLogger);

+    VIR_AUTOFREE(char *) buf = NULL;

+    VIR_AUTOSTRINGLIST lines = NULL;

+    size_t i;

+

+    if (virFileReadAll(PROC_DEVICES, BUF_SIZE, &buf) < 0)

+        return -1;

+

+    lines = virStringSplit(buf, "\n", 0);

+    if (!lines)

+        return -1;

+

+    for (i = 0; lines[i]; i++) {

+        VIR_AUTOFREE(char *) dev = NULL;

+        unsigned int maj;

+

+        if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 &&

+            STREQ(dev, DM_NAME)) {

+            virDMMajor = maj;

+            break;

+        }

+    }

+

+    if (!lines[i]) {

+        virReportError(VIR_ERR_INTERNAL_ERROR,

+                       _("Unable to find major for %s"),

+                       DM_NAME);

+        return -1;

+    }

+

     return 0;

 }

@@ -64,95 +91,200 @@ virDevMapperOnceInit(void)

 VIR_ONCE_GLOBAL_INIT(virDevMapper)

+static void *

+virDMIoctl(int controlFD, int cmd, struct dm_ioctl *dm, char **buf)

+{

+    size_t bufsize = BUF_SIZE;

+

+ reread:

+    if (VIR_ALLOC_N(*buf, bufsize) < 0)

+        return NULL;

+

+    dm->version[0] = DM_VERSION_MAJOR;

+    dm->version[1] = 0;

+    dm->version[2] = 0;

+    dm->data_size = bufsize;

+    dm->data_start = sizeof(struct dm_ioctl);

+

+    memcpy(*buf, dm, sizeof(struct dm_ioctl));

+

+    if (ioctl(controlFD, cmd, *buf) < 0) {

+        VIR_FREE(*buf);

+        return NULL;

+    }

+

+    memcpy(dm, *buf, sizeof(struct dm_ioctl));

+

+    if (dm->flags & DM_BUFFER_FULL_FLAG) {

+        bufsize += BUF_SIZE;

+        VIR_FREE(*buf);

+        goto reread;

+    }

+

+    return *buf + dm->data_start;

+}

+

+

 static int

-virDevMapperGetTargetsImpl(const char *path,

-                           char ***devPaths_ret,

-                           unsigned int ttl)

+virDMOpen(void)

 {

-    struct dm_task *dmt = NULL;

-    struct dm_deps *deps;

-    struct dm_info info;

-    VIR_AUTOSTRINGLIST devPaths = NULL;

-    size_t i;

-    int ret = -1;

+    VIR_AUTOCLOSE controlFD = -1;

+    struct dm_ioctl dm;

+    VIR_AUTOFREE(char *) tmp = NULL;

+    int ret;

-    *devPaths_ret = NULL;

+    memset(&dm, 0, sizeof(dm));

-    if (virDevMapperInitialize() < 0)

-        return ret;

+    if ((controlFD = open(CONTROL_PATH, O_RDWR)) < 0)

+        return -1;

-    if (ttl == 0) {

-        errno = ELOOP;

+    if (!virDMIoctl(controlFD, DM_VERSION, &dm, &tmp)) {

+        virReportSystemError(errno, "%s",

+                             _("Unable to get device-mapper version"));

+        return -1;

+    }

+

+    if (dm.version[0] != DM_VERSION_MAJOR) {

+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,

+                       _("Unsupported device-mapper version. Expected %d got %d"),

+                       DM_VERSION_MAJOR, dm.version[0]);

+        return -1;

+    }

+

+    ret = controlFD;

+    controlFD = -1;

+    return ret;

+}

+

+

+static char *

+virDMSanitizepath(const char *path)

+{

+    VIR_AUTOFREE(char *) dmDirPath = NULL;

+    struct dirent *ent = NULL;

+    struct stat sb[2];

+    DIR *dh = NULL;

+    const char *p;

+    char *ret = NULL;

+    int rc;

+

+    /* If a path is NOT provided then assume it's DM name */

+    p = strrchr(path, '/');

+

+    if (!p) {

+        ignore_value(VIR_STRDUP(ret, path));

         return ret;

+    } else {

+        p++;

     }

-    if (!virIsDevMapperDevice(path))

-        return 0;

+    /* It's a path. Check if the last component is DM name */

+    if (stat(path, &sb[0]) < 0) {

+        virReportError(errno,

+                       _("Unable to stat %p"),

+                       path);

+        return NULL;

+    }

-    if (!(dmt = dm_task_create(DM_DEVICE_DEPS))) {

-        if (errno == ENOENT || errno == ENODEV) {

-            /* It's okay. Kernel is probably built without

-             * devmapper support. */

-            ret = 0;

-        }

+    if (virAsprintf(&dmDirPath, DEV_DM_DIR "/%s", p) < 0)

+        return NULL;

+

+    if (stat(dmDirPath, &sb[1]) == 0 &&

+        sb[0].st_rdev == sb[1].st_rdev) {

+        ignore_value(VIR_STRDUP(ret, p));

         return ret;

     }

-    if (!dm_task_set_name(dmt, path)) {

-        if (errno == ENOENT) {

-            /* It's okay, @path is not managed by devmapper =>

-             * not a devmapper device. */

-            ret = 0;

+    /* The last component of @path wasn't DM name. Let's check if

+     * there's a device under /dev/mapper/ with the same rdev. */

+    if (virDirOpen(&dh, DEV_DM_DIR) < 0)

+        return NULL;

+

+    while ((rc = virDirRead(dh, &ent, DEV_DM_DIR)) > 0) {

+        VIR_AUTOFREE(char *) tmp = NULL;

+

+        if (virAsprintf(&tmp, DEV_DM_DIR "/%s", ent->d_name) < 0)

+            return NULL;

+

+        if (stat(tmp, &sb[1]) == 0 &&

+            sb[0].st_rdev == sb[0].st_rdev) {

+            VIR_STEAL_PTR(ret, tmp);

+            break;

         }

-        goto cleanup;

     }

-    dm_task_no_open_count(dmt);

+    virDirClose(&dh);

+    return ret;

+}

-    if (!dm_task_run(dmt)) {

-        if (errno == ENXIO) {

-            /* If @path = "/dev/mapper/control" ENXIO is returned. */

-            ret = 0;

-        }

-        goto cleanup;

+

+static int

+virDevMapperGetTargetsImpl(int controlFD,

+                           const char *path,

+                           char ***devPaths_ret,

+                           unsigned int ttl)

+{

+    VIR_AUTOFREE(char *) sanitizedPath = NULL;

+    VIR_AUTOFREE(char *) buf = NULL;

+    struct dm_ioctl dm;

+    struct dm_target_deps *deps = NULL;

+    VIR_AUTOSTRINGLIST devPaths = NULL;

+    size_t i;

+

+    memset(&dm, 0, sizeof(dm));

+    *devPaths_ret = NULL;

+

+    if (ttl == 0) {

+        errno = ELOOP;

+        return -1;

     }

-    if (!dm_task_get_info(dmt, &info))

-        goto cleanup;

+    if (!virIsDevMapperDevice(path))

+        return 0;

+

+    if (!(sanitizedPath = virDMSanitizepath(path)))

+        return 0;

-    if (!info.exists) {

-        ret = 0;

-        goto cleanup;

+    if (virStrcpy(dm.name, sanitizedPath, DM_TABLE_DEPS) == NULL) {

+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",

+                       _("Resolved device mapper name too long"));

+        return -1;

     }

-    if (!(deps = dm_task_get_deps(dmt)))

-        goto cleanup;

+    deps = virDMIoctl(controlFD, DM_TABLE_DEPS, &dm, &buf;;

+    if (!deps) {

+        if (errno == ENXIO)

+            return 0;

+

+        virReportSystemError(errno,

+                             _("Unable to query dependencies for %s"),

+                             path);

+        return -1;

+    }

     if (VIR_ALLOC_N_QUIET(devPaths, deps->count + 1) < 0)

-        goto cleanup;

+        return -1;

     for (i = 0; i < deps->count; i++) {

         if (virAsprintfQuiet(&devPaths[i], "/dev/block/%u:%u",

-                             major(deps->device[i]),

-                             minor(deps->device[i])) < 0)

-            goto cleanup;

+                             major(deps->dev[i]),

+                             minor(deps->dev[i])) < 0) {

+            return -1;

+        }

     }

     for (i = 0; i < deps->count; i++) {

         VIR_AUTOSTRINGLIST tmpPaths = NULL;

-        if (virDevMapperGetTargetsImpl(devPaths[i], &tmpPaths, ttl - 1) < 0)

-            goto cleanup;

+        if (virDevMapperGetTargetsImpl(controlFD, devPaths[i], &tmpPaths, ttl - 1) < 0)

+            return -1;

         if (virStringListMerge(&devPaths, &tmpPaths) < 0)

-            goto cleanup;

+            return -1;

     }

     VIR_STEAL_PTR(*devPaths_ret, devPaths);

-    ret = 0;

- cleanup:

-    dm_task_destroy(dmt);

-    return ret;

+    return 0;

 }

@@ -171,9 +303,6 @@ virDevMapperGetTargetsImpl(const char *path,

  * If @path consists of yet another devmapper targets these are

  * consulted recursively.

  *

- * If we don't have permissions to talk to kernel, -1 is returned

- * and errno is set to EBADF.

- *

  * Returns 0 on success,

  *        -1 otherwise (with errno set, no libvirt error is

  *        reported)

@@ -182,13 +311,20 @@ int

 virDevMapperGetTargets(const char *path,

                        char ***devPaths)

 {

+    VIR_AUTOCLOSE controlFD = -1;

     const unsigned int ttl = 32;

     /* Arbitrary limit on recursion level. A devmapper target can

      * consist of devices or yet another targets. If that's the

      * case, we have to stop recursion somewhere. */

-    return virDevMapperGetTargetsImpl(path, devPaths, ttl);

+    if (virDevMapperInitialize() < 0)

+        return -1;

+

+    if ((controlFD = virDMOpen()) < 0)

+        return -1;

+

+    return virDevMapperGetTargetsImpl(controlFD, path, devPaths, ttl);

 }

@@ -197,15 +333,18 @@ virIsDevMapperDevice(const char *dev_name)

 {

     struct stat buf;

+    if (virDevMapperInitialize() < 0)

+        return false;

+

     if (!stat(dev_name, &buf) &&

         S_ISBLK(buf.st_mode) &&

-        dm_is_dm_major(major(buf.st_rdev)))

-            return true;

+        major(buf.st_rdev) == virDMMajor)

+        return true;

     return false;

 }

-#else /* ! WITH_DEVMAPPER */

+#else /* !defined(__linux__)  */

 int

 virDevMapperGetTargets(const char *path ATTRIBUTE_UNUSED,

@@ -221,4 +360,4 @@ virIsDevMapperDevice(const char *dev_name G_GNUC_UNUSED)

 {

     return false;

 }

-#endif /* ! WITH_DEVMAPPER */

+#endif /* ! defined(__linux__) */

-- 

2.31.0