Re: [libvirt PATCH 05/23] qemu: remove use of the term 'blacklist' in seccomp capability

On Fri, Jun 19, 2020 at 01:56:55PM +0200, Ján Tomko wrote: > On a Friday in 2020, Daniel P. Berrangé wrote: > > The concept we're really testing for is whether QEMU supports > > the seccomp syscall filter groups. We need to keep one place > > using the old term to deal with upgrades from existing hosts > > with running VMs. > > > > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > > --- > > src/qemu/qemu.conf | 2 +- > > src/qemu/qemu_capabilities.c | 4 ++-- > > src/qemu/qemu_capabilities.h | 2 +- > > src/qemu/qemu_command.c | 4 ++-- > > src/qemu/qemu_domain.c | 10 +++++++--- > > tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 2 +- > > tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 2 +- > > tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 2 +- > > tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 +- > > tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml | 2 +- > > tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 2 +- > > tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 2 +- > > tests/qemustatusxml2xmldata/backup-pull-in.xml | 2 +- > > tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 2 +- > > tests/qemuxml2argvtest.c | 2 +- > > 37 files changed, 45 insertions(+), 41 deletions(-) > > > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > > index f89dbd2c3a..99b9ce53e5 100644 > > --- a/src/qemu/qemu.conf > > +++ b/src/qemu/qemu.conf > > @@ -704,7 +704,7 @@ > > # If it is unset (or -1), then seccomp will be enabled > > # only if QEMU >= 2.11.0 is detected, otherwise it is > > # left disabled. This ensures the default config gets > > -# protection for new QEMU using the blacklist approach. > > +# protection for new QEMU with filter groups. > > # > > #seccomp_sandbox = 1 > > > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > > index 68fcbd3c4f..310be800e2 100644 > > --- a/src/qemu/qemu_capabilities.c > > +++ b/src/qemu/qemu_capabilities.c > > @@ -468,7 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, > > /* 285 */ > > "qcow2-luks", > > "pcie-pci-bridge", > > - "seccomp-blacklist", > > + "seccomp-filter-groups", > > "query-cpus-fast", > > "disk-write-cache", > > > > @@ -3292,7 +3292,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { > > { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, > > { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, > > { "sandbox", "enable", QEMU_CAPS_SECCOMP_SANDBOX }, > > - { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, > > + { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_FILTER_GROUPS }, > > { "chardev", "fd", QEMU_CAPS_CHARDEV_FD_PASS }, > > { "overcommit", NULL, QEMU_CAPS_OVERCOMMIT }, > > { "smp-opts", "dies", QEMU_CAPS_SMP_DIES }, > > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > > index ad93816d41..0ee3e357cb 100644 > > --- a/src/qemu/qemu_capabilities.h > > +++ b/src/qemu/qemu_capabilities.h > > @@ -448,7 +448,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ > > /* 285 */ > > QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ > > QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ > > - QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */ > > + QEMU_CAPS_SECCOMP_FILTER_GROUPS, /* -sandbox.elevateprivileges */ > > QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */ > > QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */ > > > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > > index f27246b4c6..37113a433a 100644 > > --- a/src/qemu/qemu_command.c > > +++ b/src/qemu/qemu_command.c > > @@ -9517,8 +9517,8 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, > > return 0; > > } > > > > - /* Use blacklist by default if supported */ > > - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) { > > + /* Block undesirable syscall groups by default if supported */ > > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_FILTER_GROUPS)) { > > While 'filter groups' describes the underlying QEMU functionality > better, we only use it to deny syscalls. So using 'blocklist' as > proposed in the RFC you linked would better show the contrast between > this and the old approach. I don't want to name it based on libvirt's /current/ usage, as we could alter that usage in future, hence naming it based on the QEMU conceptual feature. > > virCommandAddArgList(cmd, "-sandbox", > > "on,obsolete=deny,elevateprivileges=deny," > > "spawn=deny,resourcecontrol=deny", > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > index 72874ee4fd..56ec5c0352 100644 > > --- a/src/qemu/qemu_domain.c > > +++ b/src/qemu/qemu_domain.c > > @@ -3851,9 +3851,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, > > if (str) { > > int flag = virQEMUCapsTypeFromString(str); > > if (flag < 0) { > > - virReportError(VIR_ERR_INTERNAL_ERROR, > > - _("Unknown qemu capabilities flag %s"), str); > > - goto error; > > + if (g_str_equal(str, "seccomp-blacklist")) { > > + flag = QEMU_CAPS_SECCOMP_FILTER_GROUPS; > > I'd just leave the XML as-is, to avoid introducing this special-casing. Renaming the capability lets us eliminate this from all the capabilities test data files we have (and the ones we cointinue to add in future), so I think it is a net win to just have this 2 line special case.