Improve handling of detached inps
AbandonedPublic
Actions

Authored by tuexen on Oct 5 2023, 9:04 AM.

Details

Reviewers

glebius
markj
rscheff

Summary

The inp returned by in_pcblookup_local() can be detached. So check if inp_socket is not NULL before dereferencing it.
This issue was reported by syzkaller and introduced in e3ba0d6adde3.

Test Plan

Run the reproducer found by syzkaller.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

tuexen created this revision.Oct 5 2023, 9:04 AM

Herald added subscribers: melifaro, imp. · View Herald TranscriptOct 5 2023, 9:04 AM

tuexen requested review of this revision.Oct 5 2023, 9:04 AM

Also fix IPv6.

Herald added a subscriber: ae. · View Herald TranscriptOct 5 2023, 9:07 AM

looks reasonable!

This revision is now accepted and ready to land.Oct 5 2023, 9:51 AM

markj added inline comments.Oct 5 2023, 1:58 PM

sys/netinet/in_pcb.c
957	We don't hold `t`'s lock here, so what prevents `inp_socket` from being set to NULL after this check?

tuexen added inline comments.Oct 5 2023, 2:41 PM

sys/netinet/in_pcb.c
957	Good point. @glebius: Should we take the inp lock here?

I missed one check. The locking issue is not resolved.

This revision now requires review to proceed.Oct 7 2023, 4:31 PM

Adding just an INP_RLOCK(t)/INP_UNLOCK(t) does not resolve the issue. First, it introduces an LOR, since we own the inp hash lock, second the system panics elsewhere when testing with reproducer. The current patch (with the missing locks) mitigates the issue found by syzkaller, but is conceptually not correct due to missing locks. Gleb?

tuexen edited the test plan for this revision. (Show Details)Oct 7 2023, 4:44 PM

tuexen abandoned this revision.Apr 19 2024, 4:56 PM