[libvirt] [PATCH 0/8 v3] Avoid unsafe usage of /proc/$PID/root in LXC driver
An update of https://www.redhat.com/archives/libvir-list/2014-February/msg00401.html Eric had previously suggested that we could have our fork helper pass by a pre-opened file descriptor and then label that, so we can move the labelling code out of the subprocess. While I believe that is technically possible, it plays havoc with the security driver APIs. We'd basically have to create an entire new set of APIs that take an FD instead of path / object in virSecurityManager. I don't think that's a net win in terms of complexity added. Considering what we're running in the fork helper - We're Linux only so can rely on sane glibc malloc behaviour - We're already using security manager calls in the QEMU driver fork helper - We've protected the logging + error APIs in virFork So I think we'll get away with bending the POSIX rules on async safe functions in this instance. Changes in this version - Avoid use of virCgroupAllowDeviceByPath where we already have the major/minor number - Avoid overwriting errors set by virCgroupAllow* - Use return value check of < 0 instead of != 0 - Other misc typos Eric mentioned Daniel P. Berrange (7): Add virFileMakeParentPath helper function Add helper for running code in separate namespaces Avoid unsafe use of /proc/$PID/root in LXC disk hotplug Avoid unsafe use of /proc/$PID/root in LXC USB hotplug Avoid unsafe use of /proc/$PID/root in LXC block hostdev hotplug Avoid unsafe use of /proc/$PID/root in LXC chardev hostdev hotplug Avoid unsafe use of /proc/$PID/root in LXC hotunplug code Eric Blake (1): Avoid unsafe use of /proc/$PID/root in LXC shutdown/reboot code src/libvirt_private.syms | 2 + src/lxc/lxc_driver.c | 507 ++++++++++++++++++++++++----------------------- src/util/virfile.c | 27 +++ src/util/virfile.h | 1 + src/util/virinitctl.c | 26 +-- src/util/virinitctl.h | 5 +- src/util/virprocess.c | 107 ++++++++++ src/util/virprocess.h | 11 + 8 files changed, 418 insertions(+), 268 deletions(-) -- 1.8.5.3
Add a helper function which takes a file path and ensures that all directory components leading up to the file exist. IOW, it strips the filename part of the path and passes the result to virFileMakePath. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 27 +++++++++++++++++++++++++++ src/util/virfile.h | 1 + 3 files changed, 29 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 67d20ed..f22fe50 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1228,6 +1228,7 @@ virFileIsMountPoint; virFileLinkPointsTo; virFileLock; virFileLoopDeviceAssociate; +virFileMakeParentPath; virFileMakePath; virFileMakePathWithMode; virFileMatchesNameSuffix; diff --git a/src/util/virfile.c b/src/util/virfile.c index 631cd06..92a8621 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2340,6 +2340,33 @@ cleanup: return ret; } + +int +virFileMakeParentPath(const char *path) +{ + char *p; + char *tmp; + int ret; + + VIR_DEBUG("path=%s", path); + + if (VIR_STRDUP(tmp, path) < 0) { + errno = ENOMEM; + return -1; + } + + if ((p = strrchr(tmp, '/')) == NULL) { + errno = EINVAL; + return -1; + } + *p = '\0'; + + ret = virFileMakePathHelper(tmp, 0777); + VIR_FREE(tmp); + return ret; +} + + /* Build up a fully qualified path for a config file to be * associated with a persistent guest or network */ char * diff --git a/src/util/virfile.h b/src/util/virfile.h index 0d20cdb..20baf6f 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -198,6 +198,7 @@ int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK; int virFileMakePathWithMode(const char *path, mode_t mode) ATTRIBUTE_RETURN_CHECK; +int virFileMakeParentPath(const char *path) ATTRIBUTE_RETURN_CHECK; char *virFileBuildPath(const char *dir, const char *name, -- 1.8.5.3
On 02/17/2014 09:38 AM, Daniel P. Berrange wrote:
Add a helper function which takes a file path and ensures that all directory components leading up to the file exist. IOW, it strips the filename part of the path and passes the result to virFileMakePath.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 27 +++++++++++++++++++++++++++ src/util/virfile.h | 1 + 3 files changed, 29 insertions(+)
+int +virFileMakeParentPath(const char *path) +{ + char *p; + char *tmp; + int ret; + + VIR_DEBUG("path=%s", path); + + if (VIR_STRDUP(tmp, path) < 0) { + errno = ENOMEM; + return -1; + } + + if ((p = strrchr(tmp, '/')) == NULL) { + errno = EINVAL; + return -1;
Memleak of tmp. ACK with that fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Implement virProcessRunInMountNamespace, which runs callback of type virProcessNamespaceCallback in a container namespace. This uses a child process to run the callback, since you can't change the mount namespace of a thread. This implies that callbacks have to be careful about what code they run due to async safety rules. Idea by Dan Berrange, based on an initial report by Reco <recoverym4n@gmail.com> at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394 Signed-off-by: Daniel Berrange <berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 11 +++++ 3 files changed, 119 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f22fe50..eee1df4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1673,6 +1673,7 @@ virProcessGetNamespaces; virProcessGetStartTime; virProcessKill; virProcessKillPainfully; +virProcessRunInMountNamespace; virProcessSetAffinity; virProcessSetMaxFiles; virProcessSetMaxMemLock; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 83d0679..5c76674 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -50,6 +50,8 @@ #include "virlog.h" #include "virutil.h" #include "virstring.h" +#include "virthread.h" +#include "vircommand.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -877,3 +879,108 @@ int virProcessGetStartTime(pid_t pid, return 0; } #endif + + +#ifdef HAVE_SETNS +static int virProcessNamespaceHelper(int errfd, + pid_t pid, + virProcessNamespaceCallback cb, + void *opaque) +{ + char *path; + int fd = -1; + int ret = -1; + + if (virAsprintf(&path, "/proc/%llu/ns/mnt", (unsigned long long)pid) < 0) + goto cleanup; + + if ((fd = open(path, O_RDONLY)) < 0) { + virReportSystemError(errno, "%s", + _("Kernel does not provide mount namespace")); + goto cleanup; + } + + if (setns(fd, 0) < 0) { + virReportSystemError(errno, "%s", + _("Unable to enter mount namespace")); + goto cleanup; + } + + ret = cb(pid, opaque); + + cleanup: + if (ret < 0) { + virErrorPtr err = virGetLastError(); + if (err) { + size_t len = strlen(err->message) + 1; + ignore_value(safewrite(errfd, err->message, len)); + } + } + VIR_FREE(path); + VIR_FORCE_CLOSE(fd); + return ret; +} + +/* Run cb(opaque) in the mount namespace of pid. Return -1 with error + * message raised if we fail to run the child, if the child dies from + * a signal, or if the child has status 1; otherwise return the exit + * status of the child. The callback will be run in a child process + * so must be careful to only use async signal safe functions. + */ +int +virProcessRunInMountNamespace(pid_t pid, + virProcessNamespaceCallback cb, + void *opaque) +{ + int ret = -1; + pid_t child = -1; + int errfd[2] = { -1, -1 }; + + if (pipe(errfd) < 0) { + virReportSystemError(errno, "%s", + _("Cannot create pipe for child")); + return -1; + } + + ret = virFork(&child); + + if (ret < 0 || child < 0) { + if (child == 0) + _exit(1); + + /* parent */ + virProcessAbort(child); + goto cleanup; + } + + if (child == 0) { + VIR_FORCE_CLOSE(errfd[0]); + ret = virProcessNamespaceHelper(errfd[1], pid, + cb, opaque); + VIR_FORCE_CLOSE(errfd[1]); + _exit(ret < 0 ? 1 : 0); + } else { + char *buf = NULL; + VIR_FORCE_CLOSE(errfd[1]); + + ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf)); + ret = virProcessWait(child, NULL); + VIR_FREE(buf); + } + +cleanup: + VIR_FORCE_CLOSE(errfd[0]); + VIR_FORCE_CLOSE(errfd[1]); + return ret; +} +#else /* !HAVE_SETNS */ +int +virProcessRunInMountNamespace(pid_t pid ATTRIBUTE_UNUSED, + virProcessNamespaceCallback cb ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Mount namespaces are not available on this platform")); + return -1; +} +#endif diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 9f77bc5..5c173b0 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -60,4 +60,15 @@ int virProcessSetNamespaces(size_t nfdlist, int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes); int virProcessSetMaxProcesses(pid_t pid, unsigned int procs); int virProcessSetMaxFiles(pid_t pid, unsigned int files); + +/* Callback to run code within the mount namespace tied to the given + * pid. This function must use only async-signal-safe functions, as + * it gets run after a fork of a multi-threaded process. The return + * value of this function is passed to _exit(), except that a + * negative value is treated as an error. */ +typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque); + +int virProcessRunInMountNamespace(pid_t pid, + virProcessNamespaceCallback cb, + void *opaque); #endif /* __VIR_PROCESS_H__ */ -- 1.8.5.3
On 02/17/2014 09:38 AM, Daniel P. Berrange wrote:
Implement virProcessRunInMountNamespace, which runs callback of type virProcessNamespaceCallback in a container namespace. This uses a child process to run the callback, since you can't change the mount namespace of a thread. This implies that callbacks have to be careful about what code they run due to async safety rules.
Idea by Dan Berrange, based on an initial report by Reco <recoverym4n@gmail.com> at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Daniel Berrange <berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 11 +++++ 3 files changed, 119 insertions(+)
ACK.
+++ b/src/util/virprocess.c @@ -50,6 +50,8 @@ #include "virlog.h" #include "virutil.h" #include "virstring.h" +#include "virthread.h"
Do we still need virthread.h, or is that leftover from your experiments?
+ +/* Run cb(opaque) in the mount namespace of pid. Return -1 with error + * message raised if we fail to run the child, if the child dies from + * a signal, or if the child has status 1; otherwise return the exit + * status of the child.
Still not quite accurate - we now require that the child has exit status 0 to succeed. But I'll clean that up when rebasing my virFork cleanups. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Eric Blake <eblake@redhat.com> Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and lxcDomainReboot. Otherwise, a malicious guest could use symlinks to force the host to manipulate the wrong file in the host's namespace. Idea by Dan Berrange, based on an initial report by Reco <recoverym4n@gmail.com> at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394 Signed-off-by: Eric Blake <eblake@redhat.com> --- src/lxc/lxc_driver.c | 38 ++++++++++++++++++++------------------ src/util/virinitctl.c | 26 ++++++++++---------------- src/util/virinitctl.h | 5 ++--- 3 files changed, 32 insertions(+), 37 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f1f7032..4c3de06 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3337,12 +3337,20 @@ lxcConnectListAllDomains(virConnectPtr conn, static int +lxcDomainInitctlCallback(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + int *command = opaque; + return virInitctlSetRunLevel(*command); +} + + +static int lxcDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { virLXCDomainObjPrivatePtr priv; virDomainObjPtr vm; - char *vroot = NULL; int ret = -1; int rc; @@ -3369,16 +3377,14 @@ lxcDomainShutdownFlags(virDomainPtr dom, goto cleanup; } - if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - if (flags == 0 || (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) { - if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, - vroot)) < 0) { + int command = VIR_INITCTL_RUNLEVEL_POWEROFF; + + if ((rc = virProcessRunInMountNamespace(priv->initpid, + lxcDomainInitctlCallback, + &command)) < 0) goto cleanup; - } if (rc == 0 && flags != 0 && ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -3404,7 +3410,6 @@ lxcDomainShutdownFlags(virDomainPtr dom, ret = 0; cleanup: - VIR_FREE(vroot); if (vm) virObjectUnlock(vm); return ret; @@ -3416,13 +3421,13 @@ lxcDomainShutdown(virDomainPtr dom) return lxcDomainShutdownFlags(dom, 0); } + static int lxcDomainReboot(virDomainPtr dom, unsigned int flags) { virLXCDomainObjPrivatePtr priv; virDomainObjPtr vm; - char *vroot = NULL; int ret = -1; int rc; @@ -3449,16 +3454,14 @@ lxcDomainReboot(virDomainPtr dom, goto cleanup; } - if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - if (flags == 0 || (flags & VIR_DOMAIN_REBOOT_INITCTL)) { - if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, - vroot)) < 0) { + int command = VIR_INITCTL_RUNLEVEL_REBOOT; + + if ((rc = virProcessRunInMountNamespace(priv->initpid, + lxcDomainInitctlCallback, + &command)) < 0) goto cleanup; - } if (rc == 0 && flags != 0 && ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -3484,7 +3487,6 @@ lxcDomainReboot(virDomainPtr dom, ret = 0; cleanup: - VIR_FREE(vroot); if (vm) virObjectUnlock(vm); return ret; diff --git a/src/util/virinitctl.c b/src/util/virinitctl.c index a85cc9c..579268f 100644 --- a/src/util/virinitctl.c +++ b/src/util/virinitctl.c @@ -111,16 +111,18 @@ struct virInitctlRequest { # endif /* - * Send a message to init to change the runlevel + * Send a message to init to change the runlevel. This function is + * asynchronous-signal-safe (thus safe to use after fork of a + * multithreaded parent) - which is good, because it should only be + * used after forking and entering correct namespace. * * Returns 1 on success, 0 if initctl does not exist, -1 on error */ -int virInitctlSetRunLevel(virInitctlRunLevel level, - const char *vroot) +int +virInitctlSetRunLevel(virInitctlRunLevel level) { struct virInitctlRequest req; int fd = -1; - char *path = NULL; int ret = -1; memset(&req, 0, sizeof(req)); @@ -131,36 +133,28 @@ int virInitctlSetRunLevel(virInitctlRunLevel level, /* Yes it is an 'int' field, but wants a numeric character. Go figure */ req.runlevel = '0' + level; - if (vroot) { - if (virAsprintf(&path, "%s/%s", vroot, VIR_INITCTL_FIFO) < 0) - return -1; - } else { - if (VIR_STRDUP(path, VIR_INITCTL_FIFO) < 0) - return -1; - } - - if ((fd = open(path, O_WRONLY|O_NONBLOCK|O_CLOEXEC|O_NOCTTY)) < 0) { + if ((fd = open(VIR_INITCTL_FIFO, + O_WRONLY|O_NONBLOCK|O_CLOEXEC|O_NOCTTY)) < 0) { if (errno == ENOENT) { ret = 0; goto cleanup; } virReportSystemError(errno, _("Cannot open init control %s"), - path); + VIR_INITCTL_FIFO); goto cleanup; } if (safewrite(fd, &req, sizeof(req)) != sizeof(req)) { virReportSystemError(errno, _("Failed to send request to init control %s"), - path); + VIR_INITCTL_FIFO); goto cleanup; } ret = 1; cleanup: - VIR_FREE(path); VIR_FORCE_CLOSE(fd); return ret; } diff --git a/src/util/virinitctl.h b/src/util/virinitctl.h index 862539f..6aeb1a6 100644 --- a/src/util/virinitctl.h +++ b/src/util/virinitctl.h @@ -1,7 +1,7 @@ /* * virinitctl.h: API for talking to init systems via initctl * - * Copyright (C) 2012 Red Hat, Inc. + * Copyright (C) 2012-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -37,7 +37,6 @@ enum virInitctlRunLevel { VIR_INITCTL_RUNLEVEL_LAST }; -int virInitctlSetRunLevel(virInitctlRunLevel level, - const char *vroot); +int virInitctlSetRunLevel(virInitctlRunLevel level); #endif -- 1.8.5.3
On 02/17/2014 09:38 AM, Daniel P. Berrange wrote:
From: Eric Blake <eblake@redhat.com>
Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and lxcDomainReboot. Otherwise, a malicious guest could use symlinks to force the host to manipulate the wrong file in the host's namespace.
Idea by Dan Berrange, based on an initial report by Reco <recoverym4n@gmail.com> at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/lxc/lxc_driver.c | 38 ++++++++++++++++++++------------------ src/util/virinitctl.c | 26 ++++++++++---------------- src/util/virinitctl.h | 5 ++--- 3 files changed, 32 insertions(+), 37 deletions(-)
ACK (but I'm biased :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Rewrite lxcDomainAttachDeviceDiskLive function to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 185 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 141 insertions(+), 44 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 4c3de06..409f8b7 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3657,6 +3657,115 @@ cleanup: } +struct lxcDomainAttachDeviceMknodData { + virLXCDriverPtr driver; + mode_t mode; + dev_t dev; + virDomainObjPtr vm; + virDomainDeviceDefPtr def; + char *file; +}; + +static int +lxcDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + struct lxcDomainAttachDeviceMknodData *data = opaque; + int ret = -1; + + virSecurityManagerPostFork(data->driver->securityManager); + + if (virFileMakeParentPath(data->file) < 0) { + virReportSystemError(errno, + _("Unable to create %s"), data->file); + goto cleanup; + } + + /* Yes, the device name we're creating may not + * actually correspond to the major:minor number + * we're using, but we've no other option at this + * time. Just have to hope that containerized apps + * don't get upset that the major:minor is different + * to that normally implied by the device name + */ + VIR_DEBUG("Creating dev %s (%d,%d)", + data->file, major(data->dev), minor(data->dev)); + if (mknod(data->file, data->mode, data->dev) < 0) { + virReportSystemError(errno, + _("Unable to create device %s"), + data->file); + goto cleanup; + } + + if (lxcContainerChown(data->vm->def, data->file) < 0) + goto cleanup; + + /* Labelling normally operates on src, but we need + * to actually label the dst here, so hack the config */ + switch (data->def->type) { + case VIR_DOMAIN_DEVICE_DISK: { + virDomainDiskDefPtr def = data->def->data.disk; + char *tmpsrc = def->src; + def->src = data->file; + if (virSecurityManagerSetImageLabel(data->driver->securityManager, + data->vm->def, def) < 0) { + def->src = tmpsrc; + goto cleanup; + } + def->src = tmpsrc; + } break; + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected device type %d"), + data->def->type); + goto cleanup; + } + + ret = 0; + + cleanup: + if (ret < 0) + unlink(data->file); + return ret; +} + + +static int +lxcDomainAttachDeviceMknod(virLXCDriverPtr driver, + mode_t mode, + dev_t dev, + virDomainObjPtr vm, + virDomainDeviceDefPtr def, + char *file) +{ + virLXCDomainObjPrivatePtr priv = vm->privateData; + struct lxcDomainAttachDeviceMknodData data; + + memset(&data, 0, sizeof(data)); + + data.driver = driver; + data.mode = mode; + data.dev = dev; + data.vm = vm; + data.def = def; + data.file = file; + + if (virSecurityManagerPreFork(driver->securityManager) < 0) + return -1; + + if (virProcessRunInMountNamespace(priv->initpid, + lxcDomainAttachDeviceMknodHelper, + &data) < 0) { + virSecurityManagerPostFork(driver->securityManager); + return -1; + } + + virSecurityManagerPostFork(driver->securityManager); + return 0; +} + + static int lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, virDomainObjPtr vm, @@ -3665,11 +3774,9 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, virLXCDomainObjPrivatePtr priv = vm->privateData; virDomainDiskDefPtr def = dev->data.disk; int ret = -1; - char *dst = NULL; struct stat sb; - bool created = false; - mode_t mode = 0; - char *tmpsrc = def->src; + char *file = NULL; + int perms; if (!priv->initpid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -3713,51 +3820,44 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, goto cleanup; } - if (virAsprintf(&dst, "/proc/%llu/root/dev/%s", - (unsigned long long)priv->initpid, def->dst) < 0) - goto cleanup; - - if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("devices cgroup isn't mounted")); goto cleanup; + } - mode = 0700 | S_IFBLK; + perms = (def->readonly ? + VIR_CGROUP_DEVICE_READ : + VIR_CGROUP_DEVICE_RW) | + VIR_CGROUP_DEVICE_MKNOD; - /* Yes, the device name we're creating may not - * actually correspond to the major:minor number - * we're using, but we've no other option at this - * time. Just have to hope that containerized apps - * don't get upset that the major:minor is different - * to that normally implied by the device name - */ - VIR_DEBUG("Creating dev %s (%d,%d) from %s", - dst, major(sb.st_rdev), minor(sb.st_rdev), def->src); - if (mknod(dst, mode, sb.st_rdev) < 0) { - virReportSystemError(errno, - _("Unable to create device %s"), - dst); + if (virCgroupAllowDevice(priv->cgroup, + 'b', + major(sb.st_rdev), + minor(sb.st_rdev), + perms) < 0) goto cleanup; - } - if (lxcContainerChown(vm->def, dst) < 0) + if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0) goto cleanup; - created = true; - - /* Labelling normally operates on src, but we need - * to actually label the dst here, so hack the config */ - def->src = dst; - if (virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, def) < 0) + if (virAsprintf(&file, + "/dev/%s", def->dst) < 0) goto cleanup; - if (virCgroupAllowDevicePath(priv->cgroup, def->src, - (def->readonly ? - VIR_CGROUP_DEVICE_READ : - VIR_CGROUP_DEVICE_RW) | - VIR_CGROUP_DEVICE_MKNOD) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot allow device %s for domain %s"), - def->src, vm->def->name); + if (lxcDomainAttachDeviceMknod(driver, + 0700 | S_IFBLK, + sb.st_rdev, + vm, + dev, + file) < 0) { + if (virCgroupDenyDevice(priv->cgroup, + 'b', + major(sb.st_rdev), + minor(sb.st_rdev), + perms) < 0) + VIR_WARN("cannot deny device %s for domain %s", + def->src, vm->def->name); goto cleanup; } @@ -3766,11 +3866,8 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, ret = 0; cleanup: - def->src = tmpsrc; virDomainAuditDisk(vm, NULL, def->src, "attach", ret == 0); - if (dst && created && ret < 0) - unlink(dst); - VIR_FREE(dst); + VIR_FREE(file); return ret; } -- 1.8.5.3
On 02/17/2014 09:38 AM, Daniel P. Berrange wrote:
Rewrite lxcDomainAttachDeviceDiskLive function to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 185 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 141 insertions(+), 44 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Rewrite lxcDomainAttachDeviceHostdevSubsysUSBLive function to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 73 ++++++++++++++++------------------------------------ 1 file changed, 22 insertions(+), 51 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 409f8b7..30aa31f 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3715,6 +3715,13 @@ lxcDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, def->src = tmpsrc; } break; + case VIR_DOMAIN_DEVICE_HOSTDEV: { + virDomainHostdevDefPtr def = data->def->data.hostdev; + if (virSecurityManagerSetHostdevLabel(data->driver->securityManager, + data->vm->def, def, NULL) < 0) + goto cleanup; + } break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected device type %d"), @@ -4009,13 +4016,8 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver, virLXCDomainObjPrivatePtr priv = vm->privateData; virDomainHostdevDefPtr def = dev->data.hostdev; int ret = -1; - char *vroot = NULL; char *src = NULL; - char *dstdir = NULL; - char *dstfile = NULL; struct stat sb; - mode_t mode; - bool created = false; virUSBDevicePtr usb = NULL; if (virDomainHostdevFind(vm->def, def, NULL) >= 0) { @@ -4024,27 +4026,13 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver, return -1; } - if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - - if (virAsprintf(&dstdir, "%s/dev/bus/usb/%03d", - vroot, - def->source.subsys.u.usb.bus) < 0) - goto cleanup; - - if (virAsprintf(&dstfile, "%s/%03d", - dstdir, - def->source.subsys.u.usb.device) < 0) - goto cleanup; - if (virAsprintf(&src, "/dev/bus/usb/%03d/%03d", def->source.subsys.u.usb.bus, def->source.subsys.u.usb.device) < 0) goto cleanup; if (!(usb = virUSBDeviceNew(def->source.subsys.u.usb.bus, - def->source.subsys.u.usb.device, vroot))) + def->source.subsys.u.usb.device, NULL))) goto cleanup; if (stat(src, &sb) < 0) { @@ -4060,53 +4048,36 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver, goto cleanup; } - mode = 0700 | S_IFCHR; - if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) goto cleanup; - if (virFileMakePath(dstdir) < 0) { - virReportSystemError(errno, - _("Unable to create %s"), dstdir); - goto cleanup; - } - - VIR_DEBUG("Creating dev %s (%d,%d)", - dstfile, major(sb.st_rdev), minor(sb.st_rdev)); - if (mknod(dstfile, mode, sb.st_rdev) < 0) { - virReportSystemError(errno, - _("Unable to create device %s"), - dstfile); - goto cleanup; - } - created = true; - - if (lxcContainerChown(vm->def, dstfile) < 0) - goto cleanup; - - if (virSecurityManagerSetHostdevLabel(driver->securityManager, - vm->def, def, vroot) < 0) - goto cleanup; - if (virUSBDeviceFileIterate(usb, virLXCSetupHostUsbDeviceCgroup, priv->cgroup) < 0) goto cleanup; + if (lxcDomainAttachDeviceMknod(driver, + 0700 | S_IFCHR, + sb.st_rdev, + vm, + dev, + src) < 0) { + if (virUSBDeviceFileIterate(usb, + virLXCTeardownHostUsbDeviceCgroup, + priv->cgroup) < 0) + VIR_WARN("cannot deny device %s for domain %s", + src, vm->def->name); + goto cleanup; + } + vm->def->hostdevs[vm->def->nhostdevs++] = def; ret = 0; cleanup: virDomainAuditHostdev(vm, def, "attach", ret == 0); - if (ret < 0 && created) - unlink(dstfile); - virUSBDeviceFree(usb); VIR_FREE(src); - VIR_FREE(dstfile); - VIR_FREE(dstdir); - VIR_FREE(vroot); return ret; } -- 1.8.5.3
On 02/17/2014 09:39 AM, Daniel P. Berrange wrote:
Rewrite lxcDomainAttachDeviceHostdevSubsysUSBLive function to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 73 ++++++++++++++++------------------------------------ 1 file changed, 22 insertions(+), 51 deletions(-)
if (!(usb = virUSBDeviceNew(def->source.subsys.u.usb.bus, - def->source.subsys.u.usb.device, vroot))) + def->source.subsys.u.usb.device, NULL)))
It looks like all callers of virUSBDeviceNew pass NULL once this series is over. Can we clean things up in a followup to kill the dead parameter? But this patch is fine as-is. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Rewrite lxcDomainAttachDeviceHostdevStorageLive function to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 66 ++++++++++++++-------------------------------------- 1 file changed, 18 insertions(+), 48 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 30aa31f..4740473 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4090,11 +4090,7 @@ lxcDomainAttachDeviceHostdevStorageLive(virLXCDriverPtr driver, virLXCDomainObjPrivatePtr priv = vm->privateData; virDomainHostdevDefPtr def = dev->data.hostdev; int ret = -1; - char *dst = NULL; - char *vroot = NULL; struct stat sb; - bool created = false; - mode_t mode = 0; if (!def->source.caps.u.storage.block) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -4122,51 +4118,29 @@ lxcDomainAttachDeviceHostdevStorageLive(virLXCDriverPtr driver, goto cleanup; } - if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - - if (virAsprintf(&dst, "%s/%s", - vroot, - def->source.caps.u.storage.block) < 0) - goto cleanup; - if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) goto cleanup; - if (lxcContainerSetupHostdevCapsMakePath(dst) < 0) { - virReportSystemError(errno, - _("Unable to create directory for device %s"), - dst); - goto cleanup; - } - - mode = 0700 | S_IFBLK; - - VIR_DEBUG("Creating dev %s (%d,%d)", - def->source.caps.u.storage.block, - major(sb.st_rdev), minor(sb.st_rdev)); - if (mknod(dst, mode, sb.st_rdev) < 0) { - virReportSystemError(errno, - _("Unable to create device %s"), - dst); - goto cleanup; - } - created = true; - - if (lxcContainerChown(vm->def, dst) < 0) - goto cleanup; - - if (virSecurityManagerSetHostdevLabel(driver->securityManager, - vm->def, def, vroot) < 0) + if (virCgroupAllowDevice(priv->cgroup, + 'b', + major(sb.st_rdev), + minor(sb.st_rdev), + VIR_CGROUP_DEVICE_RWM) < 0) goto cleanup; - if (virCgroupAllowDevicePath(priv->cgroup, def->source.caps.u.storage.block, - VIR_CGROUP_DEVICE_RW | - VIR_CGROUP_DEVICE_MKNOD) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot allow device %s for domain %s"), - def->source.caps.u.storage.block, vm->def->name); + if (lxcDomainAttachDeviceMknod(driver, + 0700 | S_IFBLK, + sb.st_rdev, + vm, + dev, + def->source.caps.u.storage.block) < 0) { + if (virCgroupDenyDevice(priv->cgroup, + 'b', + major(sb.st_rdev), + minor(sb.st_rdev), + VIR_CGROUP_DEVICE_RWM) < 0) + VIR_WARN("cannot deny device %s for domain %s", + def->source.caps.u.storage.block, vm->def->name); goto cleanup; } @@ -4176,10 +4150,6 @@ lxcDomainAttachDeviceHostdevStorageLive(virLXCDriverPtr driver, cleanup: virDomainAuditHostdev(vm, def, "attach", ret == 0); - if (dst && created && ret < 0) - unlink(dst); - VIR_FREE(dst); - VIR_FREE(vroot); return ret; } -- 1.8.5.3
On 02/17/2014 09:39 AM, Daniel P. Berrange wrote:
Rewrite lxcDomainAttachDeviceHostdevStorageLive function to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 66 ++++++++++++++-------------------------------------- 1 file changed, 18 insertions(+), 48 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Rewrite lxcDomainAttachDeviceHostdevMisceLive function to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 66 ++++++++++++++-------------------------------------- 1 file changed, 18 insertions(+), 48 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 4740473..987196b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4162,11 +4162,7 @@ lxcDomainAttachDeviceHostdevMiscLive(virLXCDriverPtr driver, virLXCDomainObjPrivatePtr priv = vm->privateData; virDomainHostdevDefPtr def = dev->data.hostdev; int ret = -1; - char *dst = NULL; - char *vroot = NULL; struct stat sb; - bool created = false; - mode_t mode = 0; if (!def->source.caps.u.misc.chardev) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -4194,51 +4190,29 @@ lxcDomainAttachDeviceHostdevMiscLive(virLXCDriverPtr driver, goto cleanup; } - if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - - if (virAsprintf(&dst, "%s/%s", - vroot, - def->source.caps.u.misc.chardev) < 0) + if (virCgroupAllowDevice(priv->cgroup, + 'c', + major(sb.st_rdev), + minor(sb.st_rdev), + VIR_CGROUP_DEVICE_RWM) < 0) goto cleanup; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) goto cleanup; - if (lxcContainerSetupHostdevCapsMakePath(dst) < 0) { - virReportSystemError(errno, - _("Unable to create directory for device %s"), - dst); - goto cleanup; - } - - mode = 0700 | S_IFCHR; - - VIR_DEBUG("Creating dev %s (%d,%d)", - def->source.caps.u.misc.chardev, - major(sb.st_rdev), minor(sb.st_rdev)); - if (mknod(dst, mode, sb.st_rdev) < 0) { - virReportSystemError(errno, - _("Unable to create device %s"), - dst); - goto cleanup; - } - created = true; - - if (lxcContainerChown(vm->def, dst) < 0) - goto cleanup; - - if (virSecurityManagerSetHostdevLabel(driver->securityManager, - vm->def, def, vroot) < 0) - goto cleanup; - - if (virCgroupAllowDevicePath(priv->cgroup, def->source.caps.u.misc.chardev, - VIR_CGROUP_DEVICE_RW | - VIR_CGROUP_DEVICE_MKNOD) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot allow device %s for domain %s"), - def->source.caps.u.misc.chardev, vm->def->name); + if (lxcDomainAttachDeviceMknod(driver, + 0700 | S_IFBLK, + sb.st_rdev, + vm, + dev, + def->source.caps.u.misc.chardev) < 0) { + if (virCgroupDenyDevice(priv->cgroup, + 'c', + major(sb.st_rdev), + minor(sb.st_rdev), + VIR_CGROUP_DEVICE_RWM) < 0) + VIR_WARN("cannot deny device %s for domain %s", + def->source.caps.u.storage.block, vm->def->name); goto cleanup; } @@ -4248,10 +4222,6 @@ lxcDomainAttachDeviceHostdevMiscLive(virLXCDriverPtr driver, cleanup: virDomainAuditHostdev(vm, def, "attach", ret == 0); - if (dst && created && ret < 0) - unlink(dst); - VIR_FREE(dst); - VIR_FREE(vroot); return ret; } -- 1.8.5.3
On 02/17/2014 09:39 AM, Daniel P. Berrange wrote:
Rewrite lxcDomainAttachDeviceHostdevMisceLive function
s/Misce/Misc/
to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 66 ++++++++++++++-------------------------------------- 1 file changed, 18 insertions(+), 48 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Rewrite multiple hotunplug functions to to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute symlink, tricking the driver into changing the host OS filesystem. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 79 ++++++++++++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 40 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 987196b..53db957 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3774,6 +3774,39 @@ lxcDomainAttachDeviceMknod(virLXCDriverPtr driver, static int +lxcDomainAttachDeviceUnlinkHelper(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + const char *path = opaque; + + VIR_DEBUG("Unlinking %s", path); + if (unlink(path) < 0 && errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove device %s"), path); + return -1; + } + + return 0; +} + + +static int +lxcDomainAttachDeviceUnlink(virDomainObjPtr vm, + char *file) +{ + virLXCDomainObjPrivatePtr priv = vm->privateData; + + if (virProcessRunInMountNamespace(priv->initpid, + lxcDomainAttachDeviceUnlinkHelper, + file) < 0) { + return -1; + } + + return 0; +} + + +static int lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) @@ -4364,8 +4397,7 @@ lxcDomainDetachDeviceDiskLive(virDomainObjPtr vm, def = vm->def->disks[idx]; - if (virAsprintf(&dst, "/proc/%llu/root/dev/%s", - (unsigned long long)priv->initpid, def->dst) < 0) + if (virAsprintf(&dst, "/dev/%s", def->dst) < 0) goto cleanup; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { @@ -4374,11 +4406,8 @@ lxcDomainDetachDeviceDiskLive(virDomainObjPtr vm, goto cleanup; } - VIR_DEBUG("Unlinking %s (backed by %s)", dst, def->src); - if (unlink(dst) < 0 && errno != ENOENT) { + if (lxcDomainAttachDeviceUnlink(vm, dst) < 0) { virDomainAuditDisk(vm, def->src, NULL, "detach", false); - virReportSystemError(errno, - _("Unable to remove device %s"), dst); goto cleanup; } virDomainAuditDisk(vm, def->src, NULL, "detach", true); @@ -4473,7 +4502,6 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver, virDomainHostdevDefPtr def = NULL; int idx, ret = -1; char *dst = NULL; - char *vroot = NULL; virUSBDevicePtr usb = NULL; if ((idx = virDomainHostdevFind(vm->def, @@ -4484,12 +4512,7 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver, goto cleanup; } - if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - - if (virAsprintf(&dst, "%s/dev/bus/usb/%03d/%03d", - vroot, + if (virAsprintf(&dst, "/dev/bus/usb/%03d/%03d", def->source.subsys.u.usb.bus, def->source.subsys.u.usb.device) < 0) goto cleanup; @@ -4504,11 +4527,8 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver, def->source.subsys.u.usb.device, NULL))) goto cleanup; - VIR_DEBUG("Unlinking %s", dst); - if (unlink(dst) < 0 && errno != ENOENT) { + if (lxcDomainAttachDeviceUnlink(vm, dst) < 0) { virDomainAuditHostdev(vm, def, "detach", false); - virReportSystemError(errno, - _("Unable to remove device %s"), dst); goto cleanup; } virDomainAuditHostdev(vm, def, "detach", true); @@ -4531,7 +4551,6 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver, cleanup: virUSBDeviceFree(usb); VIR_FREE(dst); - VIR_FREE(vroot); return ret; } @@ -4543,7 +4562,6 @@ lxcDomainDetachDeviceHostdevStorageLive(virDomainObjPtr vm, virLXCDomainObjPrivatePtr priv = vm->privateData; virDomainHostdevDefPtr def = NULL; int idx, ret = -1; - char *dst = NULL; if (!priv->initpid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4560,22 +4578,14 @@ lxcDomainDetachDeviceHostdevStorageLive(virDomainObjPtr vm, goto cleanup; } - if (virAsprintf(&dst, "/proc/%llu/root/%s", - (unsigned long long)priv->initpid, - def->source.caps.u.storage.block) < 0) - goto cleanup; - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("devices cgroup isn't mounted")); goto cleanup; } - VIR_DEBUG("Unlinking %s", dst); - if (unlink(dst) < 0 && errno != ENOENT) { + if (lxcDomainAttachDeviceUnlink(vm, def->source.caps.u.storage.block) < 0) { virDomainAuditHostdev(vm, def, "detach", false); - virReportSystemError(errno, - _("Unable to remove device %s"), dst); goto cleanup; } virDomainAuditHostdev(vm, def, "detach", true); @@ -4590,7 +4600,6 @@ lxcDomainDetachDeviceHostdevStorageLive(virDomainObjPtr vm, ret = 0; cleanup: - VIR_FREE(dst); return ret; } @@ -4602,7 +4611,6 @@ lxcDomainDetachDeviceHostdevMiscLive(virDomainObjPtr vm, virLXCDomainObjPrivatePtr priv = vm->privateData; virDomainHostdevDefPtr def = NULL; int idx, ret = -1; - char *dst = NULL; if (!priv->initpid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4619,22 +4627,14 @@ lxcDomainDetachDeviceHostdevMiscLive(virDomainObjPtr vm, goto cleanup; } - if (virAsprintf(&dst, "/proc/%llu/root/%s", - (unsigned long long)priv->initpid, - def->source.caps.u.misc.chardev) < 0) - goto cleanup; - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("devices cgroup isn't mounted")); goto cleanup; } - VIR_DEBUG("Unlinking %s", dst); - if (unlink(dst) < 0 && errno != ENOENT) { + if (lxcDomainAttachDeviceUnlink(vm, def->source.caps.u.misc.chardev) < 0) { virDomainAuditHostdev(vm, def, "detach", false); - virReportSystemError(errno, - _("Unable to remove device %s"), dst); goto cleanup; } virDomainAuditHostdev(vm, def, "detach", true); @@ -4649,7 +4649,6 @@ lxcDomainDetachDeviceHostdevMiscLive(virDomainObjPtr vm, ret = 0; cleanup: - VIR_FREE(dst); return ret; } -- 1.8.5.3
On 02/17/2014 09:39 AM, Daniel P. Berrange wrote:
Rewrite multiple hotunplug functions to to use the virProcessRunInMountNamespace helper. This avoids risk of a malicious guest replacing /dev with a absolute
s/a absolute/an absolute/
symlink, tricking the driver into changing the host OS filesystem.
Worth mentioning the CVE number in any of these commits? Are you planning on backporting to stable branches, or could you use some help on that front?
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 79 ++++++++++++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 40 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake