Page MenuHomeFreeBSD

sctp: Tighten up locking around sctp_aloc_assoc()
ClosedPublic

Authored by markj on Sep 10 2021, 8:38 PM.
Tags
None
Referenced Files
F119996316: D31908.id.diff
Sat, Jun 14, 2:40 AM
Unknown Object (File)
Wed, May 28, 1:08 PM
Unknown Object (File)
Apr 8 2025, 11:15 PM
Unknown Object (File)
Mar 15 2025, 7:23 PM
Unknown Object (File)
Jan 27 2025, 10:28 AM
Unknown Object (File)
Jan 18 2025, 5:49 AM
Unknown Object (File)
Oct 20 2024, 5:50 PM
Unknown Object (File)
Oct 20 2024, 5:49 PM

Details

Summary

All callers of sctp_aloc_assoc() mark the PCB as connected after a
successful call (for one-to-one-style sockets). In all cases this is
done without the PCB lock, so the PCB's flags can be corrupted. We also
do not atomically check whether a one-to-one-style socket is a listening
socket, which violates various assumptions in solisten_proto().

We need to hold the PCB lock across all of sctp_aloc_assoc() to fix
this. In order to do that without introducing lock order reversals, we
have to hold the global info lock as well.

So:

- Convert sctp_aloc_assoc() so that the inp and info locks are
  consistently held.  It returns with the association lock held, as
  before.
- Fix an apparent bug where we failed to remove an association from a
  global hash if sctp_add_remote_addr() fails.
- sctp_select_a_tag() is called when initializing an association, and it
  acquires the global info lock.  To avoid lock recursion, push locking
  into its callers.
- Introduce sctp_aloc_assoc_connected(), which atomically checks for a
  listening socket and sets SCTP_PCB_FLAGS_CONNECTED.

There is still one edge case in sctp_process_cookie_new() where we do
not update PCB/socket state correctly. I'm not sure how best to fix
that yet, but it looks hard to trigger.

I believe this fixes quite a few of the outstanding syzbot reports.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 41467
Build 38356: arc lint + arc unit

Event Timeline

markj requested review of this revision.Sep 10 2021, 8:38 PM
tuexen added inline comments.
sys/netinet/sctp_pcb.c
4154

Putting the last two arguments in a line might be hard to maintain. The upstream code has multiple variants of the but-last argument.

This revision is now accepted and ready to land.Sep 10 2021, 9:07 PM
sys/netinet/sctp_pcb.c
4154

I can revert this bit, but should the new wrappers use the same formatting?

sys/netinet/sctp_pcb.c
4154

I guess so. But you can go ahead and commit your version. Once I have integrated it upstream, I can commit a whitespace change. I just wanted to make you aware that I might have to change some formatting back...