Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: ipc/msg: zalloc struct msg_queue when creating a new msq
To:     Dmitry Vyukov <dvyukov@google.com>,
        Davidlohr Bueso <dave@stgolabs.net>
Cc:     syzbot <syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Kees Cook <keescook@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>, linux@dominikbrodowski.net,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Eric Dumazet <edumazet@google.com>
References: <000000000000e403b3056e76c786@google.com>
 <20180624025651.bvjlcfulbmycz5bf@linux-r8p5>
 <CACT4Y+anMTUCaRH5D4QoZR4bLdtH=_wtCxK2dM6isJozbVThDg@mail.gmail.com>
From:   Manfred Spraul <manfred@colorfullife.com>
Message-ID: <ff30f992-9c75-f230-c86a-902373f4ecaa@colorfullife.com>
Date:   Wed, 4 Jul 2018 11:18:44 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <CACT4Y+anMTUCaRH5D4QoZR4bLdtH=_wtCxK2dM6isJozbVThDg@mail.gmail.com>
Content-Type: multipart/mixed;
 boundary="------------E94E9557229FCA8A59998C09"
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

This is a multi-part message in MIME format.
--------------E94E9557229FCA8A59998C09
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

Hello together,

On 06/25/2018 11:21 AM, Dmitry Vyukov wrote:
> On Sun, Jun 24, 2018 at 4:56 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
>> The following splat was reported around the msg_queue structure
>> which can have uninitialized fields left over after newque().
>> Future syscalls which make use of the msq id (now valid) can thus
>> make KMSAN complain because not all fields are explicitly initialized
>> and we have the padding as well. This is internal to the kernel,
>> hence no bogus leaks.
> Hi Davidlohr,
>
> As far as I understand the root problem is that (1) we publish a
> not-fully initialized objects and (2) finish it's initialization in a
> racy manner when other threads already have access to it. As the
> result other threads can act on a wrong object. I am not sure that
> zeroing the object really solves these problems. It will sure get rid
> of the report at hand (but probably not of KTSAN, data race detector,
> report), other threads still can see wrong 0 id and the id is still
> initialized in racy way. I would expect that a proper fix would be to
> publish a fully initialized object with proper, final id. Am I missing
> something?
There are 2 relevant values: kern_ipc_perm.id and kern_ipc_perm.seq.

For kern_ipc_perm.id, it is possible to move the access to the codepath 
that hold the lock.

For kern_ipc_perm.seq, there are two options:
1) set it before publication.
2) initialize to an invalid value, and correct that at the end.

I'm in favor of option 2, it avoids that we must think about reducing 
the next sequence number or not:

The purpose of the sequence counter is to minimize the risk that e.g. a 
semop() will write into a newly created array.
I intentially write "minimize the risk", as it is by design impossible 
to guarantee that this cannot happen, e.g. if semop() sleeps at the 
instruction before the syscall.

Therefore, we can set seq to ULONG_MAX, then ipc_checkid() will always 
fail and the corruption is avoided.

What do you think?

And, obviously:
Just set seq to 0 is dangerous, as the first allocated sequence number 
is 0, and if that object is destroyed, then the newly created object has 
temporarily sequence number 0 as well.

--
     Manfred

--------------E94E9557229FCA8A59998C09
Content-Type: text/x-patch;
 name="0001-ipc-initialize-kern_ipc_perm.id-earlier.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-ipc-initialize-kern_ipc_perm.id-earlier.patch"

From 4791e604dcb618ed7ea1f42b2f6ca9cfe3c113c3 Mon Sep 17 00:00:00 2001
From: Manfred Spraul <manfred@colorfullife.com>
Date: Wed, 4 Jul 2018 10:04:49 +0200
Subject: [PATCH] ipc: fix races with kern_ipc_perm.id and .seq

ipc_addid() initializes kern_ipc_perm.id and kern_ipc_perm.seq after
having called ipc_idr_alloc().

Thus a parallel semop() or msgrcv() that uses ipc_obtain_object_idr()
may see an uninitialized value.

The simple solution cannot be used, as the correct id is only known
after ipc_idr_alloc().

Therefore:
- Initialize kern_ipc_perm.seq to an invalid value, so that
  ipc_checkid() is guaranteed to fail.
  This fulfills the purpose of the sequence counter: If e.g. semget() and
  semop() run in parallel, then the semop() should not write into the
  newly created array.
- Move the accesses to kern_ipc_perm.id into the code that is protected
  by kern_ipc_perm.lock.

The patch also fixes a use-after free that can be triggered by concurrent
semget() and semctl(IPC_RMID): reading kern_ipc_perm.id must happen
before dropping the locks.

Reported-by: syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/msg.c  | 23 +++++++++++++++++------
 ipc/sem.c  | 23 ++++++++++++++++-------
 ipc/shm.c  | 19 ++++++++++++++-----
 ipc/util.c |  8 +++++++-
 4 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 724000c15296..551c10be8d06 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -166,10 +166,12 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
 		return retval;
 	}
 
+	retval = msq->q_perm.id;
+
 	ipc_unlock_object(&msq->q_perm);
 	rcu_read_unlock();
 
-	return msq->q_perm.id;
+	return retval;
 }
 
 static inline bool msg_fits_inqueue(struct msg_queue *msq, size_t msgsz)
