Page MenuHomeFreeBSD

Fix a pair of races in sigio handling
ClosedPublic

Authored by markj on Nov 10 2020, 12:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sep 23 2024, 11:09 PM
Unknown Object (File)
Sep 23 2024, 12:23 PM
Unknown Object (File)
Sep 23 2024, 3:55 AM
Unknown Object (File)
Sep 22 2024, 3:20 PM
Unknown Object (File)
Sep 16 2024, 9:57 PM
Unknown Object (File)
Sep 16 2024, 6:43 PM
Unknown Object (File)
Sep 13 2024, 6:26 PM
Unknown Object (File)
Sep 4 2024, 4:55 PM
Subscribers

Details

Summary

First, funsetownlst() list looks at the first element of the list to see
whether it's processing a process or a process group list. Then it
acquires the global sigio lock and processes the list. However, nothing
prevents the first sigio structure from being freed by a concurrent
funsetown() before the sigio lock is acquired.

Fix this by acquiring the global sigio lock immediately after checking
whether the list is empty. Callers of funsetownlst() ensure that new
sigios cannot be added concurrently.

Second, fsetown() uses funsetown() to remove an existing sigio structure
from a file object (e.g., a pipe or TTY). However, funsetown() uses a
racy check to avoid the sigio lock, so two threads may call fsetown() on
the same file object, observe that no sigio structure is present, and
enqueue two sigio structures for the same file object. However, if the
file object is destroyed, funsetown() will only remove one sigio
structure, and funsetownlst() may later trigger a use-after-free when it
clears *sigio->sio_myref for each sigio list entry.

Fix this by introducing funsetown_locked(), which avoids the racy check.

Test Plan

Peter reported panic on amd64 with !UMA_MD_SMALL_ALLOC in
funsetownlst() where sio_myref is dereferenced. I believe this is a
result of the second race described above.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj created this revision.
markj added a reviewer: kib.
markj added a subscriber: pho.
kib added inline comments.
sys/kern/kern_descrip.c
1101 ↗(On Diff #79364)

Why not save the pointer to the first element of the list (either pg_sigiolst, or p_sigiolst) to a local var, and then reinit it. Then you do not need relocking pgrp/proc, and perhaps even SIGIO as well.

This revision is now accepted and ready to land.Nov 10 2020, 9:47 PM
sys/kern/kern_descrip.c
1101 ↗(On Diff #79364)

I don't really follow. Reinit what, exactly?

Note, the lock order is SIGIO -> PROC/PGRP, so if the SIGIO lock is dropped, the others must be reacquired as well. Actually, I don't know why we drop the SIGIO lock in this loop. Maybe there's some other LOR. In any case, I think we could move sigios to a free list and dispose of them in a separate loop. Then there's no need to drop and reacquire any locks.

sys/kern/kern_descrip.c
1101 ↗(On Diff #79364)

Reinit list head. You called it 'move'.

This revision now requires review to proceed.Nov 10 2020, 11:28 PM
This revision is now accepted and ready to land.Nov 10 2020, 11:54 PM
This revision was automatically updated to reflect the committed changes.