[PATCH 0/2] ipc: Fix <ipc>ctl(..IPC_STAT..) bugs

[PATCH 0/2] ipc: Fix <ipc>ctl(..IPC_STAT..) bugs

[Philippe Mikoyan]

Hi,

Some applications that uses System V IPC mechanisms rely on data
structures that are returned by <ipc>ctl(..IPC_STAT..) system calls.

However, up to now information in these structures was not reliable,
due to following reasons:
1) Non-atomic data structures filling process, which, for obvious reasons
of not taking ipc lock, performed better - see:
commit ac0ba20ea6f2 ("ipc,msg: make msgctl_nolock lockless")
commit 16df3674efe3 ("ipc,sem: do not hold ipc lock more than necessary")
commit c97cb9ccab8c ("ipc,shm: make shmctl_nolock lockless")
2) [Refer only to shared memory] Because shm_nattch is used by kernel as
a ref_count, as a side effect it has to be increased twice in do_shmat
in order not to lose segment before mmap and shm_open.

These matters can lead, for example, to following unexpectable ipc data
structures values:
1) When there are concurrently running shmat and shmctl(... IPC_STAT ...):
{... shm_lpid = 0, shm_nattch = 1, ...}
2) If a shared memory segment was created just now and first process
attaches, another process, concurrently checking number of shmat-s via
shmctl(... IPC_STAT ...), can, at some point of time, get the following
result:
{... shm_nattch = 2, ...}

Bug reproducing code can be found here:
https://github.com/Phikimon/sysipc_break
To catch bug you have to execute and kill bug reproducing program several
times(one to five times, I guess).

In this patchset I make an attempt to fix these bugs:
1) By taking locks before filling data structure fields. Note that lock
is taken after permissions and security checks in order to increase
performance.
2) By filling certain data structure fields directly in do_shmat in order
to fill shm_nattch and other fields atomically. shm_open call is removed
from shm_mmap.

Philippe Mikoyan (2):
  ipc/shm: Fix shm_nattch incorrect value
  ipc: Fix ipc data structures inconsistency

 ipc/msg.c  | 20 +++++++++++-----
 ipc/sem.c  | 10 ++++++++
 ipc/shm.c  | 77 +++++++++++++++++++++++++++++++++-----------------------------
 ipc/util.c |  5 +++-
 4 files changed, 69 insertions(+), 43 deletions(-)

--
2.11.0

[PATCH 1/2] ipc/shm: Fix shm_nattch incorrect value

[Philippe Mikoyan]

This patch fixes that do_shmat increases shm_nattch value twice.

E.g. if memory segment was created just now and process attaches it,
shmctl(..IPC_STAT..) of concurrently running process can at some
point of time return data structure with 'shm_nattch' equal to 2.

Signed-off-by: Philippe Mikoyan <philippe.mikoyan@skat.systems>
Signed-off-by: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com>
---
 ipc/shm.c | 58 +++++++++++++++++++++++++++-------------------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index badac463e2c8..565f17925128 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -190,33 +190,31 @@ static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s)
 	ipc_rmid(&shm_ids(ns), &s->shm_perm);
 }

-
-static int __shm_open(struct vm_area_struct *vma)
-{
-	struct file *file = vma->vm_file;
-	struct shm_file_data *sfd = shm_file_data(file);
-	struct shmid_kernel *shp;
-
-	shp = shm_lock(sfd->ns, sfd->id);
-
-	if (IS_ERR(shp))
-		return PTR_ERR(shp);
-
-	shp->shm_atim = ktime_get_real_seconds();
-	shp->shm_lprid = task_tgid_vnr(current);
-	shp->shm_nattch++;
-	shm_unlock(shp);
-	return 0;
-}
-
 /* This is called by fork, once for every shm attach. */
 static void shm_open(struct vm_area_struct *vma)
 {
-	int err = __shm_open(vma);
+	struct file *file = vma->vm_file;
+	struct shm_file_data *sfd = shm_file_data(file);
+	struct shmid_kernel *shp;
+	int err = 0;
+
+	shp = shm_lock(sfd->ns, sfd->id);
+
+	if (IS_ERR(shp)) {
+		err = PTR_ERR(shp);
+		goto warn;
+	}
+
+	shp->shm_atim = ktime_get_real_seconds();
+	shp->shm_lprid = task_tgid_vnr(current);
+	shp->shm_nattch++;
+	shm_unlock(shp);
+
 	/*
 	 * We raced in the idr lookup or with shm_destroy().
 	 * Either way, the ID is busted.
 	 */
+warn:
 	WARN_ON_ONCE(err);
 }

