On 13/03/2019 - 13:22:13, Marc-André Lureau wrote: > Hi > > On Wed, Mar 13, 2019 at 10:49 AM Daniel P. Berrangé <berra...@redhat.com> > wrote: > > > > The Mesa library tries to set process affinity on some of its threads in > > order to optimize its performance. Currently this results in QEMU being > > immediately terminated when seccomp is enabled. > > > > Mesa doesn't consider failure of the process affinity settings to be > > fatal to its operation, but our seccomp policy gives it no choice in > > gracefully handling this denial. > > > > It is reasonable to consider that malicious code using the resource > > control syscalls to be a less serious attack than if they were trying > > to spawn processes or change UIDs and other such things. Generally > > speaking changing the resource control setting will "merely" affect > > quality of service of processes on the host. With this in mind, rather > > than kill the process, we can relax the policy for these syscalls to > > return the EPERM errno value. This allows callers to detect that QEMU > > does not want them to change resource allocations, and apply some > > reasonable fallback logic. > > > > The main downside to this is for code which uses these syscalls but does > > not check the return value, blindly assuming they will always > > succeeed. Returning an errno could result in sub-optimal behaviour. > > Arguably though such code is already broken & needs fixing regardless. > > > > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com> > > --- > > qemu-seccomp.c | 32 +++++++++++++++++++++++++------- > > 1 file changed, 25 insertions(+), 7 deletions(-) > > > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > > index 36d5829831..9776c9ef40 100644 > > --- a/qemu-seccomp.c > > +++ b/qemu-seccomp.c > > @@ -121,20 +121,37 @@ qemu_seccomp(unsigned int operation, unsigned int > > flags, void *args) > > #endif > > } > > > > -static uint32_t qemu_seccomp_get_kill_action(void) > > +static uint32_t qemu_seccomp_get_kill_action(int set) > > Minor nit, let's rename qemu_seccomp_get_kill_action() -> > qemu_seccomp_get_action()
I think that would be better too. And thanks for the contribution, Daniel. I can fix this when I send the pull request. Acked-by: Eduardo Otubo <ot...@redhat.com> > > > { > > + switch (set) { > > + case QEMU_SECCOMP_SET_DEFAULT: > > + case QEMU_SECCOMP_SET_OBSOLETE: > > + case QEMU_SECCOMP_SET_PRIVILEGED: > > + case QEMU_SECCOMP_SET_SPAWN: { > > #if defined(SECCOMP_GET_ACTION_AVAIL) && defined(SCMP_ACT_KILL_PROCESS) && > > \ > > defined(SECCOMP_RET_KILL_PROCESS) > > - { > > - uint32_t action = SECCOMP_RET_KILL_PROCESS; > > + static int kill_process = -1; > > + if (kill_process == -1) { > > + uint32_t action = SECCOMP_RET_KILL_PROCESS; > > > > - if (qemu_seccomp(SECCOMP_GET_ACTION_AVAIL, 0, &action) == 0) { > > + if (qemu_seccomp(SECCOMP_GET_ACTION_AVAIL, 0, &action) == 0) { > > + kill_process = 1; > > + } > > + kill_process = 0; > > + } > > + if (kill_process == 1) { > > return SCMP_ACT_KILL_PROCESS; > > } > > - } > > #endif > > + return SCMP_ACT_TRAP; > > + } > > + > > + case QEMU_SECCOMP_SET_RESOURCECTL: > > + return SCMP_ACT_ERRNO(EPERM); > > > > - return SCMP_ACT_TRAP; > > + default: > > + g_assert_not_reached(); > > + } > > } > > > > > > @@ -143,7 +160,6 @@ static int seccomp_start(uint32_t seccomp_opts) > > int rc = 0; > > unsigned int i = 0; > > scmp_filter_ctx ctx; > > - uint32_t action = qemu_seccomp_get_kill_action(); > > > > ctx = seccomp_init(SCMP_ACT_ALLOW); > > if (ctx == NULL) { > > @@ -157,10 +173,12 @@ static int seccomp_start(uint32_t seccomp_opts) > > } > > > > for (i = 0; i < ARRAY_SIZE(blacklist); i++) { > > + uint32_t action; > > if (!(seccomp_opts & blacklist[i].set)) { > > continue; > > } > > > > + action = qemu_seccomp_get_kill_action(blacklist[i].set); > > rc = seccomp_rule_add_array(ctx, action, blacklist[i].num, > > blacklist[i].narg, > > blacklist[i].arg_cmp); > > if (rc < 0) { > > -- > > 2.20.1 > > > > > -- > Marc-André Lureau -- Eduardo Otubo
signature.asc
Description: PGP signature