From 3558b40b5bd95f2dbea40d5c787536a903d98e2a1839e5945fdd31f3d77655ac Mon Sep 17 00:00:00 2001 From: James Fehlig Date: Fri, 8 Feb 2019 22:26:33 +0000 Subject: [PATCH] Accepting request 672885 from home:jfehlig:branches:Virtualization - qemu: fix issues related to restricted permissions on /dev/sev b6440119-qemu-conf-sev.patch, a404ac34-qemu-cgroup-sev.patch, 6fd4c8f8-qemu-domain-sev.patch, 17f6a257-security-dac-sev.patch, a2d3dea9-qemu-caps-dac-override-sev.patch bsc#1124842 OBS-URL: https://build.opensuse.org/request/show/672885 OBS-URL: https://build.opensuse.org/package/show/Virtualization/libvirt?expand=0&rev=728 --- 17f6a257-security-dac-sev.patch | 103 ++++++++++++++++++++ 6fd4c8f8-qemu-domain-sev.patch | 60 ++++++++++++ a2d3dea9-qemu-caps-dac-override-sev.patch | 112 ++++++++++++++++++++++ a404ac34-qemu-cgroup-sev.patch | 50 ++++++++++ b6440119-qemu-conf-sev.patch | 64 +++++++++++++ libvirt.changes | 9 ++ libvirt.spec | 10 ++ 7 files changed, 408 insertions(+) create mode 100644 17f6a257-security-dac-sev.patch create mode 100644 6fd4c8f8-qemu-domain-sev.patch create mode 100644 a2d3dea9-qemu-caps-dac-override-sev.patch create mode 100644 a404ac34-qemu-cgroup-sev.patch create mode 100644 b6440119-qemu-conf-sev.patch diff --git a/17f6a257-security-dac-sev.patch b/17f6a257-security-dac-sev.patch new file mode 100644 index 0000000..0ecb48f --- /dev/null +++ b/17f6a257-security-dac-sev.patch @@ -0,0 +1,103 @@ +commit 17f6a257f1ea484489277f4da38be914b246a30b +Author: Erik Skultety +Date: Thu Jan 31 15:16:50 2019 +0100 + + security: dac: Relabel /dev/sev in the namespace + + The default permissions (0600 root:root) are of no use to the qemu + process so we need to change the owner to qemu iff running with + namespaces. + + Signed-off-by: Erik Skultety + Reviewed-by: Daniel P. Berrangé + +Index: libvirt-5.0.0/src/security/security_dac.c +=================================================================== +--- libvirt-5.0.0.orig/src/security/security_dac.c ++++ libvirt-5.0.0/src/security/security_dac.c +@@ -48,6 +48,7 @@ + VIR_LOG_INIT("security.security_dac"); + + #define SECURITY_DAC_NAME "dac" ++#define DEV_SEV "/dev/sev" + + typedef struct _virSecurityDACData virSecurityDACData; + typedef virSecurityDACData *virSecurityDACDataPtr; +@@ -1690,6 +1691,16 @@ virSecurityDACRestoreMemoryLabel(virSecu + + + static int ++virSecurityDACRestoreSEVLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, ++ virDomainDefPtr def ATTRIBUTE_UNUSED) ++{ ++ /* we only label /dev/sev when running with namespaces, so we don't need to ++ * restore anything */ ++ return 0; ++} ++ ++ ++static int + virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + bool migrated, +@@ -1759,6 +1770,11 @@ virSecurityDACRestoreAllLabel(virSecurit + rc = -1; + } + ++ if (def->sev) { ++ if (virSecurityDACRestoreSEVLabel(mgr, def) < 0) ++ rc = -1; ++ } ++ + if (def->os.loader && def->os.loader->nvram && + virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0) + rc = -1; +@@ -1833,6 +1849,36 @@ virSecurityDACSetMemoryLabel(virSecurity + + + static int ++virSecurityDACSetSEVLabel(virSecurityManagerPtr mgr, ++ virDomainDefPtr def) ++{ ++ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); ++ virSecurityLabelDefPtr seclabel; ++ uid_t user; ++ gid_t group; ++ ++ /* Skip chowning /dev/sev if namespaces are disabled as we'd significantly ++ * increase the chance of a DOS attack on SEV ++ */ ++ if (!priv->mountNamespace) ++ return 0; ++ ++ seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); ++ if (seclabel && !seclabel->relabel) ++ return 0; ++ ++ if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) ++ return -1; ++ ++ if (virSecurityDACSetOwnership(mgr, NULL, DEV_SEV, ++ user, group, false) < 0) ++ return -1; ++ ++ return 0; ++} ++ ++ ++static int + virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *stdin_path ATTRIBUTE_UNUSED, +@@ -1902,6 +1948,11 @@ virSecurityDACSetAllLabel(virSecurityMan + return -1; + } + ++ if (def->sev) { ++ if (virSecurityDACSetSEVLabel(mgr, def) < 0) ++ return -1; ++ } ++ + if (virSecurityDACGetImageIds(secdef, priv, &user, &group)) + return -1; + diff --git a/6fd4c8f8-qemu-domain-sev.patch b/6fd4c8f8-qemu-domain-sev.patch new file mode 100644 index 0000000..170725d --- /dev/null +++ b/6fd4c8f8-qemu-domain-sev.patch @@ -0,0 +1,60 @@ +commit 6fd4c8f8785a063112c8161a3a3f5ad3cb6647ea +Author: Erik Skultety +Date: Tue Jan 22 13:46:16 2019 +0100 + + 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 + Reviewed-by: Daniel P. Berrangé + +Index: libvirt-5.0.0/src/qemu/qemu_domain.c +=================================================================== +--- libvirt-5.0.0.orig/src/qemu/qemu_domain.c ++++ libvirt-5.0.0/src/qemu/qemu_domain.c +@@ -116,6 +116,7 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_ + #define DEVPREFIX "/dev/" + #define DEV_VFIO "/dev/vfio/vfio" + #define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control" ++#define DEV_SEV "/dev/sev" + + + struct _qemuDomainLogContext { +@@ -12018,6 +12019,26 @@ qemuDomainSetupLoader(virQEMUDriverConfi + } + + ++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, +@@ -12089,6 +12110,9 @@ qemuDomainBuildNamespace(virQEMUDriverCo + 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; diff --git a/a2d3dea9-qemu-caps-dac-override-sev.patch b/a2d3dea9-qemu-caps-dac-override-sev.patch new file mode 100644 index 0000000..05955b9 --- /dev/null +++ b/a2d3dea9-qemu-caps-dac-override-sev.patch @@ -0,0 +1,112 @@ +commit a2d3dea9d41dba313d9566120a8ec9d358567bd0 +Author: Erik Skultety +Date: Thu Jan 24 10:33:01 2019 +0100 + + qemu: caps: Use CAP_DAC_OVERRIDE for probing to avoid permission issues + + This is mainly about /dev/sev and its default permissions 0600. Of + course, rule of 'tinfoil' would be that we can't trust anything, but the + probing code in QEMU is considered safe from security's perspective + we + can't create an udev rule for this at the moment, because ioctls and + file system permissions aren't cross-checked in kernel and therefore a + user with read permissions could issue a 'privileged' operation on SEV + which is currently only limited to root. + + https://bugzilla.redhat.com/show_bug.cgi?id=1665400 + + Signed-off-by: Erik Skultety + Reviewed-by: Daniel P. Berrangé + +Index: libvirt-5.0.0/src/qemu/qemu_capabilities.c +=================================================================== +--- libvirt-5.0.0.orig/src/qemu/qemu_capabilities.c ++++ libvirt-5.0.0/src/qemu/qemu_capabilities.c +@@ -53,6 +53,10 @@ + #include + #include + ++#if WITH_CAPNG ++# include ++#endif ++ + #define VIR_FROM_THIS VIR_FROM_QEMU + + VIR_LOG_INIT("qemu.qemu_capabilities"); +@@ -4521,6 +4525,13 @@ virQEMUCapsInitQMPCommandRun(virQEMUCaps + NULL); + virCommandAddEnvPassCommon(cmd->cmd); + virCommandClearCaps(cmd->cmd); ++ ++#if WITH_CAPNG ++ /* QEMU might run into permission issues, e.g. /dev/sev (0600), override ++ * them just for the purpose of probing */ ++ virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE); ++#endif ++ + virCommandSetGID(cmd->cmd, cmd->runGid); + virCommandSetUID(cmd->cmd, cmd->runUid); + +Index: libvirt-5.0.0/src/util/virutil.c +=================================================================== +--- libvirt-5.0.0.orig/src/util/virutil.c ++++ libvirt-5.0.0/src/util/virutil.c +@@ -1502,8 +1502,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gi + { + size_t i; + int capng_ret, ret = -1; +- bool need_setgid = false, need_setuid = false; ++ bool need_setgid = false; ++ bool need_setuid = false; + bool need_setpcap = false; ++ const char *capstr = NULL; + + /* First drop all caps (unless the requested uid is "unchanged" or + * root and clearExistingCaps wasn't requested), then add back +@@ -1512,14 +1514,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gi + */ + + if (clearExistingCaps || (uid != (uid_t)-1 && uid != 0)) +- capng_clear(CAPNG_SELECT_BOTH); ++ capng_clear(CAPNG_SELECT_BOTH); + + for (i = 0; i <= CAP_LAST_CAP; i++) { ++ capstr = capng_capability_to_name(i); ++ + if (capBits & (1ULL << i)) { + capng_update(CAPNG_ADD, + CAPNG_EFFECTIVE|CAPNG_INHERITABLE| + CAPNG_PERMITTED|CAPNG_BOUNDING_SET, + i); ++ ++ VIR_DEBUG("Added '%s' to child capabilities' set", capstr); + } + } + +@@ -1579,6 +1585,27 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gi + goto cleanup; + } + ++# ifdef PR_CAP_AMBIENT ++ /* we couldn't do this in the loop earlier above, because the capabilities ++ * were not applied yet, since in order to add a capability into the AMBIENT ++ * set, it has to be present in both the PERMITTED and INHERITABLE sets ++ * (capabilities(7)) ++ */ ++ for (i = 0; i <= CAP_LAST_CAP; i++) { ++ capstr = capng_capability_to_name(i); ++ ++ if (capBits & (1ULL << i)) { ++ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, i, 0, 0) < 0) { ++ virReportSystemError(errno, ++ _("prctl failed to enable '%s' in the " ++ "AMBIENT set"), ++ capstr); ++ goto cleanup; ++ } ++ } ++ } ++# endif ++ + /* Set bounding set while we have CAP_SETPCAP. Unfortunately we cannot + * do this if we failed to get the capability above, so ignore the + * return value. diff --git a/a404ac34-qemu-cgroup-sev.patch b/a404ac34-qemu-cgroup-sev.patch new file mode 100644 index 0000000..af17e39 --- /dev/null +++ b/a404ac34-qemu-cgroup-sev.patch @@ -0,0 +1,50 @@ +commit a404ac34768e975bd420d1eeac3811563da67e3f +Author: Erik Skultety +Date: Mon Jan 21 14:50:11 2019 +0100 + + 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 + Reviewed-by: Daniel P. Berrangé + +Index: libvirt-5.0.0/src/qemu/qemu_cgroup.c +=================================================================== +--- libvirt-5.0.0.orig/src/qemu/qemu_cgroup.c ++++ libvirt-5.0.0/src/qemu/qemu_cgroup.c +@@ -692,6 +692,22 @@ qemuTeardownChardevCgroup(virDomainObjPt + + + 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) + { + qemuDomainObjPrivatePtr priv = vm->privateData; +@@ -798,6 +814,9 @@ qemuSetupDevicesCgroup(virDomainObjPtr v + goto cleanup; + } + ++ if (vm->def->sev && qemuSetupSEVCgroup(vm) < 0) ++ goto cleanup; ++ + ret = 0; + cleanup: + virObjectUnref(cfg); diff --git a/b6440119-qemu-conf-sev.patch b/b6440119-qemu-conf-sev.patch new file mode 100644 index 0000000..e8b0eb3 --- /dev/null +++ b/b6440119-qemu-conf-sev.patch @@ -0,0 +1,64 @@ +commit b6440119185a4e307654a8d26d6d551a2675bf82 +Author: Erik Skultety +Date: Mon Jan 21 14:48:02 2019 +0100 + + 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. Remove it from the qemu driver docs too. + + Signed-off-by: Erik Skultety + Reviewed-by: Daniel P. Berrangé + +Index: libvirt-5.0.0/docs/drvqemu.html.in +=================================================================== +--- libvirt-5.0.0.orig/docs/drvqemu.html.in ++++ libvirt-5.0.0/docs/drvqemu.html.in +@@ -396,8 +396,7 @@ chmod o+x /path/to/directory + /dev/null, /dev/full, /dev/zero, + /dev/random, /dev/urandom, + /dev/ptmx, /dev/kvm, /dev/kqemu, +-/dev/rtc, /dev/hpet, /dev/net/tun, +-/dev/sev ++/dev/rtc, /dev/hpet, /dev/net/tun + + +