@@ -418,19 +416,10 @@ static int shm_mmap(struct file *file, struct vm_area_struct *vma)
 	struct shm_file_data *sfd = shm_file_data(file);
 	int ret;

-	/*
-	 * In case of remap_file_pages() emulation, the file can represent
-	 * removed IPC ID: propogate shm_lock() error to caller.
-	 */
-	ret = __shm_open(vma);
-	if (ret)
-		return ret;
-
 	ret = call_mmap(sfd->file, vma);
-	if (ret) {
-		shm_close(vma);
+	if (ret)
 		return ret;
-	}
+
 	sfd->vm_ops = vma->vm_ops;
 #ifdef CONFIG_MMU
 	WARN_ON(!sfd->vm_ops->fault);
@@ -944,6 +933,7 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 	tbuf->shm_cpid	= shp->shm_cprid;
 	tbuf->shm_lpid	= shp->shm_lprid;
 	tbuf->shm_nattch = shp->shm_nattch;
+
 	rcu_read_unlock();
 	return result;

@@ -1351,7 +1341,11 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,

 	path = shp->shm_file->f_path;
 	path_get(&path);
+
+	shp->shm_atim = ktime_get_real_seconds();
+	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_nattch++;
+
 	size = i_size_read(d_inode(path.dentry));
 	ipc_unlock_object(&shp->shm_perm);
 	rcu_read_unlock();
@@ -1411,6 +1405,8 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,

 out_fput:
 	fput(file);
+	if (!err)
+		goto out;

 out_nattch:
 	down_write(&shm_ids(ns).rwsem);
--
2.11.0

[PATCH 2/2] ipc: Fix ipc data structures inconsistency

[Philippe Mikoyan]

As described in the title, this patch fixes <ipc>id_ds inconsistency
when <ipc>ctl_stat runs concurrently with some ds-changing function,
e.g. shmat, msgsnd or whatever.

For instance, if shmctl(IPC_STAT) is running concurrently with shmat,
following data structure can be returned:
{... shm_lpid = 0, shm_nattch = 1, ...}

Signed-off-by: Philippe Mikoyan <philippe.mikoyan@skat.systems>
---
 ipc/msg.c  | 20 ++++++++++++++------
 ipc/sem.c  | 10 ++++++++++
 ipc/shm.c  | 19 ++++++++++++++-----
 ipc/util.c |  5 ++++-
 4 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 06be5a9adfa4..047579b42de4 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -475,9 +475,9 @@ static int msgctl_info(struct ipc_namespace *ns, int msqid,
 static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 			 int cmd, struct msqid64_ds *p)
 {
-	int err;
 	struct msg_queue *msq;
-	int success_return;
+	int id = 0;
+	int err;

 	memset(p, 0, sizeof(*p));

@@ -488,14 +488,13 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 			err = PTR_ERR(msq);
 			goto out_unlock;
 		}
-		success_return = msq->q_perm.id;
+		id = msq->q_perm.id;
 	} else {
 		msq = msq_obtain_object_check(ns, msqid);
 		if (IS_ERR(msq)) {
 			err = PTR_ERR(msq);
 			goto out_unlock;
 		}
-		success_return = 0;
 	}

 	err = -EACCES;
@@ -506,6 +505,14 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 	if (err)
 		goto out_unlock;

+	ipc_lock_object(&msq->q_perm);
+
+	if (!ipc_valid_object(&msq->q_perm)) {
+		ipc_unlock_object(&msq->q_perm);
+		err = -EIDRM;
+		goto out_unlock;
+	}
+
 	kernel_to_ipc64_perm(&msq->q_perm, &p->msg_perm);
 	p->msg_stime  = msq->q_stime;
 	p->msg_rtime  = msq->q_rtime;
@@ -515,9 +522,10 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 	p->msg_qbytes = msq->q_qbytes;
 	p->msg_lspid  = msq->q_lspid;
 	p->msg_lrpid  = msq->q_lrpid;
-	rcu_read_unlock();

-	return success_return;
+	ipc_unlock_object(&msq->q_perm);
+	rcu_read_unlock();
+	return id;

 out_unlock:
 	rcu_read_unlock();
diff --git a/ipc/sem.c b/ipc/sem.c
index f7385bce5fd3..9b6f80d1b3f1 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1211,10 +1211,20 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 	if (err)
 		goto out_unlock;

