9:32 a.m.
This is a followup to Eric's original proposal
https://www.redhat.com/archives/libvir-list/2013-December/msg01242.html
The first 5 patches fix non-security bugs in the LXC hotplug
code. Then there's a couple of helper patches. Finally the
last 6 fix the actual security issue previously publically
reported.
Eric originally had a huge cleanup of virFork, but I'd
prefer that be done afterwards, to minimize the backporting
pain for stable branches.
Daniel P. Berrange (13):
Don't block use of USB with containers
Fix path used for USB device attach with LXC
Record hotplugged USB device in LXC live guest config
Fix reset of cgroup when detaching USB device from LXC guests
Disks are always block devices, never character devices
Move check for cgroup devices ACL upfront in LXC hotplug
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/conf/domain_conf.c | 1 +
src/libvirt_private.syms | 2 +
src/lxc/lxc_driver.c | 552 +++++++++++++++++++++++------------------------
src/util/virfile.c | 27 +++
src/util/virfile.h | 1 +
src/util/virinitctl.c | 26 +--
src/util/virinitctl.h | 5 +-
src/util/virprocess.c | 114 ++++++++++
src/util/virprocess.h | 11 +
9 files changed, 442 insertions(+), 297 deletions(-)
--
1.8.5.3
9:32 a.m.
New subject: [libvirt] [PATCH 01/14] Don't block use of USB with containers
virDomainDefCompatibleDevice blocks use of USB if no USB
controller is present. This is not correct for containers
since devices can be assigned directly regardless of any
controllers.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/domain_conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 512fe51..61e69e1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17582,6 +17582,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev)
{
if (!virDomainDefHasUSB(def) &&
+ STRNEQ(def->os.type, "exe") &&
virDomainDeviceIsUSB(dev)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Device configuration is not compatible: "
--
1.8.5.3
9:50 a.m.
New subject: [libvirt] [PATCH 01/14] Don't block use of USB with containers
On 02/07/2014 08:32 AM, Daniel P. Berrange wrote:
virDomainDefCompatibleDevice blocks use of USB if no USB
controller is present. This is not correct for containers
since devices can be assigned directly regardless of any
controllers.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/domain_conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 512fe51..61e69e1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17582,6 +17582,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev)
{
if (!virDomainDefHasUSB(def) &&
+ STRNEQ(def->os.type, "exe") &&
Elsewhere we alternate between STREQ_NULLABLE
(virDomainDeviceInfoIterateInternal) and mandating os.type
(virDomainDefPostParseInternal). Which is right? Can os.type validly
be NULL?
Depending on the answer, you may need STREQ_NULLABLE here, or a followup
patches to the other places; but with that fixed, ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:53 a.m.
New subject: [libvirt] [PATCH 01/14] Don't block use of USB with containers
On Fri, Feb 07, 2014 at 08:50:19AM -0700, Eric Blake wrote:
On 02/07/2014 08:32 AM, Daniel P. Berrange wrote:
> virDomainDefCompatibleDevice blocks use of USB if no USB
> controller is present. This is not correct for containers
> since devices can be assigned directly regardless of any
> controllers.
>
> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
> ---
> src/conf/domain_conf.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 512fe51..61e69e1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17582,6 +17582,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
> virDomainDeviceDefPtr dev)
> {
> if (!virDomainDefHasUSB(def) &&
> + STRNEQ(def->os.type, "exe") &&
Elsewhere we alternate between STREQ_NULLABLE
(virDomainDeviceInfoIterateInternal) and mandating os.type
(virDomainDefPostParseInternal). Which is right? Can os.type validly
be NULL?
Depending on the answer, you may need STREQ_NULLABLE here, or a followup
patches to the other places; but with that fixed, ACK.
In virDomainDefPostParseInternal we have:
if (!def->os.type) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("hypervisor type must be specified"));
return -1;
}
So I believe we're fine here.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:33 a.m.
New subject: [libvirt] [PATCH 02/14] Fix path used for USB device attach with LXC
The LXC code missed the 'usb' component out of the path
/dev/bus/usb/$BUSNUM/$DEVNUM, so it failed to actually
setup cgroups for the device. This was in fact lucky
because the call to virLXCSetupHostUsbDeviceCgroup
was also mistakenly passing '&priv->cgroup' instead of
just 'priv->cgroup'. So once the path is fixed, libvirtd
would then crash trying to access the bogus virCgroupPtr
pointer. This would have been a security issue, were it
not for the bogus path preventing the pointer reference
being reached.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 687046e..cd48be8 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3903,7 +3903,7 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver,
(unsigned long long)priv->initpid) < 0)
goto cleanup;
- if (virAsprintf(&dstdir, "%s/dev/bus/%03d",
+ if (virAsprintf(&dstdir, "%s/dev/bus/usb/%03d",
vroot,
def->source.subsys.u.usb.bus) < 0)
goto cleanup;
@@ -3968,7 +3968,7 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver,
if (virUSBDeviceFileIterate(usb,
virLXCSetupHostUsbDeviceCgroup,
- &priv->cgroup) < 0)
+ priv->cgroup) < 0)
goto cleanup;
ret = 0;
--
1.8.5.3
9:58 a.m.
New subject: [libvirt] [PATCH 02/14] Fix path used for USB device attach with LXC
On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
The LXC code missed the 'usb' component out of the path
/dev/bus/usb/$BUSNUM/$DEVNUM, so it failed to actually
setup cgroups for the device. This was in fact lucky
because the call to virLXCSetupHostUsbDeviceCgroup
was also mistakenly passing '&priv->cgroup' instead of
just 'priv->cgroup'. So once the path is fixed, libvirtd
would then crash trying to access the bogus virCgroupPtr
pointer. This would have been a security issue, were it
not for the bogus path preventing the pointer reference
being reached.
One bug masking another. Eww.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 03/14] Record hotplugged USB device in LXC live guest config
After hotplugging a USB device, the LXC driver forgot
to add the device def to the virDomainDefPtr.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index cd48be8..1c7abb9 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3943,6 +3943,9 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver,
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);
@@ -3971,6 +3974,8 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver,
priv->cgroup) < 0)
goto cleanup;
+ vm->def->hostdevs[vm->def->nhostdevs++] = def;
+
ret = 0;
cleanup:
--
1.8.5.3
10:24 a.m.
New subject: [libvirt] [PATCH 03/14] Record hotplugged USB device in LXC live guest config
On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
After hotplugging a USB device, the LXC driver forgot
to add the device def to the virDomainDefPtr.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 5 +++++
1 file changed, 5 insertions(+)
ACK.
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index cd48be8..1c7abb9 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3943,6 +3943,9 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver,
mode = 0700 | S_IFCHR;
+ if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0)
Space around +
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 04/14] Fix reset of cgroup when detaching USB device from LXC guests
When detaching a USB device from an LXC guest we must remove
the device from the cgroup ACL. Unfortunately we were telling
the cgroup code to use the guest /dev path, not the host /dev
path, and the guest device node had already been unlinked.
This was, however, fortunate since the code passed &priv->cgroup
instead of priv->cgroup, so would have crash if the device node
were accessible.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 1c7abb9..b9da8ef 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4477,7 +4477,7 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver,
}
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;
VIR_DEBUG("Unlinking %s", dst);
@@ -4491,7 +4491,7 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver,
if (virUSBDeviceFileIterate(usb,
virLXCTeardownHostUsbDeviceCgroup,
- &priv->cgroup) < 0)
+ priv->cgroup) < 0)
VIR_WARN("cannot deny device %s for domain %s",
dst, vm->def->name);
--
1.8.5.3
10:30 a.m.
New subject: [libvirt] [PATCH 04/14] Fix reset of cgroup when detaching USB device from LXC guests
On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
When detaching a USB device from an LXC guest we must remove
the device from the cgroup ACL. Unfortunately we were telling
the cgroup code to use the guest /dev path, not the host /dev
path, and the guest device node had already been unlinked.
This was, however, fortunate since the code passed &priv->cgroup
instead of priv->cgroup, so would have crash if the device node
were accessible.
Another case of one bug masking another. Wow.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 05/14] Disks are always block devices, never character devices
The LXC disk hotplug code was allowing block or character devices
to be given as disk. A disk is always a block device.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index b9da8ef..79a9e36 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3668,9 +3668,9 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
goto cleanup;
}
- if (!S_ISCHR(sb.st_mode) && !S_ISBLK(sb.st_mode)) {
+ if (!S_ISBLK(sb.st_mode)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Disk source %s must be a character/block device"),
+ _("Disk source %s must be a block device"),
def->src);
goto cleanup;
}
@@ -3682,11 +3682,7 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
goto cleanup;
- mode = 0700;
- if (S_ISCHR(sb.st_mode))
- mode |= S_IFCHR;
- else
- mode |= S_IFBLK;
+ mode = 0700 | S_IFBLK;
/* Yes, the device name we're creating may not
* actually correspond to the major:minor number
--
1.8.5.3
10:35 a.m.
New subject: [libvirt] [PATCH 05/14] Disks are always block devices, never character devices
On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
The LXC disk hotplug code was allowing block or character devices
to be given as disk. A disk is always a block device.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 06/14] Move check for cgroup devices ACL upfront in LXC hotplug
The check for whether the cgroup devices ACL is available is
done quite late during LXC hotplug - in fact after the device
node is already created in the container in some cases. Better
todo it upfront so we fail immediately.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 36 ++++++++++++------------------------
1 file changed, 12 insertions(+), 24 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 79a9e36..51ca98c 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3645,6 +3645,12 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
goto cleanup;
}
+ if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("devices cgroup isn't mounted"));
+ goto cleanup;
+ }
+
if (def->type != VIR_DOMAIN_DISK_TYPE_BLOCK) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Can't setup disk for non-block device"));
@@ -3712,12 +3718,6 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
vm->def, def) < 0)
goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
-
if (virCgroupAllowDevicePath(priv->cgroup, def->src,
(def->readonly ?
VIR_CGROUP_DEVICE_READ :
@@ -3914,12 +3914,6 @@ lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver,
def->source.subsys.u.usb.device) < 0)
goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
-
if (!(usb = virUSBDeviceNew(def->source.subsys.u.usb.bus,
def->source.subsys.u.usb.device, vroot)))
goto cleanup;
@@ -4067,12 +4061,6 @@ lxcDomainAttachDeviceHostdevStorageLive(virLXCDriverPtr driver,
vm->def, def, vroot) < 0)
goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
-
if (virCgroupAllowDevicePath(priv->cgroup, def->source.caps.u.storage.block,
VIR_CGROUP_DEVICE_RW |
VIR_CGROUP_DEVICE_MKNOD) != 0) {
@@ -4175,12 +4163,6 @@ lxcDomainAttachDeviceHostdevMiscLive(virLXCDriverPtr driver,
vm->def, def, vroot) < 0)
goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
-
if (virCgroupAllowDevicePath(priv->cgroup, def->source.caps.u.misc.chardev,
VIR_CGROUP_DEVICE_RW |
VIR_CGROUP_DEVICE_MKNOD) != 0) {
@@ -4256,6 +4238,12 @@ lxcDomainAttachDeviceHostdevLive(virLXCDriverPtr driver,
return -1;
}
+ if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("devices cgroup isn't mounted"));
+ return -1;
+ }
+
switch (dev->data.hostdev->mode) {
case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
return lxcDomainAttachDeviceHostdevSubsysLive(driver, vm, dev);
--
1.8.5.3
10:43 a.m.
New subject: [libvirt] [PATCH 06/14] Move check for cgroup devices ACL upfront in LXC hotplug
On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
The check for whether the cgroup devices ACL is available is
done quite late during LXC hotplug - in fact after the device
node is already created in the container in some cases. Better
todo it upfront so we fail immediately.
s/todo/to do/
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 36 ++++++++++++------------------------
1 file changed, 12 insertions(+), 24 deletions(-)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 07/14] Add virFileMakeParentPath helper function
Add a helper function which takes a file path and ensures
that all directory components leading upto 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(a)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 2c9536a..5554a30 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1225,6 +1225,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
10:58 a.m.
New subject: [libvirt] [PATCH 07/14] Add virFileMakeParentPath helper function
On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
Add a helper function which takes a file path and ensures
that all directory components leading upto the file exist.
s/upto/up to/
IOW, it strips the filename part of the path and passes
the result to virFileMakePath.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 27 +++++++++++++++++++++++++++
src/util/virfile.h | 1 +
3 files changed, 29 insertions(+)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 08/14] Add helper for running code in separate namespaces
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(a)gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Daniel Berrange <berrange(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virprocess.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++
src/util/virprocess.h | 11 +++++
3 files changed, 126 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5554a30..3066506 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1670,6 +1670,7 @@ virProcessGetNamespaces;
virProcessGetStartTime;
virProcessKill;
virProcessKillPainfully;
+virProcessRunInMountNamespace;
virProcessSetAffinity;
virProcessSetMaxFiles;
virProcessSetMaxMemLock;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 83d0679..a0cbfbf 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,115 @@ 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 EXIT_CANCELED; 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);
+ else if (child > 0)
+ 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 {
+ int status;
+ char *buf = NULL;
+ VIR_FORCE_CLOSE(errfd[1]);
+
+ ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
+ if (virProcessWait(child, &status) < 0)
+ ret = -1;
+ else
+ ret = status == 0 ? 0 : -1;
+ if (ret < 0)
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ buf ? buf : _("Failed to run callback in mount
namespace"));
+ 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..75c7d1b 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 EXIT_CANCELED. */
+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
11:31 a.m.
New subject: [libvirt] [PATCH 08/14] Add helper for running code in separate namespaces
On 02/07/2014 08:33 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(a)gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Daniel Berrange <berrange(a)redhat.com>
Looks like you improved from my version, so it would be nice to add:
Signed-off-by: Eric Blake <eblake(a)redhat.com>
+
+/* 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 EXIT_CANCELED; otherwise
Comment is wrong, since EXIT_CANCELED is not defined yet (it will be
fixed when I rebase the virFork cleanups on top of this).
+ if (ret < 0 || child < 0) {
+ if (child == 0)
+ _exit(1);
Similarly, I prefer EXIT_FAILURE over the magic '1'. But I don't mind
cleaning that up in my rebased virFork stuff.
+ else if (child > 0)
The else is redundant, since _exit() never returns.
+ 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);
Again, more magic numbers I can clean up later.
+
+ ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
+ if (virProcessWait(child, &status) < 0)
+ ret = -1;
+ else
+ ret = status == 0 ? 0 : -1;
This can be simplified:
ret = virProcessWait(child, NULL);
+ if (ret < 0)
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ buf ? buf : _("Failed to run callback in mount
namespace"));
And this error message overwrites the error from virProcessWait.
+/* 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 EXIT_CANCELED. */
+typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque);
Again, the comment doesn't quite work with the rearranged patch order.
My comments above are improvements, but I didn't spot any bugs in your
implementation.
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:34 a.m.
New subject: [libvirt] [PATCH 08/14] Add helper for running code in separate namespaces
On Fri, Feb 07, 2014 at 10:31:03AM -0700, Eric Blake wrote:
On 02/07/2014 08:33 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(a)gmail.com> at
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
>
> Signed-off-by: Daniel Berrange <berrange(a)redhat.com>
Looks like you improved from my version, so it would be nice to add:
Signed-off-by: Eric Blake <eblake(a)redhat.com>
Sure. What happened is that I started with your code, then I
rewrote the method from scratch to use threads, then I found
threads + mount namespace didn't work, so I rewrote it again
to use fork again. Fun times :-)
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:33 a.m.
New subject: [libvirt] [PATCH 09/14] Avoid unsafe use of /proc/$PID/root in LXC shutdown/reboot code
From: Eric Blake <eblake(a)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(a)gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Eric Blake <eblake(a)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 51ca98c..96987cd 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3307,12 +3307,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;
@@ -3339,16 +3347,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",
@@ -3374,7 +3380,6 @@ lxcDomainShutdownFlags(virDomainPtr dom,
ret = 0;
cleanup:
- VIR_FREE(vroot);
if (vm)
virObjectUnlock(vm);
return ret;
@@ -3386,13 +3391,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;
@@ -3419,16 +3424,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",
@@ -3454,7 +3457,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..18052c0 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-2013 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
11:38 a.m.
New subject: [libvirt] [PATCH 09/14] Avoid unsafe use of /proc/$PID/root in LXC shutdown/reboot code
On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
From: Eric Blake <eblake(a)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(a)gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Eric Blake <eblake(a)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(-)
I guess the fact that you reposted my patch serves as the ACK :)
+++ 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-2013 Red Hat, Inc.
Oh my - I wrote it that long ago. We can add 2014 here.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 10/14] Avoid unsafe use of /proc/$PID/root in LXC disk hotplug
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(a)redhat.com>
---
src/lxc/lxc_driver.c | 183 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 139 insertions(+), 44 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 96987cd..a1d0e81 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3627,6 +3627,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,
@@ -3635,11 +3744,8 @@ 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;
if (!priv->initpid) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -3683,51 +3789,43 @@ 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;
-
- /* 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),
+ (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);
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 (virCgroupDenyDevicePath(priv->cgroup, def->src,
+ VIR_CGROUP_DEVICE_RWM) < 0)
+ VIR_WARN("cannot deny device %s for domain %s",
+ def->src, vm->def->name);
goto cleanup;
}
@@ -3736,11 +3834,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
12:22 p.m.
New subject: [libvirt] [PATCH 10/14] Avoid unsafe use of /proc/$PID/root in LXC disk hotplug
On 02/07/2014 08:33 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(a)redhat.com>
---
src/lxc/lxc_driver.c | 183 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 139 insertions(+), 44 deletions(-)
+ }
+
+
+ /* Yes, the device name we're creating may not
Why two blank lines?
+
+ if (lxcContainerChown(data->vm->def, data->file) < 0)
+ goto cleanup;
Hmm. Calling _() can trigger malloc(), which is not async-safe in
general. However, at least with glibc, I've never seen malloc deadlock
because of use after multi-threaded fork, and we already have other
spots in the code base that would also need cleaning if we truly want to
avoid malloc between fork and exec.
+
+ /* 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) {
Wow, that's quite the call stack that gets pulled in, when auditing for
async safety. In particular, I have no idea if setfilecon_raw() and
getfilecon_raw() are safe. We might be able to argue that because you
grab virSecurityManagerPreFork, then no other libvirt threads will be
using getfilecon_raw() at the same time; and since this code is only run
in libvirtd, we don't have to worry about non-libvirt threads in the
context of a larger application that linked in libvirt.so. So I _think_
we are safe.
+ def->src = tmpsrc;
+ goto cleanup;
+ }
+ def->src = tmpsrc;
+ } break;
+
No real need to put def back to normal - we're in a forked child and
about to exit. But doesn't hurt either, in case this gets copied and
pasted to something in parent context where it does matter.
+static int lxcDomainAttachDeviceMknod(virLXCDriverPtr driver,
Style nit for breaking this to two lines, but not a showstopper.
- if (lxcContainerChown(vm->def, dst) < 0)
+ if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
spacing around +
+ if (lxcDomainAttachDeviceMknod(driver,
+ 0700 | S_IFBLK,
+ sb.st_rdev,
+ vm,
+ dev,
+ file) < 0) {
+ if (virCgroupDenyDevicePath(priv->cgroup, def->src,
+ VIR_CGROUP_DEVICE_RWM) < 0)
+ VIR_WARN("cannot deny device %s for domain %s",
+ def->src, vm->def->name);
This denies by path, while the allow was fixed to be by major:minor,
which means you aren't denying what you thought. This needs to deny by
major:minor.
Do we need to audit this cleanup action?
goto cleanup;
}
@@ -3736,11 +3834,8 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
ret = 0;
cleanup:
- def->src = tmpsrc;
virDomainAuditDisk(vm, NULL, def->src, "attach", ret == 0);
As written, you only audit the grant, not the corresponding deny on
failure. That's a pre-existing issue, not made worse by your patch, but
something to consider whether it warrants a v2 or a followup patch to fix.
My overall thoughts:
If we had a way to do _just_ the mknod, then open the file, and pass the
fd back to the parent, then do labeling on the fd from the parent
context (rather than on the path in the child context), it would make
for a smaller child action easier to audit. But I'm not sure that would
get the labeling right - it looks like we have to label the actual path
name in the child. Or even if selinux took a leaf from openat() and
friends, and gave us the ability to do actions on a name relative to an
fd, then all we'd need to do is fork, change namespace, open the fd of
the container directory, pass that back, then do the remaining options
in the parent, where life is much easier.
But lacking either of those options, I think your approach is the best
we can do. Please fix the cgroup deny, then I can give a reluctant:
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:04 a.m.
New subject: [libvirt] [PATCH 10/14] Avoid unsafe use of /proc/$PID/root in LXC disk hotplug
On Fri, Feb 07, 2014 at 11:22:12AM -0700, Eric Blake wrote:
On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
My overall thoughts:
If we had a way to do _just_ the mknod, then open the file, and pass the
fd back to the parent, then do labeling on the fd from the parent
context (rather than on the path in the child context), it would make
for a smaller child action easier to audit. But I'm not sure that would
get the labeling right - it looks like we have to label the actual path
name in the child. Or even if selinux took a leaf from openat() and
friends, and gave us the ability to do actions on a name relative to an
fd, then all we'd need to do is fork, change namespace, open the fd of
the container directory, pass that back, then do the remaining options
in the parent, where life is much easier.
The FD passing idea is interesting. I think I will explore that idea
further to see if it is viable before we finalize this.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:33 a.m.
New subject: [libvirt] [PATCH 11/14] Avoid unsafe use of /proc/$PID/root in LXC USB hotplug
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(a)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 a1d0e81..589a45d 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3686,6 +3686,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"),
@@ -3977,13 +3984,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) {
@@ -3992,27 +3994,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) {
@@ -4028,53 +4016,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
1:43 p.m.
New subject: [libvirt] [PATCH 11/14] Avoid unsafe use of /proc/$PID/root in LXC USB hotplug
On 02/07/2014 08:33 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(a)redhat.com>
---
src/lxc/lxc_driver.c | 73 ++++++++++++++++------------------------------------
1 file changed, 22 insertions(+), 51 deletions(-)
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 12/14] Avoid unsafe use of /proc/$PID/root in LXC block hostdev hotplug
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(a)redhat.com>
---
src/lxc/lxc_driver.c | 64 ++++++++++++++--------------------------------------
1 file changed, 17 insertions(+), 47 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 589a45d..5780e11 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4058,11 +4058,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",
@@ -4090,64 +4086,38 @@ 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)
- goto cleanup;
-
- if (virCgroupAllowDevicePath(priv->cgroup, def->source.caps.u.storage.block,
- VIR_CGROUP_DEVICE_RW |
- VIR_CGROUP_DEVICE_MKNOD) != 0) {
+ if (virCgroupAllowDevicePath(priv->cgroup,
+ def->source.caps.u.storage.block,
+ VIR_CGROUP_DEVICE_RWM) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot allow device %s for domain %s"),
def->source.caps.u.storage.block, vm->def->name);
goto cleanup;
}
+ if (lxcDomainAttachDeviceMknod(driver,
+ 0700 | S_IFBLK,
+ sb.st_rdev,
+ vm,
+ dev,
+ def->source.caps.u.storage.block) < 0) {
+ if (virCgroupDenyDevicePath(priv->cgroup,
+ def->source.caps.u.storage.block,
+ 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;
+ }
+
vm->def->hostdevs[vm->def->nhostdevs++] = def;
ret = 0;
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
1:47 p.m.
New subject: [libvirt] [PATCH 12/14] Avoid unsafe use of /proc/$PID/root in LXC block hostdev hotplug
On 02/07/2014 08:33 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(a)redhat.com>
---
src/lxc/lxc_driver.c | 64 ++++++++++++++--------------------------------------
1 file changed, 17 insertions(+), 47 deletions(-)
+ if (virCgroupAllowDevicePath(priv->cgroup,
+ def->source.caps.u.storage.block,
+ VIR_CGROUP_DEVICE_RWM) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot allow device %s for domain %s"),
def->source.caps.u.storage.block, vm->def->name);
Here, I think we're okay setting up cgroup by name - because it is the
host-visible name, not the guest-visible name, and cgroup just stat()s
the host name to use the major:minor anyways. But since we already have
sb.st_rdev, why not avoid a second stat() and just tell cgroup to change
by major:minor in the first place?
goto cleanup;
}
+ if (lxcDomainAttachDeviceMknod(driver,
+ 0700 | S_IFBLK,
+ sb.st_rdev,
+ vm,
+ dev,
+ def->source.caps.u.storage.block) < 0) {
+ if (virCgroupDenyDevicePath(priv->cgroup,
+ def->source.caps.u.storage.block,
+ 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;
Same problem as in 10/14 about no audit of cleanup attempts. And again,
something probably worth fixing in its own patch, so for this one:
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 13/14] Avoid unsafe use of /proc/$PID/root in LXC chardev hostdev hotplug
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(a)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 5780e11..763b22b 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4130,11 +4130,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",
@@ -4162,51 +4158,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)
- 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);
+ if (virCgroupAllowDevicePath(priv->cgroup,
+ def->source.caps.u.misc.chardev,
+ VIR_CGROUP_DEVICE_RWM) != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot allow device %s for domain %s"),
+ def->source.caps.u.misc.chardev, vm->def->name);
goto cleanup;
}
- created = true;
-
- if (lxcContainerChown(vm->def, dst) < 0)
- goto cleanup;
- if (virSecurityManagerSetHostdevLabel(driver->securityManager,
- vm->def, def, vroot) < 0)
+ if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 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 (virCgroupDenyDevicePath(priv->cgroup,
+ def->source.caps.u.misc.chardev,
+ 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;
}
@@ -4216,10 +4190,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
1:49 p.m.
New subject: [libvirt] [PATCH 13/14] Avoid unsafe use of /proc/$PID/root in LXC chardev hostdev hotplug
On 02/07/2014 08:33 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(a)redhat.com>
---
src/lxc/lxc_driver.c | 66 ++++++++++++++--------------------------------------
1 file changed, 18 insertions(+), 48 deletions(-)
+ if (virCgroupAllowDevicePath(priv->cgroup,
+ def->source.caps.u.misc.chardev,
+ VIR_CGROUP_DEVICE_RWM) != 0) {
Again, why make cgroup do another stat of the host-side name, when we've
already done a stat and could tell cgroup to operate on major:minor?
+
def->source.caps.u.misc.chardev) < 0) {
+ if (virCgroupDenyDevicePath(priv->cgroup,
+ def->source.caps.u.misc.chardev,
+ 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;
And same issue on no auditing of deny on the failure path.
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:33 a.m.
New subject: [libvirt] [PATCH 14/14] Avoid unsafe use of /proc/$PID/root in LXC hotunplug code
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(a)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 763b22b..5940343 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3744,6 +3744,39 @@ static int 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)
@@ -4332,8 +4365,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)) {
@@ -4342,11 +4374,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);
@@ -4441,7 +4470,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,
@@ -4452,12 +4480,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;
@@ -4472,11 +4495,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);
@@ -4499,7 +4519,6 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver,
cleanup:
virUSBDeviceFree(usb);
VIR_FREE(dst);
- VIR_FREE(vroot);
return ret;
}
@@ -4511,7 +4530,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",
@@ -4528,22 +4546,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);
@@ -4558,7 +4568,6 @@ lxcDomainDetachDeviceHostdevStorageLive(virDomainObjPtr vm,
ret = 0;
cleanup:
- VIR_FREE(dst);
return ret;
}
@@ -4570,7 +4579,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",
@@ -4587,22 +4595,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);
@@ -4617,7 +4617,6 @@ lxcDomainDetachDeviceHostdevMiscLive(virDomainObjPtr vm,
ret = 0;
cleanup:
- VIR_FREE(dst);
return ret;
}
--
1.8.5.3
1:51 p.m.
New subject: [libvirt] [PATCH 14/14] Avoid unsafe use of /proc/$PID/root in LXC hotunplug code
On 02/07/2014 08:33 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
symlink, tricking the driver into changing the host OS
filesystem.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/lxc/lxc_driver.c | 79 ++++++++++++++++++++++++++--------------------------
1 file changed, 39 insertions(+), 40 deletions(-)
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);
Same generic concern about _() using malloc in an async-safe context,
but not worth worrying about in this patch.
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3940
days inactive
3943
days old
31 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Eric Blake