+Index: libvirt-5.0.0/src/qemu/qemu.conf +=================================================================== +--- libvirt-5.0.0.orig/src/qemu/qemu.conf ++++ libvirt-5.0.0/src/qemu/qemu.conf +@@ -484,7 +484,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: +Index: libvirt-5.0.0/src/qemu/qemu_cgroup.c +=================================================================== +--- libvirt-5.0.0.orig/src/qemu/qemu_cgroup.c ++++ libvirt-5.0.0/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 +Index: libvirt-5.0.0/src/qemu/test_libvirtd_qemu.aug.in +=================================================================== +--- libvirt-5.0.0.orig/src/qemu/test_libvirtd_qemu.aug.in ++++ libvirt-5.0.0/src/qemu/test_libvirtd_qemu.aug.in +@@ -62,7 +62,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" } diff --git a/libvirt.changes b/libvirt.changes index d5dd7b8..b2f66a3 100644 --- a/libvirt.changes +++ b/libvirt.changes @@ -1,3 +1,12 @@ +------------------------------------------------------------------- +Fri Feb 8 21:32:29 UTC 2019 - James Fehlig + +- qemu: fix issues related to restricted permissions on /dev/sev + b6440119-qemu-conf-sev.patch, a404ac34-qemu-cgroup-sev.patch, + 6fd4c8f8-qemu-domain-sev.patch, 17f6a257-security-dac-sev.patch, + a2d3dea9-qemu-caps-dac-override-sev.patch + bsc#1124842 + ------------------------------------------------------------------- Wed Jan 23 20:53:29 UTC 2019 - James Fehlig diff --git a/libvirt.spec b/libvirt.spec index afd0d16..18675c2 100644 --- a/libvirt.spec +++ b/libvirt.spec @@ -335,6 +335,11 @@ Source100: %{name}-rpmlintrc Patch0: 11c8aca9-libxl-set-mem-after-balloon.patch Patch1: 70c2933d-apparmor-named-profiles.patch Patch2: a3ab6d42-apparmor-conv-libvirtd-named-profile.patch +Patch3: b6440119-qemu-conf-sev.patch +Patch4: a404ac34-qemu-cgroup-sev.patch +Patch5: 6fd4c8f8-qemu-domain-sev.patch +Patch6: 17f6a257-security-dac-sev.patch +Patch7: a2d3dea9-qemu-caps-dac-override-sev.patch # Patches pending upstream review Patch100: libxl-dom-reset.patch Patch101: network-don-t-use-dhcp-authoritative-on-static-netwo.patch @@ -871,6 +876,11 @@ libvirt plugin for NSS for translating domain names into IP addresses. %patch0 -p1 %patch1 -p1 %patch2 -p1 +%patch3 -p1 +%patch4 -p1 +%patch5 -p1 +%patch6 -p1 +%patch7 -p1 %patch100 -p1 %patch101 -p1 %patch150 -p1