Move the building of -sandbox command line into a separate function. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- src/qemu/qemu_command.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 514c3ab2e..dfeba54ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9969,6 +9969,26 @@ qemuBuildCommandLineValidate(virQEMUDriverPtr driver, } +static int +qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virQEMUCapsPtr qemuCaps) +{ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) { + if (cfg->seccompSandbox == 0) + virCommandAddArgList(cmd, "-sandbox", "off", NULL); + else if (cfg->seccompSandbox > 0) + virCommandAddArgList(cmd, "-sandbox", "on", NULL); + } else if (cfg->seccompSandbox > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("QEMU does not support seccomp sandboxes")); + return -1; + } + return 0; + +} + + /* * Constructs a argv suitable for launching qemu with config defined * for a given virtual machine. @@ -10206,16 +10226,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, ? qemucmd->env_value[i] : ""); } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) { - if (cfg->seccompSandbox == 0) - virCommandAddArgList(cmd, "-sandbox", "off", NULL); - else if (cfg->seccompSandbox > 0) - virCommandAddArgList(cmd, "-sandbox", "on", NULL); - } else if (cfg->seccompSandbox > 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("QEMU does not support seccomp sandboxes")); + if (qemuBuildSeccompSandboxCommandLine(cmd, cfg, qemuCaps) < 0) goto error; - } if (qemuBuildPanicCommandLine(cmd, def, qemuCaps) < 0) goto error; -- 2.16.1

On Tue, Apr 10, 2018 at 04:49:40PM +0200, Ján Tomko wrote: Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; 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 :|

On 04/10/2018 10:49 AM, Ján Tomko wrote: Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John Could also use bool has_seccomp_cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX); ?? Then use - doesn't matter probably because the compiler will fix it.

If QEMU uses a seccomp blacklist (since 2.11), -sandbox on no longer tries to whitelist all the calls, but uses sets of blacklists: default (always blacklisted with -sandbox on) obsolete (defaults to deny) elevateprivileges (setuid & co, default: allow) spawn (fork & execve, default: allow) resourcecontrol (setaffinity, setscheduler, default: allow) If these are supported, default to sandbox with all four categories blacklisted. https://bugzilla.redhat.com/show_bug.cgi?id=1492597 Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- src/qemu/qemu.conf | 7 +++--- src/qemu/qemu_command.c | 10 +++++++++ tests/qemuxml2argvdata/minimal-sandbox.args | 29 ++++++++++++++++++++++++ tests/qemuxml2argvdata/minimal-sandbox.xml | 34 +++++++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 11 ++++++++++ 5 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.args create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.xml diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 07eab7eff..740129cf5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -669,9 +669,10 @@ -# Use seccomp syscall whitelisting in QEMU. -# 1 = on, 0 = off, -1 = use QEMU default -# Defaults to -1. +# Use seccomp syscall sandbox in QEMU. +# 1 = on, 0 = off, -1 = use the default +# For QEMUs using a whitelist, the default (-1) is off. +# For QEMUs using a blacklist, the default (-1) is on. # #seccomp_sandbox = 1 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ba279e640..fa5906d0b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9987,6 +9987,16 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; } + /* Use blacklist by default if supported */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) { + virCommandAddArgList(cmd, "-sandbox", + "on,obsolete=deny,elevateprivileges=deny," + "spawn=deny,resourcecontrol=deny", + NULL); + return 0; + } + + /* Seccomp whitelist is opt-in */ if (cfg->seccompSandbox > 0) virCommandAddArgList(cmd, "-sandbox", "on", NULL); diff --git a/tests/qemuxml2argvdata/minimal-sandbox.args b/tests/qemuxml2argvdata/minimal-sandbox.args new file mode 100644 index 000000000..c9d71fe8f --- /dev/null +++ b/tests/qemuxml2argvdata/minimal-sandbox.args @@ -0,0 +1,29 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny diff --git a/tests/qemuxml2argvdata/minimal-sandbox.xml b/tests/qemuxml2argvdata/minimal-sandbox.xml new file mode 100644 index 000000000..9ef92f8fe --- /dev/null +++ b/tests/qemuxml2argvdata/minimal-sandbox.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <title>A description of the test machine.</title> + <description> + A test of qemu&apos;s minimal configuration. + This test also tests the description and title elements. + </description> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9a22fe5f4..cf69135e8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -729,6 +729,17 @@ mymain(void) unsetenv("SDL_AUDIODRIVER"); DO_TEST("minimal", NONE); + DO_TEST("minimal-sandbox", + QEMU_CAPS_MACHINE_OPT, + QEMU_CAPS_MONITOR_JSON, + QEMU_CAPS_NO_USER_CONFIG, + QEMU_CAPS_RTC, + QEMU_CAPS_NO_SHUTDOWN, + QEMU_CAPS_DUMP_GUEST_CORE, + QEMU_CAPS_DISPLAY, + QEMU_CAPS_MACHINE_USB_OPT, + QEMU_CAPS_SECCOMP_SANDBOX, + QEMU_CAPS_SECCOMP_BLACKLIST); DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); DO_TEST("machine-aliases1", NONE); -- 2.16.1

On Fri, Apr 13, 2018 at 10:08:34AM -0400, John Ferlan wrote: Yes, setting this to '1' just means "enable use of seccomp". We explicitly never defined what kind of seccomp rules would be enabled - only that something seccomp related is on. Whether its a blacklist or a whitelist is a low level impl detail that we don't expect users to care about. Essentially the default (-1) means "do the best thing". On old QEMU the best thing was to disable it because it was horribly unreliable with a whitelist. On modern QEMU the best thing is to enable it because the blacklist is much saner 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 :|