Re: general protection fault in put_pid

Manfred Spraul
2018-12-19T09:04:38Z

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

Hello Dmitry,

On 12/12/18 11:55 AM, Dmitry Vyukov wrote:
> On Tue, Dec 11, 2018 at 9:23 PM syzbot
> <syzbot+1145ec2e23165570c3ac@syzkaller.appspotmail.com> wrote:
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit: f5d582777bcb Merge branch 'for-linus' of git://git.kernel...
>> git tree: upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=135bc547400000
>> kernel config: https://syzkaller.appspot.com/x/.config?x=c8970c89a0efbb23
>> dashboard link: https://syzkaller.appspot.com/bug?extid=1145ec2e23165570c3ac
>> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16803afb400000
> +Manfred, this looks similar to the other few crashes related to
> semget$private(0x0, 0x4000, 0x3f) that you looked at.

I found one unexpected (incorrect?) locking, see the attached patch.

But I doubt that this is the root cause of the crashes.

Any remarks on the patch?

I would continue to search, and then send a series with all findings.

--

Manfred

[-- Attachment #2: 0001-ipc-sem.c-ensure-proper-locking-during-namespace-tea.patch --]
[-- Type: text/x-patch, Size: 3137 bytes --]

From 733e888993b71fb3c139f71de61534bc603a2bcb Mon Sep 17 00:00:00 2001
From: Manfred Spraul <manfred@colorfullife.com>
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 <manfred@colorfullife.com>
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 <akpm@linux-foundation.org>
---
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