sysvsem: Fix a TOCTOU race in semctl({GET,SET}ALL)

[markj]

Summary

These commands take a snapshot of the size of a semaphore set, then drop
the lock and malloc an appropriately sized array before reacquiring the
lock. A comment explains why this is (probably) safe. Unfortunately,
it's wrong; it is indeed possible for a malicious userspace to create
and destroy 2^{15} sets in the window where the lock is dropped. This
race can lead to out-of-bounds reads and writes, and that can be
exploited to elevate privileges.

Replace the assertions with runtime checks.

Reported by: Maik Muench of Secfault Security

[kib]

sys/kern/sysv_sem.c

858

Shouldn't we check semvalid() before reading the count, too?

862

Why EINVAL was chosen? I would say that more proper error is EAGAIN, or might be we should just free the array and retry in kernel.

As I understand it, the interface itself is weird: userspace is supposed to perform semctl(IPC_STAT) to get the semaphore set' size, then allocate the buffer of the right size and call into GETALL/SETALL. So EINVAL in this case indicates that the set was changed meantime, but EFAULT could mean it as well. Also userspace cannot detect that there were less data returned than requested (e.g. due to ABA for IPC_STAT nsems).

[markj]

Change errno value

[markj]

sys/kern/sysv_sem.c

858

Why? The result needs to be re-checked anyway.

862

I changed it to EAGAIN.

[kib]

sys/kern/sysv_sem.c

858

semvalid() tests for SEM_ALLOC

[markj]

Check semvalid() before dropping the lock.