File 22494556-CVE-2020-14339.patch of Package libvirt.19372

Overview Repositories Revisions Requests Users Attributes Meta

File 22494556-CVE-2020-14339.patch of Package libvirt.19372

commit 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e
Author: Michal Prívozník <mprivozn@redhat.com>
Date:   Thu Jul 23 16:02:00 2020 +0200

virdevmapper: Don't use libdevmapper to obtain dependencies
    
    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>

Index: libvirt-5.1.0/src/util/virdevmapper.c
===================================================================
--- libvirt-5.1.0.orig/src/util/virdevmapper.c
+++ libvirt-5.1.0/src/util/virdevmapper.c
@@ -20,116 +20,270 @@
 
 #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>
+
+# 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)
 
-#ifdef WITH_DEVMAPPER
-# include <libdevmapper.h>
-#endif
+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);
-    return 0;
+    char *buf = NULL;
+    VIR_AUTOSTRINGLIST lines = NULL;
+    size_t i;
+    int ret = -1;
+
+    if (virFileReadAll(PROC_DEVICES, BUF_SIZE, &buf) < 0)
+        return -1;
+
+    lines = virStringSplit(buf, "\n", 0);
+    if (!lines)
+        goto cleanup;
+
+    for (i = 0; lines[i]; i++) {
+        char *dev = NULL;
+        unsigned int maj;
+
+        if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 &&
+            STREQ(dev, DM_NAME)) {
+            virDMMajor = maj;
+            VIR_FREE(dev);
+            break;
+        }
+        VIR_FREE(dev);
+    }
+
+    if (!lines[i]) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unable to find major for %s"),
+                       DM_NAME);
+        goto cleanup;
+    }
+
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(buf);
+    return ret;
 }
 
 
 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
+virDMOpen(void)
+{
+    VIR_AUTOCLOSE controlFD = -1;
+    struct dm_ioctl dm;
+    char *tmp = NULL;
+    int ret = -1;
+
+    memset(&dm, 0, sizeof(dm));
+
+    if ((controlFD = open(CONTROL_PATH, O_RDWR)) < 0)
+        return -1;
+
+    if (!virDMIoctl(controlFD, DM_VERSION, &dm, &tmp)) {
+        virReportSystemError(errno, "%s",
+                             _("Unable to get device-mapper version"));
+        goto cleanup;
+    }
+
+    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]);
+        goto cleanup;
+    }
+
+    ret = controlFD;
+    controlFD = -1;
+
+ cleanup:
+    VIR_FREE(tmp);
+    return ret;
+}
+
+
+static char *
+virDMSanitizepath(const char *path)
+{
+    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) {
+        if (VIR_STRDUP(ret, path) < 0)
+            return NULL;
+
+        return ret;
+    } else {
+        p++;
+    }
+
+    /* 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 (virAsprintf(&dmDirPath, DEV_DM_DIR "/%s", p) < 0)
+        return NULL;
+
+    if (stat(dmDirPath, &sb[1]) == 0 &&
+        sb[0].st_rdev == sb[1].st_rdev) {
+        if (VIR_STRDUP(ret, p) < 0)
+            goto cleanup;
+
+        goto cleanup;
+    }
+
+    /* 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)
+        goto cleanup;
+
+    while ((rc = virDirRead(dh, &ent, DEV_DM_DIR)) > 0) {
+        char *tmp = NULL;
+
+        if (virAsprintf(&tmp, DEV_DM_DIR "/%s", ent->d_name) < 0)
+            goto cleanup;
+
+        if (stat(tmp, &sb[1]) == 0 &&
+            sb[0].st_rdev == sb[0].st_rdev) {
+            VIR_STEAL_PTR(ret, tmp);
+            break;
+        }
+        VIR_FREE(tmp);
+    }
+
+ cleanup:
+    VIR_FREE(dmDirPath);
+    virDirClose(&dh);
+    return ret;
+}
+
+
 static int
-virDevMapperGetTargetsImpl(const char *path,
+virDevMapperGetTargetsImpl(int controlFD,
+                           const char *path,
                            char ***devPaths_ret,
                            unsigned int ttl)
 {
-    struct dm_task *dmt = NULL;
-    struct dm_deps *deps;
-    struct dm_info info;
+    char *sanitizedPath = NULL;
+    char *buf = NULL;
+    struct dm_ioctl dm;
+    struct dm_target_deps *deps = NULL;
     char **devPaths = NULL;
     char **recursiveDevPaths = NULL;
     size_t i;
     int ret = -1;
 
+    memset(&dm, 0, sizeof(dm));
     *devPaths_ret = NULL;
 
-    if (virDevMapperInitialize() < 0)
-        return ret;
-
     if (ttl == 0) {
         errno = ELOOP;
-        return ret;
+        return -1;
     }
 
-    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;
-        }
-        return ret;
-    }
+    if (!(sanitizedPath = virDMSanitizepath(path)))
+        return 0;
 
-    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;
-        }
+    if (virStrncpy(dm.name, sanitizedPath, -1, DM_TABLE_DEPS) < 0) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("Resolved device mapper name too long"));
         goto cleanup;
     }
 
-    dm_task_no_open_count(dmt);
-
-    if (!dm_task_run(dmt)) {
+    deps = virDMIoctl(controlFD, DM_TABLE_DEPS, &dm, &buf);
+    if (!deps) {
         if (errno == ENXIO) {
-            /* If @path = "/dev/mapper/control" ENXIO is returned. */
             ret = 0;
