KMSAN: uninit-value in do_msgrcv

KMSAN: uninit-value in do_msgrcv

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    8fc8ecd1c58a kmsan: unpoison regs in arch_uprobe_exception..
git tree:       https://github.com/google/kmsan.git/master
console output: https://syzkaller.appspot.com/x/log.txt?x=1481799f800000
kernel config:  https://syzkaller.appspot.com/x/.config?x=9fa436d3ae606638
dashboard link: https://syzkaller.appspot.com/bug?extid=2827ef6b3385deb07eaf
compiler:       clang version 7.0.0 (trunk 332596)
syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=1265edb7800000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16eeee9f800000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com

==================================================================
BUG: KMSAN: uninit-value in do_msgrcv+0x509/0x1e30 ipc/msg.c:1048
CPU: 0 PID: 4528 Comm: syz-executor852 Not tainted 4.17.0-rc5+ #103
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x185/0x1d0 lib/dump_stack.c:113
  kmsan_report+0x149/0x260 mm/kmsan/kmsan.c:1084
  __msan_warning_32+0x6e/0xc0 mm/kmsan/kmsan_instr.c:686
  do_msgrcv+0x509/0x1e30 ipc/msg.c:1048
  ksys_msgrcv ipc/msg.c:1184 [inline]
  __do_sys_msgrcv ipc/msg.c:1190 [inline]
  __se_sys_msgrcv ipc/msg.c:1187 [inline]
  __x64_sys_msgrcv+0x160/0x1b0 ipc/msg.c:1187
  do_syscall_64+0x152/0x230 arch/x86/entry/common.c:287
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x4459b9
RSP: 002b:00007f0d57662db8 EFLAGS: 00000297 ORIG_RAX: 0000000000000046
RAX: ffffffffffffffda RBX: 00000000006dac54 RCX: 00000000004459b9
RDX: 00000000000000d0 RSI: 0000000020000000 RDI: 0000000000260007
RBP: 00000000006dac50 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000297 R12: 0000000000000000
R13: 00007ffd5ab7e25f R14: 00007f0d576639c0 R15: 0000000000000006

Uninit was created at:
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:279 [inline]
  kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:189
  kmsan_kmalloc+0x94/0x100 mm/kmsan/kmsan.c:315
  __kmalloc_node+0xe25/0x11f0 mm/slub.c:3865
  kmalloc_node include/linux/slab.h:554 [inline]
  kvmalloc_node+0x197/0x2f0 mm/util.c:421
  kvmalloc include/linux/mm.h:550 [inline]
  newque+0xb4/0x7d0 ipc/msg.c:139
  ipcget_new ipc/util.c:315 [inline]
  ipcget+0x27b/0xd90 ipc/util.c:653
  ksys_msgget ipc/msg.c:289 [inline]
  __do_sys_msgget ipc/msg.c:294 [inline]
  __se_sys_msgget ipc/msg.c:292 [inline]
  __x64_sys_msgget+0x14c/0x1d0 ipc/msg.c:292
  do_syscall_64+0x152/0x230 arch/x86/entry/common.c:287
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
==================================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with  
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

ipc/msg: zalloc struct msg_queue when creating a new msq

[Davidlohr Bueso]

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.

==================================================================
BUG: KMSAN: uninit-value in do_msgrcv+0x509/0x1e30 ipc/msg.c:1048
CPU: 0 PID: 4528 Comm: syz-executor852 Not tainted 4.17.0-rc5+ #103
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x185/0x1d0 lib/dump_stack.c:113
  kmsan_report+0x149/0x260 mm/kmsan/kmsan.c:1084
  __msan_warning_32+0x6e/0xc0 mm/kmsan/kmsan_instr.c:686
  do_msgrcv+0x509/0x1e30 ipc/msg.c:1048
  ksys_msgrcv ipc/msg.c:1184 [inline]
  __do_sys_msgrcv ipc/msg.c:1190 [inline]
  __se_sys_msgrcv ipc/msg.c:1187 [inline]
  __x64_sys_msgrcv+0x160/0x1b0 ipc/msg.c:1187
  do_syscall_64+0x152/0x230 arch/x86/entry/common.c:287
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x4459b9
RSP: 002b:00007f0d57662db8 EFLAGS: 00000297 ORIG_RAX: 0000000000000046
RAX: ffffffffffffffda RBX: 00000000006dac54 RCX: 00000000004459b9
RDX: 00000000000000d0 RSI: 0000000020000000 RDI: 0000000000260007
RBP: 00000000006dac50 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000297 R12: 0000000000000000
R13: 00007ffd5ab7e25f R14: 00007f0d576639c0 R15: 0000000000000006

