6:57 a.m.
Erik Skultety (3):
qemu: conf: Remove /dev/sev from the default cgroup device acl list
qemu: cgroup: Expose /dev/sev/ only to domains that require SEV
qemu: domain: Add /dev/sev into the domain mount namespace selectively
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_cgroup.c | 21 ++++++++++++++++++++-
src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
src/qemu/test_libvirtd_qemu.aug.in | 1 -
4 files changed, 44 insertions(+), 3 deletions(-)
--
2.20.1
6:57 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: conf: Remove /dev/sev from the default cgroup device acl list
We should not give domains access to something they don't necessarily
need by default.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_cgroup.c | 2 +-
src/qemu/test_libvirtd_qemu.aug.in | 1 -
3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index c1f1201134..7820e72dd8 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -490,7 +490,7 @@
# "/dev/null", "/dev/full", "/dev/zero",
# "/dev/random", "/dev/urandom",
# "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
-# "/dev/rtc","/dev/hpet", "/dev/sev"
+# "/dev/rtc","/dev/hpet"
#]
#
# RDMA migration requires the following extra files to be added to the list:
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 4931fb6575..1eb5bffce3 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -46,7 +46,7 @@ const char *const defaultDeviceACL[] = {
"/dev/null", "/dev/full", "/dev/zero",
"/dev/random", "/dev/urandom",
"/dev/ptmx", "/dev/kvm", "/dev/kqemu",
- "/dev/rtc", "/dev/hpet", "/dev/sev",
+ "/dev/rtc", "/dev/hpet",
NULL,
};
#define DEVICE_PTY_MAJOR 136
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 4235464530..51a7ad5892 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -63,7 +63,6 @@ module Test_libvirtd_qemu =
{ "8" = "/dev/kqemu" }
{ "9" = "/dev/rtc" }
{ "10" = "/dev/hpet" }
- { "11" = "/dev/sev" }
}
{ "save_image_format" = "raw" }
{ "dump_image_format" = "raw" }
--
2.20.1
6:26 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: conf: Remove /dev/sev from the default cgroup device acl list
On 1/23/19 1:57 PM, Erik Skultety wrote:
We should not give domains access to something they don't
necessarily
need by default.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_cgroup.c | 2 +-
src/qemu/test_libvirtd_qemu.aug.in | 1 -
3 files changed, 2 insertions(+), 3 deletions(-)
ACK if you remove it from docs/drvqemu.html.in too. BTW: /dev/net/tun
should be removed from there as well.
Michal
9:30 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: conf: Remove /dev/sev from the default cgroup device acl list
On Tue, Jan 29, 2019 at 01:26:47PM +0100, Michal Privoznik wrote:
On 1/23/19 1:57 PM, Erik Skultety wrote:
> We should not give domains access to something they don't necessarily
> need by default.
>
> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
> ---
> src/qemu/qemu.conf | 2 +-
> src/qemu/qemu_cgroup.c | 2 +-
> src/qemu/test_libvirtd_qemu.aug.in | 1 -
> 3 files changed, 2 insertions(+), 3 deletions(-)
ACK if you remove it from docs/drvqemu.html.in too. BTW: /dev/net/tun should
be removed from there as well.
Thanks, fixed. I fixed the /dev/net/tun thing separately in a trivial patch.
Erik
6:57 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: cgroup: Expose /dev/sev/ only to domains that require SEV
SEV has a limit on number of concurrent guests. From security POV we
should only expose resources (any resources for that matter) to domains
that truly need them.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 1eb5bffce3..2f9d34ebd2 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -691,6 +691,22 @@ qemuTeardownChardevCgroup(virDomainObjPtr vm,
}
+static int
+qemuSetupSEVCgroup(virDomainObjPtr vm)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ int ret;
+
+ if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
+ return 0;
+
+ ret = virCgroupAllowDevicePath(priv->cgroup, "/dev/sev",
+ VIR_CGROUP_DEVICE_RW, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow",
"/dev/sev",
+ "rw", ret);
+ return ret;
+}
+
static int
qemuSetupDevicesCgroup(virDomainObjPtr vm)
{
@@ -798,6 +814,9 @@ qemuSetupDevicesCgroup(virDomainObjPtr vm)
goto cleanup;
}
+ if (vm->def->sev && qemuSetupSEVCgroup(vm) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
virObjectUnref(cfg);
--
2.20.1
6:26 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: cgroup: Expose /dev/sev/ only to domains that require SEV
On 1/23/19 1:57 PM, Erik Skultety wrote:
SEV has a limit on number of concurrent guests. From security POV we
should only expose resources (any resources for that matter) to domains
that truly need them.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
ACK
Michal
6:57 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: domain: Add /dev/sev into the domain mount namespace selectively
Instead of exposing /dev/sev to every domain, do it selectively.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 32a43f2064..a4cdb8d355 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12112,6 +12112,26 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg
ATTRIBUTE_UNUSED,
}
+static int
+qemuDomainSetupLaunchSecurity(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
+ const struct qemuDomainCreateDeviceData *data)
+{
+ virDomainSEVDefPtr sev = vm->def->sev;
+
+ if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV)
+ return 0;
+
+ VIR_DEBUG("Setting up launch security");
+
+ if (qemuDomainCreateDevice("/dev/sev", data, false) < 0)
+ return -1;
+
+ VIR_DEBUG("Set up launch security");
+ return 0;
+}
+
+
int
qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
virSecurityManagerPtr mgr,
@@ -12183,6 +12203,9 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
if (qemuDomainSetupLoader(cfg, vm, &data) < 0)
goto cleanup;
+ if (qemuDomainSetupLaunchSecurity(cfg, vm, &data) < 0)
+ goto cleanup;
+
/* Save some mount points because we want to share them with the host */
for (i = 0; i < ndevMountsPath; i++) {
struct stat sb;
--
2.20.1
6:26 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: domain: Add /dev/sev into the domain mount namespace selectively
On 1/23/19 1:57 PM, Erik Skultety wrote:
Instead of exposing /dev/sev to every domain, do it selectively.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 32a43f2064..a4cdb8d355 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12112,6 +12112,26 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg
ATTRIBUTE_UNUSED,
}
+static int
+qemuDomainSetupLaunchSecurity(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
+ const struct qemuDomainCreateDeviceData *data)
+{
+ virDomainSEVDefPtr sev = vm->def->sev;
+
+ if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV)
+ return 0;
+
+ VIR_DEBUG("Setting up launch security");
+
+ if (qemuDomainCreateDevice("/dev/sev", data, false) < 0)
nitpick - I'd rather see this as a macro:
#define SEV_PATH "/dev/sev"
...
qemuDomainCreateDevice(SEV_PATH, ..)
+ return -1;
+
+ VIR_DEBUG("Set up launch security");
+ return 0;
+}
+
+
ACK
Michal
9:32 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: domain: Add /dev/sev into the domain mount namespace selectively
On Tue, Jan 29, 2019 at 01:26:46PM +0100, Michal Privoznik wrote:
On 1/23/19 1:57 PM, Erik Skultety wrote:
> Instead of exposing /dev/sev to every domain, do it selectively.
>
> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 32a43f2064..a4cdb8d355 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -12112,6 +12112,26 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg
ATTRIBUTE_UNUSED,
> }
> +static int
> +qemuDomainSetupLaunchSecurity(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm,
> + const struct qemuDomainCreateDeviceData *data)
> +{
> + virDomainSEVDefPtr sev = vm->def->sev;
> +
> + if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV)
> + return 0;
> +
> + VIR_DEBUG("Setting up launch security");
> +
> + if (qemuDomainCreateDevice("/dev/sev", data, false) < 0)
nitpick - I'd rather see this as a macro:
#define SEV_PATH "/dev/sev"
...
qemuDomainCreateDevice(SEV_PATH, ..)
Fixed,
although I didn't push the patches, as the SEV probing discussion upstream
concluded in libvirt using DAC_OVERRIDE capability, so I did that and applied
it on top of this series. Also, I forgot to make sure DAC relabels the device
within namespace so I fixed that in the new series as well.
Thanks,
Erik
2152
days inactive
2160
days old
8 comments
2 participants
participants (2)
-
Erik Skultety -
Michal Privoznik