+            goto cleanup;
         }
-        goto cleanup;
-    }
-
-    if (!dm_task_get_info(dmt, &info))
-        goto cleanup;
 
-    if (!info.exists) {
-        ret = 0;
+        virReportSystemError(errno,
+                             _("Unable to query dependencies for %s"),
+                             path);
         goto cleanup;
     }
 
-    if (!(deps = dm_task_get_deps(dmt)))
-        goto cleanup;
-
     if (VIR_ALLOC_N_QUIET(devPaths, deps->count + 1) < 0)
         goto cleanup;
 
     for (i = 0; i < deps->count; i++) {
         if (virAsprintfQuiet(&devPaths[i], "/dev/block/%u:%u",
-                             major(deps->device[i]),
-                             minor(deps->device[i])) < 0)
+                             major(deps->dev[i]),
+                             minor(deps->dev[i])) < 0)
             goto cleanup;
     }
 
@@ -137,7 +291,7 @@ virDevMapperGetTargetsImpl(const char *p
     for (i = 0; i < deps->count; i++) {
         char **tmpPaths;
 
-        if (virDevMapperGetTargetsImpl(devPaths[i], &tmpPaths, ttl - 1) < 0)
+        if (virDevMapperGetTargetsImpl(controlFD, devPaths[i], &tmpPaths, ttl - 1) < 0)
             goto cleanup;
 
         if (tmpPaths &&
@@ -155,7 +309,8 @@ virDevMapperGetTargetsImpl(const char *p
  cleanup:
     virStringListFree(recursiveDevPaths);
     virStringListFree(devPaths);
-    dm_task_destroy(dmt);
+    VIR_FREE(buf);
+    VIR_FREE(sanitizedPath);
     return ret;
 }
 
@@ -175,9 +330,6 @@ virDevMapperGetTargetsImpl(const char *p
  * 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)
@@ -186,16 +338,23 @@ 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);
 }
 
-#else /* ! WITH_DEVMAPPER */
+#else /* !defined(__linux__)  */
 
 int
 virDevMapperGetTargets(const char *path ATTRIBUTE_UNUSED,
@@ -204,4 +363,4 @@ virDevMapperGetTargets(const char *path
     errno = ENOSYS;
     return -1;
 }
-#endif /* ! WITH_DEVMAPPER */
+#endif /* ! defined(__linux__) */
Index: libvirt-5.1.0/po/POTFILES
===================================================================
--- libvirt-5.1.0.orig/po/POTFILES
+++ libvirt-5.1.0/po/POTFILES
@@ -204,6 +204,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

Places

File 22494556-CVE-2020-14339.patch of Package libvirt.19372

Places