Syzkaller found a panic with sysv semaphores: https://syzkaller.appspot.com/bug?id=f7e8e2822779918d7a23d9ff9d7c0a3779c00a46
The problem is that the code uses a few globals (sema, semtot, seminfo) that can change during sleeping points where the kernel lock is released. Usually copyin, copyout, malloc and pool_get (with WAIT_OK). It is especially bad with sema, whihc is a list of semaphore pointers. The crash syzkaller found is likely this: Another process removes the current semaphore while the initial process is sleeping in one of the modification commands. Then the process wakes up and writes to the unassigned pool region and the next pool_get call will notice this and panic. To fix this, I move the sleeping points up and move the icpperm checks after the sleeping points. For the theoretical case, that the sleep of the modifying command takes longer than removing and reallocationg the semaphore, the permissions must be rechecked. And if the number of semaphores changed, we better start over too. To confirm that the attached diff fixes the crash, instructions on running syzkaller reproducers follow. pkg_add gmake go git git clone https://github.com/google/syzkaller/ cd syzkaller && gmake syzkaller_home=$PWD cd /tmp && ln -s $syzkaller_home/bin/openbsd_amd64/syz-* . mount -u -o ro / ./syz-execprog -repeat 1000 -procs 4 repro.syz where repro.syz is ftp -o repro.syz \ 'https://syzkaller.appspot.com/text?tag=ReproSyz&x=175f4ff2080000' kind regards, 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 16 Sep 2022 21:29:36 -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,20 @@ 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_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 +361,16 @@ 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_nsems != nsems) { + free(semval, M_TEMP, nsems * sizeof(arg.array[0])); + goto again; + } + if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_W))) + return (error); + for (i = 0; i < nsems; i++) { error = copyin(&arg.array[i], &semval[i], sizeof(arg.array[0])); if (error != 0) @@ -364,7 +380,13 @@ sys___semctl(struct proc *p, void *v, re goto error; } } - for (i = 0; i < semaptr->sem_nsems; i++) + if (semaptr != sema[ix] || semaptr->sem_nsems != nsems) { + free(semval, M_TEMP, nsems * sizeof(arg.array[0])); + goto again; + } + if ((error = ipcperm(cred, &semaptr->sem_perm, IPC_W))) + return (error); + for (i = 0; i < nsems; i++) semaptr->sem_base[i].semval = semval[i]; semundo_clear(ix, -1); wakeup(&sema[ix]); @@ -376,8 +398,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 +439,14 @@ 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.semmsl) { + error = EINVAL; + goto error; + } + if (nsems > seminfo.semmns - semtot) { + error = ENOSPC; + goto error; + } } if (key != IPC_PRIVATE) { @@ -518,15 +547,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 +564,17 @@ 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 ((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; }