+	ipc_lock_object(&sma->sem_perm);
+
+	if (!ipc_valid_object(&sma->sem_perm)) {
+		ipc_unlock_object(&sma->sem_perm);
+		err = -EIDRM;
+		goto out_unlock;
+	}
+
 	kernel_to_ipc64_perm(&sma->sem_perm, &semid64->sem_perm);
 	semid64->sem_otime = get_semotime(sma);
 	semid64->sem_ctime = sma->sem_ctime;
 	semid64->sem_nsems = sma->sem_nsems;
+
+	ipc_unlock_object(&sma->sem_perm);
 	rcu_read_unlock();
 	return id;

diff --git a/ipc/shm.c b/ipc/shm.c
index 565f17925128..8f58faba7429 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -896,9 +896,11 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 			int cmd, struct shmid64_ds *tbuf)
 {
 	struct shmid_kernel *shp;
-	int result;
+	int id = 0;
 	int err;

+	memset(tbuf, 0, sizeof(*tbuf));
+
 	rcu_read_lock();
 	if (cmd == SHM_STAT) {
 		shp = shm_obtain_object(ns, shmid);
@@ -906,14 +908,13 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 			err = PTR_ERR(shp);
 			goto out_unlock;
 		}
-		result = shp->shm_perm.id;
+		id = shp->shm_perm.id;
 	} else {
 		shp = shm_obtain_object_check(ns, shmid);
 		if (IS_ERR(shp)) {
 			err = PTR_ERR(shp);
 			goto out_unlock;
 		}
-		result = 0;
 	}

 	err = -EACCES;
@@ -924,7 +925,14 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 	if (err)
 		goto out_unlock;

-	memset(tbuf, 0, sizeof(*tbuf));
+	ipc_lock_object(&shp->shm_perm);
+
+	if (!ipc_valid_object(&shp->shm_perm)) {
+		ipc_unlock_object(&shp->shm_perm);
+		err = -EIDRM;
+		goto out_unlock;
+	}
+
 	kernel_to_ipc64_perm(&shp->shm_perm, &tbuf->shm_perm);
 	tbuf->shm_segsz	= shp->shm_segsz;
 	tbuf->shm_atime	= shp->shm_atim;
@@ -934,8 +942,9 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 	tbuf->shm_lpid	= shp->shm_lprid;
 	tbuf->shm_nattch = shp->shm_nattch;

+	ipc_unlock_object(&shp->shm_perm);
 	rcu_read_unlock();
-	return result;
+	return id;

 out_unlock:
 	rcu_read_unlock();
diff --git a/ipc/util.c b/ipc/util.c
index 78755873cc5b..8d3c3946c825 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -22,9 +22,12 @@
  *	    tree.
  *	    - perform initial checks (capabilities, auditing and permission,
  *	      etc).
- *	    - perform read-only operations, such as STAT, INFO commands.
+ *	    - perform read-only operations, such as INFO command, that
+ *	      do not demand atomicity
  *	      acquire the ipc lock (kern_ipc_perm.lock) through
  *	      ipc_lock_object()
+ *		- perform read-only operatoins that demand atomicity,
+ *		  such as STAT command.
  *		- perform data updates, such as SET, RMID commands and
  *		  mechanism-specific operations (semop/semtimedop,
  *		  msgsnd/msgrcv, shmat/shmdt).
--
2.11.0

Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency

[Davidlohr Bueso]

On Thu, 30 Nov 2017, Philippe Mikoyan wrote:

>As described in the title, this patch fixes <ipc>id_ds inconsistency
>when <ipc>ctl_stat runs concurrently with some ds-changing function,
>e.g. shmat, msgsnd or whatever.
>
>For instance, if shmctl(IPC_STAT) is running concurrently with shmat,
>following data structure can be returned:
>{... shm_lpid = 0, shm_nattch = 1, ...}

Hmm yeah that's pretty fishy, also shm_atime = 0, no?

So I think this patch is fine as we can obviously race at a user level.
This is another justification for converting the ipc lock to rwlock;
performance wise they are the pretty much the same (being queued)...
but that's irrelevant to this patch. I like that you manage to do
security and such checks still only under rcu, like all ipc calls
work; *_stat() is no longer special.

With a nit below:

Reviewed-by: Davidlohr Bueso <dbueso@suse.de>

>diff --git a/ipc/util.c b/ipc/util.c
>index 78755873cc5b..8d3c3946c825 100644
>--- a/ipc/util.c
>+++ b/ipc/util.c
>@@ -22,9 +22,12 @@
>  *	    tree.
>  *	    - perform initial checks (capabilities, auditing and permission,
>  *	      etc).
>- *	    - perform read-only operations, such as STAT, INFO commands.
>+ *	    - perform read-only operations, such as INFO command, that
>+ *	      do not demand atomicity
>  *	      acquire the ipc lock (kern_ipc_perm.lock) through
>  *	      ipc_lock_object()
>+ *		- perform read-only operatoins that demand atomicity,
                                          ^^ typo
