[libvirt] [PATCH 0/3] SEV: Make /dev/sev available only to domains that need it
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
We should not give domains access to something they don't necessarily need by default. Signed-off-by: Erik Skultety <eskultet@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
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@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
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@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
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@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
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@redhat.com> --- src/qemu/qemu_cgroup.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
ACK Michal
Instead of exposing /dev/sev to every domain, do it selectively. Signed-off-by: Erik Skultety <eskultet@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
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@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
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@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
participants (2)
-
Erik Skultety -
Michal Privoznik