Page MenuHomeFreeBSD

netinet: Fix sockaddr type confusion in_pcb_lport_dest()
AbandonedPublic

Authored by markj on Mar 22 2023, 6:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 2, 11:48 PM
Unknown Object (File)
Dec 28 2023, 6:55 PM
Unknown Object (File)
Dec 20 2023, 7:52 AM
Unknown Object (File)
Dec 12 2023, 7:02 AM
Unknown Object (File)
Oct 1 2023, 7:13 AM
Unknown Object (File)
Sep 27 2023, 1:31 PM
Unknown Object (File)
Jul 25 2023, 12:44 PM
Unknown Object (File)
Jul 7 2023, 2:24 AM
Subscribers

Details

Reviewers
karels
glebius
tuexen
Group Reviewers
network
Summary

in_pcb_lport_dest() uses the inp vflag to determine whether the caller
passed in a v4 or a v6 address. This breaks in the face of v4-mapped
IPv6 addresses, wherein the caller will pass a v4 address. In
particular, the test
(inp->inp_vflag & (INP_IPV4 | INP_IPV6)) == INP_IPV4 fails in this case,
and we end up treating a sockaddr_in as a sockaddr_in6.

Instead, use the sockaddr family to determine what to do, assuming that
the caller has done any required checking (e.g., checking that
IN6P_IPV6_V6ONLY is unset). We can assume that the local and foreign
addresses are from the same family.

Reported by: syzbot+c8e3dac881bba85bc029@syzkaller.appspotmail.com
Reported by: syzbot+81ccc423a2737ed031ac@syzkaller.appspotmail.com

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 50528
Build 47419: arc lint + arc unit

Event Timeline

markj requested review of this revision.Mar 22 2023, 6:03 PM

I think this looks right as far as it goes.

sys/netinet/in_pcb.c
811

Do this test and the one at line 816 look correct still? Or should they test both vflag and the address family? I think the worst case is if laddr is used when it is INADDR_ANY but shouldn't be used.

bz added inline comments.
sys/netinet/in_pcb.c
711

Can you print fsa and lsa pointers and sa_families (if xsa is not NULL) in the message so they'll be on the console; so much easier to debug with users then.

markj added inline comments.
sys/netinet/in_pcb.c
811

Hmm, this is a bit tricky since we might not have an address family to check here, and the PCB doesn't tell us the address format it's using for its laddr and faddr.

I note that when binding or connecting to a v4-mapped v6 address, protocol layers (e.g., udp6_bind()) will clear INP_IPV6 before updating the inpcb. TCP will also do this for an implicit connect(), but UDP does not, which seems like a bug. Maybe that's the real bug here - I think this patch is not needed if protocols ensure that only one of INP_IPV4/V6 is set before calling this function.

sys/netinet/in_pcb.c
811

Hmm, that might be intentional after all since a UDP socket can be connected and disconnected multiple times in its life cycle. But I don't understand how that works in the face of in_pcbinshash() and in_pcbrehash(), which use the vflag to select whether to use a v4 or a v6 hash table. For v4-mapped addresses, don't we want to use the v4 hash table?

It seems like udp_send() should indeed clear INP_IPV6 when connecting to a v4-mapped address, and udp6_disconnect() should re-set INP_IPV6.

sys/netinet/in_pcb.c
811

I think it might be worth going back to FreeBSD 11/10 or so and see there how it once worked and where we changed from v6->v4 (sock)addr there. I have a suspicious feeling that this was half-done with a lot of updates but never looked at the full picture in a while.