Uninit was created at:
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:279 [inline]
  kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:189
  kmsan_kmalloc+0x94/0x100 mm/kmsan/kmsan.c:315
  __kmalloc_node+0xe25/0x11f0 mm/slub.c:3865
  kmalloc_node include/linux/slab.h:554 [inline]
  kvmalloc_node+0x197/0x2f0 mm/util.c:421
  kvmalloc include/linux/mm.h:550 [inline]
  newque+0xb4/0x7d0 ipc/msg.c:139
  ipcget_new ipc/util.c:315 [inline]
  ipcget+0x27b/0xd90 ipc/util.c:653
  ksys_msgget ipc/msg.c:289 [inline]
  __do_sys_msgget ipc/msg.c:294 [inline]
  __se_sys_msgget ipc/msg.c:292 [inline]
  __x64_sys_msgget+0x14c/0x1d0 ipc/msg.c:292
  do_syscall_64+0x152/0x230 arch/x86/entry/common.c:287
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
==================================================================

Fix this by simply zero init the whole structure, something that
sysvsems also do; this is safe as it's a nop, having no secondary
effect afaict.

Reported-by: syzbot <syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com>
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 ipc/msg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 62545ce19173..da81b374f9fd 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -136,7 +136,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
 	key_t key = params->key;
 	int msgflg = params->flg;
 
-	msq = kvmalloc(sizeof(*msq), GFP_KERNEL);
+	msq = kvzalloc(sizeof(*msq), GFP_KERNEL);
 	if (unlikely(!msq))
 		return -ENOMEM;
 
-- 
2.16.4

Re: ipc/msg: zalloc struct msg_queue when creating a new msq

[Dmitry Vyukov]

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?



> ==================================================================
> BUG: KMSAN: uninit-value in do_msgrcv+0x509/0x1e30 ipc/msg.c:1048
> CPU: 0 PID: 4528 Comm: syz-executor852 Not tainted 4.17.0-rc5+ #103
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x185/0x1d0 lib/dump_stack.c:113
>  kmsan_report+0x149/0x260 mm/kmsan/kmsan.c:1084
>  __msan_warning_32+0x6e/0xc0 mm/kmsan/kmsan_instr.c:686
>  do_msgrcv+0x509/0x1e30 ipc/msg.c:1048
>  ksys_msgrcv ipc/msg.c:1184 [inline]
>  __do_sys_msgrcv ipc/msg.c:1190 [inline]
>  __se_sys_msgrcv ipc/msg.c:1187 [inline]
>  __x64_sys_msgrcv+0x160/0x1b0 ipc/msg.c:1187
>  do_syscall_64+0x152/0x230 arch/x86/entry/common.c:287
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x4459b9
> RSP: 002b:00007f0d57662db8 EFLAGS: 00000297 ORIG_RAX: 0000000000000046
> RAX: ffffffffffffffda RBX: 00000000006dac54 RCX: 00000000004459b9
> RDX: 00000000000000d0 RSI: 0000000020000000 RDI: 0000000000260007
> RBP: 00000000006dac50 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000297 R12: 0000000000000000
> R13: 00007ffd5ab7e25f R14: 00007f0d576639c0 R15: 0000000000000006
>
> Uninit was created at:
>  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:279 [inline]
>  kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:189
>  kmsan_kmalloc+0x94/0x100 mm/kmsan/kmsan.c:315
>  __kmalloc_node+0xe25/0x11f0 mm/slub.c:3865
>  kmalloc_node include/linux/slab.h:554 [inline]
>  kvmalloc_node+0x197/0x2f0 mm/util.c:421
>  kvmalloc include/linux/mm.h:550 [inline]
>  newque+0xb4/0x7d0 ipc/msg.c:139
>  ipcget_new ipc/util.c:315 [inline]
>  ipcget+0x27b/0xd90 ipc/util.c:653
>  ksys_msgget ipc/msg.c:289 [inline]
>  __do_sys_msgget ipc/msg.c:294 [inline]
>  __se_sys_msgget ipc/msg.c:292 [inline]
>  __x64_sys_msgget+0x14c/0x1d0 ipc/msg.c:292
>  do_syscall_64+0x152/0x230 arch/x86/entry/common.c:287
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ==================================================================
>
> Fix this by simply zero init the whole structure, something that
> sysvsems also do; this is safe as it's a nop, having no secondary
> effect afaict.
>
> Reported-by: syzbot <syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> ipc/msg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 62545ce19173..da81b374f9fd 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -136,7 +136,7 @@ static int newque(struct ipc_namespace *ns, struct
> ipc_params *params)
>         key_t key = params->key;
>         int msgflg = params->flg;
>
> -       msq = kvmalloc(sizeof(*msq), GFP_KERNEL);
> +       msq = kvzalloc(sizeof(*msq), GFP_KERNEL);
>         if (unlikely(!msq))
>                 return -ENOMEM;
>
> --
> 2.16.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/syzkaller-bugs/20180624025651.bvjlcfulbmycz5bf%40linux-r8p5.
> For more options, visit https://groups.google.com/d/optout.

