Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency
On Sat, 02 Dec 2017, Philippe Mikoyan wrote: On Fri, 1 Dec 2017 09:20:07 -0800 Davidlohr Bueso 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.
Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency
On Fri, 1 Dec 2017 09:20:07 -0800 Davidlohr Bueso 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 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 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
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 id_ds inconsistency
when 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
On Thu, 30 Nov 2017, Philippe Mikoyan wrote:
As described in the title, this patch fixes id_ds inconsistency
when 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
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
[PATCH 2/2] ipc: Fix ipc data structures inconsistency
As described in the title, this patch fixes id_ds inconsistency
when 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
---
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;
+ re
