From 733e888993b71fb3c139f71de61534bc603a2bcb Mon Sep 17 00:00:00 2001 From: Manfred Spraul Date: Wed, 19 Dec 2018 09:26:48 +0100 Subject: [PATCH] ipc/sem.c: ensure proper locking during namespace teardown free_ipcs() only calls ipc_lock_object() before calling the free callback. This means: - There is no exclusion against parallel simple semop() calls. - sma->use_global_lock may underflow (i.e. jump to UNIT_MAX) when freeary() calls sem_unlock(,,-1). The patch fixes that, by adding complexmode_enter() before calling freeary(). There are multiple syzbot crashes in this code area, but I don't see yet how a missing complexmode_enter() may cause a crash: - 1) simple semop() calls are not used by these syzbox tests, and 2) we are in namespace teardown, noone may run in parallel. - 1) freeary() is the last call (except parallel operations, which are impossible due to namespace teardown) and 2) the underflow of use_global_lock merely delays switching to parallel simple semop handling for the next UINT_MAX semop() calls. Thus I think the patch is "only" a cleanup, and does not fix the observed crashes. Signed-off-by: Manfred Spraul Reported-by: syzbot+1145ec2e23165570c3ac@syzkaller.appspotmail.com Reported-by: syzbot+c92d3646e35bc5d1a909@syzkaller.appspotmail.com Reported-by: syzbot+9d8b6fa6ee7636f350c1@syzkaller.appspotmail.com Cc: dvyukov@google.com Cc: dbueso@suse.de Cc: Andrew Morton --- ipc/sem.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/ipc/sem.c b/ipc/sem.c index 745dc6187e84..8ccacd11fb15 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -184,6 +184,9 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it); */ #define USE_GLOBAL_LOCK_HYSTERESIS 10 +static void complexmode_enter(struct sem_array *sma); +static void complexmode_tryleave(struct sem_array *sma); + /* * Locking: * a) global sem_lock() for read/write @@ -232,9 +235,24 @@ void sem_init_ns(struct ipc_namespace *ns) } #ifdef CONFIG_IPC_NS + +static void freeary_lock(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) +{ + struct sem_array *sma = container_of(ipcp, struct sem_array, sem_perm); + + /* + * free_ipcs() isn't aware of sem_lock(), it calls ipc_lock_object() + * directly. In order to stay compatible with sem_lock(), we must + * upgrade from "simple" ipc_lock_object() to sem_lock(,,-1). + */ + complexmode_enter(sma); + + freeary(ns, ipcp); +} + void sem_exit_ns(struct ipc_namespace *ns) { - free_ipcs(ns, &sem_ids(ns), freeary); + free_ipcs(ns, &sem_ids(ns), freeary_lock); idr_destroy(&ns->ids[IPC_SEM_IDS].ipcs_idr); rhashtable_destroy(&ns->ids[IPC_SEM_IDS].key_ht); } @@ -374,7 +392,9 @@ static inline int sem_lock(struct sem_array *sma, struct sembuf *sops, /* Complex operation - acquire a full lock */ ipc_lock_object(&sma->sem_perm); - /* Prevent parallel simple ops */ + /* Prevent parallel simple ops. + * This must be identical to freeary_lock(). + */ complexmode_enter(sma); return SEM_GLOBAL_LOCK; } -- 2.17.2