Re: ipc/msg: zalloc struct msg_queue when creating a new msq

[Manfred Spraul]

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

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

[-- Attachment #2: 0001-ipc-initialize-kern_ipc_perm.id-earlier.patch --]
[-- Type: text/x-patch, Size: 6453 bytes --]

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

Re: ipc/msg: zalloc struct msg_queue when creating a new msq

[Dmitry Vyukov]

On Wed, Jul 4, 2018 at 11:18 AM, Manfred Spraul
<manfred@colorfullife.com> wrote:
> 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.

Hi Manfred,

It still looks fishy to me. This code published uninitialized uid's
for years (which lead not only to accidentally accessing wrong
objects, but also to privilege escalation). Now it publishes uninit
id/seq. The first proposed fix still did not make it correct. I can't
say that I see a but in your patch, but initializing id/seq in a racy
manner rings bells for me. Say, if we write/read seq ahead of id, can
reader still get access to a wrong object?
It all suggests some design flaw to me. Could ipc_idr_alloc() do full
initialization, i.e. also do what ipc_buildid() does? This would
ensure that we publish a fully constructed object in the first place.
We already have cleanup for ipc_idr_alloc(), which is idr_remove(), so
if we care about seq space conservation even in error conditions
(ENOMEM?), idr_remove() could accept an additional flag saying "this
object should not have been used by sane users yet, so retake its
seq". Did I get your concern about seq properly?

Thanks

p.s. I wonder how do you folks decipher unified patches without
context? If somebody will find it useful, side-by-side patch with
context is available here:
https://github.com/dvyukov/linux/commit/c3726a54a3c05b54192b2f5a4eaa4f7fa66ce68a

Re: ipc/msg: zalloc struct msg_queue when creating a new msq

[Manfred Spraul]

Hello Dmitry,
On 07/04/2018 12:03 PM, Dmitry Vyukov wrote:
> On Wed, Jul 4, 2018 at 11:18 AM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
>>
>> 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.
> Hi Manfred,
>
> It still looks fishy to me. This code published uninitialized uid's
> for years (which lead not only to accidentally accessing wrong
> objects, but also to privilege escalation). Now it publishes uninit
> id/seq. The first proposed fix still did not make it correct. I can't
> say that I see a but in your patch, but initializing id/seq in a racy
> manner rings bells for me. Say, if we write/read seq ahead of id, can
> reader still get access to a wrong object?
> It all suggests some design flaw to me. Could ipc_idr_alloc() do full
> initialization, i.e. also do what ipc_buildid() does? This would
> ensure that we publish a fully constructed object in the first place.
> We already have cleanup for ipc_idr_alloc(), which is idr_remove(), so
> if we care about seq space conservation even in error conditions
> (ENOMEM?), idr_remove() could accept an additional flag saying "this
> object should not have been used by sane users yet, so retake its
> seq". Did I get your concern about seq properly?
You have convinced me, I'll rewrite the patch:

1) kern_ipc_perm.seq should be accessible under rcu_read_lock(), this 
means replacing ipc_build_id() with two functions:
One that initializes kern_ipc_perm.seq, and one that would set 
kern_ipc_perm.id.
2) the accesses to kern_ipc_perm.id must be moved to the position where 
the lock is held. This is trivial.
3) we need a clear table that describes which variables can be accessed 
under rcu_read_lock() and which need ipc_lock_object().
   e.g.: kern_ipc_perm.id would end up under ipc_lock_object, 