@@ -491,7 +493,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 			 int cmd, struct msqid64_ds *p)
 {
 	struct msg_queue *msq;
-	int id = 0;
 	int err;
 
 	memset(p, 0, sizeof(*p));
@@ -503,7 +504,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 			err = PTR_ERR(msq);
 			goto out_unlock;
 		}
-		id = msq->q_perm.id;
 	} else { /* IPC_STAT */
 		msq = msq_obtain_object_check(ns, msqid);
 		if (IS_ERR(msq)) {
@@ -548,10 +548,21 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 	p->msg_lspid  = pid_vnr(msq->q_lspid);
 	p->msg_lrpid  = pid_vnr(msq->q_lrpid);
 
-	ipc_unlock_object(&msq->q_perm);
-	rcu_read_unlock();
-	return id;
+	if (cmd == IPC_STAT) {
+		/*
+		 * As defined in SUS:
+		 * Return 0 on success
+		 */
+		err = 0;
+	} else {
+		/*
+		 * MSG_STAT and MSG_STAT_ANY (both Linux specific)
+		 * Return the full id, including the sequence counter
+		 */
+		err = msq->q_perm.id;
+	}
 
+	ipc_unlock_object(&msq->q_perm);
 out_unlock:
 	rcu_read_unlock();
 	return err;
diff --git a/ipc/sem.c b/ipc/sem.c
index c269fae05b24..42fcd0979a46 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -567,13 +567,14 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
 	}
 	ns->used_sems += nsems;
 
+	retval = sma->sem_perm.id;
+
 	sem_unlock(sma, -1);
 	rcu_read_unlock();
 
-	return sma->sem_perm.id;
+	return retval;
 }
 
-
 /*
  * Called with sem_ids.rwsem and ipcp locked.
  */
@@ -1228,7 +1229,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 {
 	struct sem_array *sma;
 	time64_t semotime;
-	int id = 0;
 	int err;
 
 	memset(semid64, 0, sizeof(*semid64));
@@ -1240,7 +1240,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 			err = PTR_ERR(sma);
 			goto out_unlock;
 		}
-		id = sma->sem_perm.id;
 	} else { /* IPC_STAT */
 		sma = sem_obtain_object_check(ns, semid);
 		if (IS_ERR(sma)) {
@@ -1280,10 +1279,20 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 #endif
 	semid64->sem_nsems = sma->sem_nsems;
 
+	if (cmd == IPC_STAT) {
+		/*
+		 * As defined in SUS:
+		 * Return 0 on success
+		 */
+		err = 0;
+	} else {
+		/*
+		 * SEM_STAT and SEM_STAT_ANY (both Linux specific)
+		 * Return the full id, including the sequence counter
+		 */
+		err = sma->sem_perm.id;
+	}
 	ipc_unlock_object(&sma->sem_perm);
-	rcu_read_unlock();
-	return id;
-
 out_unlock:
 	rcu_read_unlock();
 	return err;
diff --git a/ipc/shm.c b/ipc/shm.c
index 051a3e1fb8df..59fe8b3b3794 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -949,7 +949,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 			int cmd, struct shmid64_ds *tbuf)
 {
 	struct shmid_kernel *shp;
-	int id = 0;
 	int err;
 
 	memset(tbuf, 0, sizeof(*tbuf));
@@ -961,7 +960,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 			err = PTR_ERR(shp);
 			goto out_unlock;
 		}
-		id = shp->shm_perm.id;
 	} else { /* IPC_STAT */
 		shp = shm_obtain_object_check(ns, shmid);
 		if (IS_ERR(shp)) {
@@ -1011,10 +1009,21 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 	tbuf->shm_lpid	= pid_vnr(shp->shm_lprid);
 	tbuf->shm_nattch = shp->shm_nattch;
 
-	ipc_unlock_object(&shp->shm_perm);
-	rcu_read_unlock();
-	return id;
+	if (cmd == IPC_STAT) {
+		/*
+		 * As defined in SUS:
+		 * Return 0 on success
+		 */
+		err = 0;
+	} else {
+		/*
+		 * SHM_STAT and SHM_STAT_ANY (both Linux specific)
+		 * Return the full id, including the sequence counter
+		 */
+		err = shp->shm_perm.id;
+	}
 
+	ipc_unlock_object(&shp->shm_perm);
 out_unlock:
 	rcu_read_unlock();
 	return err;
diff --git a/ipc/util.c b/ipc/util.c
index ba7f96fc8a61..ba696e3e0574 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -22,7 +22,7 @@
  *          obtain the ipc object (kern_ipc_perm) by looking up the id in an idr
  *	    tree.
  *	    - perform initial checks (capabilities, auditing and permission,
- *	      etc).
+ *	      check the sequence counter, etc).
  *	    - perform read-only operations, such as INFO command, that
  *	      do not demand atomicity
  *	      acquire the ipc lock (kern_ipc_perm.lock) through
@@ -270,6 +270,12 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
 	new->cuid = new->uid = euid;
 	new->gid = new->cgid = egid;
 
+	/*
+	 * Initialize ->seq to ULONG_MAX, so that any early ipc_checkid()
+	 * calls are guarenteed to fail.
+	 */
+	new->seq = ULONG_MAX;
+
 	id = ipc_idr_alloc(ids, new);
 	idr_preload_end();
 
-- 
2.14.4


--------------E94E9557229FCA8A59998C09--