>+ *		  such as STAT command.
>  *		- perform data updates, such as SET, RMID commands and
>  *		  mechanism-specific operations (semop/semtimedop,
>  *		  msgsnd/msgrcv, shmat/shmdt).

Thanks,
Davidlohr

Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency

[Manfred Spraul]

Hi,

On 12/01/2017 06:20 PM, Davidlohr Bueso wrote:
> On Thu, 30 Nov 2017, Philippe Mikoyan wrote:
>
>> As described in the title, this patch fixes <ipc>id_ds inconsistency
>> when <ipc>ctl_stat runs concurrently with some ds-changing function,
>> e.g. shmat, msgsnd or whatever.
>>
>> For instance, if shmctl(IPC_STAT) is running concurrently with shmat,
>> following data structure can be returned:
>> {... shm_lpid = 0, shm_nattch = 1, ...}
>
The patch appears to be good. I'll try to perform some tests, but I'm 
not sure when I will be able to.
Especially: I don't know the shm code good enough to immediately check 
the change you make to nattach.

And, perhaps as a side information:
There appears to be a use-after-free in shm, I now got a 2nd mail from 
syzbot:
http://lkml.iu.edu/hypermail/linux/kernel/1702.3/02480.html


> Hmm yeah that's pretty fishy, also shm_atime = 0, no?
>
> So I think this patch is fine as we can obviously race at a user level.
> This is another justification for converting the ipc lock to rwlock;
> performance wise they are the pretty much the same (being queued)...
> but that's irrelevant to this patch. I like that you manage to do
> security and such checks still only under rcu, like all ipc calls
> work; *_stat() is no longer special.
>
I don't like rwlock, they add complexity without reducing the cache line 
pressure.

What I would like to try is to create a mutex_lock_rcu() function, and 
then convert everything to a mutex.

As pseudocode::
     rcu_lock();
     idr_lookup();
     mutex_trylock();
     if (failed) {
         getref();
         rcu_unlock();
         mutex_lock();
         putref();
     } else {
         rcu_unlock();
     }

Obviously, the getref then within the mutex framework, i.e. only if 
mutex_lock() really sleeps.
If the code in ipc gets significantly simpler, then perhaps convert it 
to an rw mutex.

Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency

[Philippe Mikoyan]

On Fri, 1 Dec 2017 09:20:07 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> 
> Hmm yeah that's pretty fishy, also shm_atime = 0, no?
> 

Yeah, definitely, other data structure fields can also be
inconsistent, and applying not only to shmem, but also to 
other ipc mechanisms.

Thank you for noting the typo, 'll send fixed version in next
message(without another patch, see below).

On Sat, 2 Dec 2017 07:03:30 +0100
Manfred Spraul <manfred@colorfullife.com> wrote:

> Especially: I don't know the shm code good enough to immediately
> check the change you make to nattach.

It seems that I didn't know the shm code good enough too: I've
recently discovered that 
[PATCH 1/2] ipc/shm: Fix shm_nattch incorrect value
is, frankly speaking, clearly total crap as it 
1) doesn't handle that shmem segment can be already RMID-ed
when entering shm_mmap, when called from 'remap_file_pages'
2) doesn't support (broken) logic of detaching remapped via
'remap_file_pages' shmem segment.

Regardless of handling (deprecated) 'remap_file_pages' call, patch
shall be OK. However, it has to be made over.

Sorry about that, hope I will find at least halfway elegant 
solution and send it ASAP.

On Sat, 2 Dec 2017 07:03:30 +0100
Manfred Spraul <manfred@colorfullife.com> wrote:

> 
> And, perhaps as a side information:
> There appears to be a use-after-free in shm, I now got a 2nd mail
> from syzbot:
> http://lkml.iu.edu/hypermail/linux/kernel/1702.3/02480.html
> 

Will dig into.


Thanks,
Phil

Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency

[Davidlohr Bueso]

On Sat, 02 Dec 2017, Philippe Mikoyan wrote:

>On Fri, 1 Dec 2017 09:20:07 -0800
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>>
>> Hmm yeah that's pretty fishy, also shm_atime = 0, no?
>>
>
>Yeah, definitely, other data structure fields can also be
>inconsistent, and applying not only to shmem, but also to
>other ipc mechanisms.

Right.

>
>Thank you for noting the typo, 'll send fixed version in next
>message(without another patch, see below).

Ok, I had yet to look at that patch.