kern_ipc_perm.seq or the xuid fields can be read under rcu_read_lock().
   Everything that can be accessed without ipc_lock_object must be 
initialized before publication of a new object.

Or, as all access to kern_ipc_perm.id are in rare codepaths:
I'll remove kern_ipc_perm.id entirely, and build the id on demand.

Ok?

--
     Manfred

Re: ipc/msg: zalloc struct msg_queue when creating a new msq

[Dmitry Vyukov]

On Wed, Jul 4, 2018 at 4:08 PM, Manfred Spraul <manfred@colorfullife.com> wrote:
> Hello Dmitry,
> On 07/04/2018 12:03 PM, Dmitry Vyukov wrote:
>>
>> On Wed, Jul 4, 2018 at 11:18 AM, Manfred Spraul
>> <manfred@colorfullife.com> wrote:
>>>
>>>
>>> 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.
>>
>> Hi Manfred,
>>
>> It still looks fishy to me. This code published uninitialized uid's
>> for years (which lead not only to accidentally accessing wrong
>> objects, but also to privilege escalation). Now it publishes uninit
>> id/seq. The first proposed fix still did not make it correct. I can't
>> say that I see a but in your patch, but initializing id/seq in a racy
>> manner rings bells for me. Say, if we write/read seq ahead of id, can
>> reader still get access to a wrong object?
>> It all suggests some design flaw to me. Could ipc_idr_alloc() do full
>> initialization, i.e. also do what ipc_buildid() does? This would
>> ensure that we publish a fully constructed object in the first place.
>> We already have cleanup for ipc_idr_alloc(), which is idr_remove(), so
>> if we care about seq space conservation even in error conditions
>> (ENOMEM?), idr_remove() could accept an additional flag saying "this
>> object should not have been used by sane users yet, so retake its
>> seq". Did I get your concern about seq properly?
>
> You have convinced me, I'll rewrite the patch:
>
> 1) kern_ipc_perm.seq should be accessible under rcu_read_lock(), this means
> replacing ipc_build_id() with two functions:
> One that initializes kern_ipc_perm.seq, and one that would set
> kern_ipc_perm.id.
> 2) the accesses to kern_ipc_perm.id must be moved to the position where the
> lock is held. This is trivial.
> 3) we need a clear table that describes which variables can be accessed
> under rcu_read_lock() and which need ipc_lock_object().
>   e.g.: kern_ipc_perm.id would end up under ipc_lock_object,
> kern_ipc_perm.seq or the xuid fields can be read under rcu_read_lock().
>   Everything that can be accessed without ipc_lock_object must be
> initialized before publication of a new object.
>
> Or, as all access to kern_ipc_perm.id are in rare codepaths:
> I'll remove kern_ipc_perm.id entirely, and build the id on demand.
>
> Ok?


Sounds like a more solid plan. If we remove kern_ipc_perm.id, then we
also don't need to split ipc_buildid() into two functions, right? And
since seq becomes constant throughout object lifetime, it probably
does not need any special access rules.

Thanks