Skip to content

Commit

Permalink
ipc/sem: Fix dangling sem_array access in semtimedop race
Browse files Browse the repository at this point in the history
When __do_semtimedop() goes to sleep because it has to wait for a
semaphore value becoming zero or becoming bigger than some threshold, it
links the on-stack sem_queue to the sem_array, then goes to sleep
without holding a reference on the sem_array.

When __do_semtimedop() comes back out of sleep, one of two things must
happen:

 a) We prove that the on-stack sem_queue has been disconnected from the
    (possibly freed) sem_array, making it safe to return from the stack
    frame that the sem_queue exists in.

 b) We stabilize our reference to the sem_array, lock the sem_array, and
    detach the sem_queue from the sem_array ourselves.

sem_array has RCU lifetime, so for case (b), the reference can be
stabilized inside an RCU read-side critical section by locklessly
checking whether the sem_queue is still connected to the sem_array.

However, the current code does the lockless check on sem_queue before
starting an RCU read-side critical section, so the result of the
lockless check immediately becomes useless.

Fix it by doing rcu_read_lock() before the lockless check.  Now RCU
ensures that if we observe the object being on our queue, the object
can't be freed until rcu_read_unlock().

This bug is only hittable on kernel builds with full preemption support
(either CONFIG_PREEMPT or PREEMPT_DYNAMIC with preempt=full).

Fixes: 370b262 ("ipc/sem: avoid idr tree lookup for interrupted semop")
Cc: [email protected]
Signed-off-by: Jann Horn <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
  • Loading branch information
thejh authored and torvalds committed Dec 5, 2022
1 parent 76dcd73 commit b52be55
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion ipc/sem.c
Original file line number Diff line number Diff line change
Expand Up @@ -2179,14 +2179,15 @@ long __do_semtimedop(int semid, struct sembuf *sops,
* scenarios where we were awakened externally, during the
* window between wake_q_add() and wake_up_q().
*/
rcu_read_lock();
error = READ_ONCE(queue.status);
if (error != -EINTR) {
/* see SEM_BARRIER_2 for purpose/pairing */
smp_acquire__after_ctrl_dep();
rcu_read_unlock();
goto out;
}

rcu_read_lock();
locknum = sem_lock(sma, sops, nsops);

if (!ipc_valid_object(&sma->sem_perm))
Expand Down

0 comments on commit b52be55

Please sign in to comment.