linux-kernel - [PATCH-next] ipc: Fix race condition in ipc_idr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 10 Mar 2019 21:25:36 -0400
From:   Waiman Long <longman@...hat.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Jonathan Corbet <corbet@....net>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-doc@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
        Matthew Wilcox <willy@...radead.org>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        Takashi Iwai <tiwai@...e.de>, Davidlohr Bueso <dbueso@...e.de>,
        Manfred Spraul <manfred@...orfullife.com>,
        Waiman Long <longman@...hat.com>
Subject: [PATCH-next] ipc: Fix race condition in ipc_idr_alloc()

In ipc_idr_alloc(), the sequence number of the kern_ipc_perm object was
also set before calling idr_alloc(). That gets changed recently in order
to conserve the sequence number space. That can lead to a possible race
condition where another thread of the same kern_ipc_perm may have called
ipc_obtain_object_check() concurrently with a recently deleted IPC id.
If idr_alloc() function happens to allocate the deleted index value,
that thread will incorrectly get a handle to the new IPC id.

However, we don't know if we should increment seq before the index value
is allocated and compared with the previously allocated index value. To
solve this dilemma, we will always put a new sequence number into the
kern_ipc_perm object before calling idr_alloc(). If it happens that the
sequence number don't need to be changed, we write back the right value
afterward. This will ensure that a concurrent ipc_obtain_object_check()
will not incorrectly match a deleted IPC id to to a new one.

Reported-by: Manfred Spraul <manfred@...orfullife.com>
Signed-off-by: Waiman Long <longman@...hat.com>
---
 ipc/util.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/ipc/util.c b/ipc/util.c
index 78e14acb51a7..6cb4129a2649 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -221,15 +221,34 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
 	 */
 
 	if (next_id < 0) { /* !CHECKPOINT_RESTORE or next_id is unset */
+		/*
+		 * It is possible that another thread of the same
+		 * kern_ipc_perm may have called ipc_obtain_object_check()
+		 * concurrently with a recently deleted IPC id (idx|seq).
+		 * If idr_alloc() happens to allocate this deleted idx value,
+		 * the other thread may incorrectly get a handle to the new
+		 * IPC id.
+		 *
+		 * To prevent this race condition from happening, we will
+		 * always store a new sequence number into the kern_ipc_perm
+		 * object before calling idr_alloc(). If we find out that we
+		 * don't need to change seq, we write back the right value.
+		 */
+		new->seq = ids->seq + 1;
+		if (new->seq > IPCID_SEQ_MAX)
+			new->seq = 0;
+
 		if (ipc_mni_extended)
 			idx = idr_alloc_cyclic(&ids->ipcs_idr, new, 0, ipc_mni,
 						GFP_NOWAIT);
 		else
 			idx = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
 
-		if ((idx <= ids->last_idx) && (++ids->seq > IPCID_SEQ_MAX))
-			ids->seq = 0;
-		new->seq = ids->seq;
+		/* Make ids->seq and new->seq stay in sync */
+		if (idx <= ids->last_idx)
+			ids->seq = new->seq;
+		else
+			new->seq = ids->seq;
 		ids->last_idx = idx;
 	} else {
 		new->seq = ipcid_to_seqx(next_id);
-- 
2.18.1