Re: [PATCH 01/12] ipc: reorganize initialization of kern_ipc_perm.id

[Davidlohr Bueso]

On Mon, 09 Jul 2018, Manfred Spraul wrote:


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

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

The patch moves all accesses to kern_ipc_perm.id under the spin_lock().


Yeah this is a lot better than my kzalloc() idea. I was considering
completions to avoid these races with ->getnew(), but careful init beats
that idea for obvious reasons.

The only thing I would say is that the title should be more descriptive.
Unlike the next patch, this one isn't really reorganizing anything.
How about:

 ipc: compute kern_ipc_perm.id under the ipc lock for stat cmds



The issues is related to the finding of
syzbot+2827ef6b3385deb07...@syzkaller.appspotmail.com:
syzbot found an issue with kern_ipc_perm.seq

Signed-off-by: Manfred Spraul 
Cc: Dmitry Vyukov 
Cc: Kees Cook 
Cc: Davidlohr Bueso 


Reviewed-by: Davidlohr Bueso 

[PATCH 01/12] ipc: reorganize initialization of kern_ipc_perm.id

[Manfred Spraul]

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

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

The patch moves all accesses to kern_ipc_perm.id under the spin_lock().

The issues is related to the finding of
syzbot+2827ef6b3385deb07...@syzkaller.appspotmail.com:
syzbot found an issue with kern_ipc_perm.seq

Signed-off-by: Manfred Spraul 
Cc: Dmitry Vyukov 
Cc: Kees Cook 
Cc: Davidlohr Bueso 
---
 ipc/msg.c | 19 ++-
 ipc/sem.c | 18 +-
 ipc/shm.c | 19 ++-
 3 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 3b6545302598..829c2062ded4 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -491,7 +491,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 +502,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 +546,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 5af1943ad782..e8971fa1d847 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1222,7 +1222,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));
@@ -1234,7 +1233,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)) {
@@ -1274,10 +1272,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();