On Thu, Jan 31, 2019 at 04:26:18PM +0100, Erik Skultety wrote: > 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 > filesystem permisions are cross checked in kernel and therefore a user > with read permisions could issue a 'privileged' operation on SEV which > is currently only limited to root. > > Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx> > --- > src/qemu/qemu_capabilities.c | 11 +++++++++++ > src/util/virutil.c | 31 +++++++++++++++++++++++++++++-- > 2 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 5cf4b617c6..2e84c965e8 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -53,6 +53,10 @@ > #include <stdarg.h> > #include <sys/utsname.h> > > +#if WITH_CAPNG > +# include <cap-ng.h> > +#endif > + > #define VIR_FROM_THIS VIR_FROM_QEMU > > VIR_LOG_INIT("qemu.qemu_capabilities"); > @@ -4515,6 +4519,13 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, > 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); > > diff --git a/src/util/virutil.c b/src/util/virutil.c > index 5251b66454..02de92061c 100644 > --- a/src/util/virutil.c > +++ b/src/util/virutil.c > @@ -1502,8 +1502,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, > { > 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 gid, gid_t *groups, int ngroups, > */ > > 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 gid, gid_t *groups, int ngroups, > 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 This is set a bit earlier than I set it in my PoC patch, but I'll assume it still works given the comment you added. Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list