On Fri, Sep 16, 2022 at 11:38:49PM +0200, Moritz Buhl wrote:
> Syzkaller found a panic with sysv semaphores:
> https://syzkaller.appspot.com/bug?id=f7e8e2822779918d7a23d9ff9d7c0a3779c00a46
The last diff would introduced two memory leaks, seminfo.semopm is
the only seminfo value that can shrink via sysctl, so we have to
recheck it. semtot also can change during a sleep.
These problems are now addressed below.
I am still uncertain if anything relevant could change during a
sleep in copyout for GETALL.
The diff is growing in size, would it make sense to look at semctl
separately from semop and semget?
Except for the fuzzer, I am not aware of any code that makes extensive
use of the related syscalls. But they are cute.
mbuhl
Index: kern/sysv_sem.c
===================================================================
RCS file: /cvs/src/sys/kern/sysv_sem.c,v
retrieving revision 1.62
diff -u -p -r1.62 sysv_sem.c
--- kern/sysv_sem.c 16 Sep 2022 15:57:23 -0000 1.62
+++ kern/sysv_sem.c 17 Sep 2022 15:12:32 -0000
@@ -236,7 +236,7 @@ sys___semctl(struct proc *p, void *v, re
union semun arg, *uarg = SCARG(uap, arg);
struct semid_ds sbuf;
struct semid_ds *semaptr;
- unsigned short *semval = NULL;
+ unsigned short *semval = NULL, nsems;
int i, ix, error;
switch (cmd) {
@@ -248,6 +248,9 @@ sys___semctl(struct proc *p, void *v, re
if ((error = copyin(uarg, &arg, sizeof(union semun))))
return (error);
}
+ if (cmd == IPC_SET)
+ if ((error = copyin(arg.buf, &sbuf, sizeof(sbuf))))
+ return (error);
DPRINTF(("call to semctl(%d, %d, %d, %p)\n", semid, semnum, cmd, uarg));
@@ -255,6 +258,7 @@ sys___semctl(struct proc *p, void *v, re
if (ix < 0 || ix >= seminfo.semmni)
return (EINVAL);
+again:
if ((semaptr = sema[ix]) == NULL ||
semaptr->sem_perm.seq != IPCID_TO_SEQ(semid))
return (EINVAL);
@@ -277,8 +281,6 @@ sys___semctl(struct proc *p, void *v, re
case IPC_SET:
if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_M)))
return (error);
- if ((error = copyin(arg.buf, &sbuf, sizeof(sbuf))) != 0)
- return (error);
semaptr->sem_perm.uid = sbuf.sem_perm.uid;
semaptr->sem_perm.gid = sbuf.sem_perm.gid;
semaptr->sem_perm.mode = (semaptr->sem_perm.mode & ~0777) |
@@ -319,11 +321,22 @@ sys___semctl(struct proc *p, void *v, re
break;
case GETALL:
+ nsems = semaptr->sem_nsems;
+ semval = mallocarray(nsems, sizeof(arg.array[0]),
+ M_TEMP, M_WAITOK);
+ if (semaptr != sema[ix] ||
+ semaptr->sem_perm.seq != IPCID_TO_SEQ(semid) ||
+ semaptr->sem_nsems != nsems) {
+ free(semval, M_TEMP, nsems * sizeof(arg.array[0]));
+ goto again;
+ }
if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_R)))
- return (error);
- for (i = 0; i < semaptr->sem_nsems; i++) {
- error = copyout(&semaptr->sem_base[i].semval,
- &arg.array[i], sizeof(arg.array[0]));
+ goto error;
+ for (i = 0; i < nsems; i++)
+ semval[i] = semaptr->sem_base[i].semval;
+ for (i = 0; i < nsems; i++) {
+ error = copyout(&semval[i], &arg.array[i],
+ sizeof(arg.array[0]));
if (error != 0)
break;
}
@@ -350,11 +363,18 @@ sys___semctl(struct proc *p, void *v, re
break;
case SETALL:
- if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_W)))
- return (error);
+ nsems = semaptr->sem_nsems;
semval = mallocarray(semaptr->sem_nsems, sizeof(arg.array[0]),
M_TEMP, M_WAITOK);
- for (i = 0; i < semaptr->sem_nsems; i++) {
+ if (semaptr != sema[ix] ||
+ semaptr->sem_perm.seq != IPCID_TO_SEQ(semid) ||
+ semaptr->sem_nsems != nsems) {
+ free(semval, M_TEMP, nsems * sizeof(arg.array[0]));
+ goto again;
+ }
+ if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_W)))
+ goto error;
+ for (i = 0; i < nsems; i++) {
error = copyin(&arg.array[i], &semval[i],
sizeof(arg.array[0]));
if (error != 0)
@@ -364,7 +384,15 @@ sys___semctl(struct proc *p, void *v, re
goto error;
}
}
- for (i = 0; i < semaptr->sem_nsems; i++)
+ if (semaptr != sema[ix] ||
+ semaptr->sem_perm.seq != IPCID_TO_SEQ(semid) ||
+ semaptr->sem_nsems != nsems) {
+ free(semval, M_TEMP, nsems * sizeof(arg.array[0]));
+ goto again;
+ }
+ if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_W)))
+ goto error;
+ for (i = 0; i < nsems; i++)
semaptr->sem_base[i].semval = semval[i];
semundo_clear(ix, -1);
wakeup(&sema[ix]);
@@ -375,9 +403,7 @@ sys___semctl(struct proc *p, void *v, re
}
error:
- if (semval)
- free(semval, M_TEMP,
- semaptr->sem_nsems * sizeof(arg.array[0]));
+ free(semval, M_TEMP, nsems * sizeof(arg.array[0]));
return (error);
}
@@ -418,6 +444,10 @@ sys_semget(struct proc *p, void *v, regi
semaptr_new = pool_get(&sema_pool, PR_WAITOK | PR_ZERO);
semaptr_new->sem_base = mallocarray(nsems, sizeof(struct sem),
M_SEM, M_WAITOK|M_ZERO);
+ if (nsems > seminfo.semmns - semtot) {
+ error = ENOSPC;
+ goto error;
+ }
}
if (key != IPC_PRIVATE) {
@@ -518,15 +548,6 @@ sys_semop(struct proc *p, void *v, regis
if (semid < 0 || semid >= seminfo.semmni)
return (EINVAL);
- if ((semaptr = sema[semid]) == NULL ||
- semaptr->sem_perm.seq != IPCID_TO_SEQ(SCARG(uap, semid)))
- return (EINVAL);
-
- if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_W))) {
- DPRINTF(("error = %d from ipaccess\n", error));
- return (error);
- }
-
if (nsops == 0) {
*retval = 0;
return (0);
@@ -544,6 +565,22 @@ sys_semop(struct proc *p, void *v, regis
if (error != 0) {
DPRINTF(("error = %d from copyin(%p, %p, %u)\n", error,
SCARG(uap, sops), &sops, nsops * sizeof(struct sembuf)));
+ goto done2;
+ }
+
+ if (nsops > (size_t)seminfo.semopm) {
+ error = E2BIG;
+ goto done2;
+ }
+
+ if ((semaptr = sema[semid]) == NULL ||
+ semaptr->sem_perm.seq != IPCID_TO_SEQ(SCARG(uap, semid))) {
+ error = EINVAL;
+ goto done2;
+ }
+
+ if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_W))) {
+ DPRINTF(("error = %d from ipaccess\n", error));
goto done2;
}
