Differential D43134

inpcb: poison several inpcb pointer in in_pcbfree()
ClosedPublic
Actions

Authored by glebius on Dec 20 2023, 7:03 PM.

Details

Reviewers

markj

Group Reviewers

network

Commits

rG4a0c6403b0ec: inpcb: poison several inpcb pointer in in_pcbfree()

Summary

There are few subsystems that reference inpcb and allow it to outlive
in_pcbfree(). There are no known bugs with them to unreference the
socket and options pointers for a freed inpcb. Enforce this so that
such bugs don't appear in the future.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

glebius created this revision.Dec 20 2023, 7:03 PM

Herald added subscribers: melifaro, imp. · View Herald TranscriptDec 20 2023, 7:03 PM

glebius requested review of this revision.Dec 20 2023, 7:03 PM

glebius added a parent revision: D43122: inpcb: reoder inpcb destruction.

Harbormaster completed remote builds in B55048: Diff 131613.Dec 20 2023, 7:04 PM

Setting socket to non-NULL was too much of a change.

Harbormaster completed remote builds in B55049: Diff 131614.Dec 20 2023, 7:06 PM

markj added inline comments.Dec 22 2023, 4:04 PM

sys/netinet/in_pcb.c
1761	I think this should probably be unconditional? i.e., not dependent on whether INP_IPV6PROTO is set.

I'd prefer to leave it conditional if you don't mind. The field is IPv6 specific. In the past I tried to unionize IPv4 and IPv6 stuff in the inpcb. The blunt attempt failed, most likely due to 4to6 mapped connections. Maybe in future I'll make a smarter attempt. If that succeeds, then we should trash v6 pointers only for a INP_IPV6PROTO pcb.

In D43134#983981, @glebius wrote:

I'd prefer to leave it conditional if you don't mind. The field is IPv6 specific. In the past I tried to unionize IPv4 and IPv6 stuff in the inpcb. The blunt attempt failed, most likely due to 4to6 mapped connections. Maybe in future I'll make a smarter attempt. If that succeeds, then we should trash v6 pointers only for a INP_IPV6PROTO pcb.

I don't mind, but IMHO it is a strange argument: it is not much work to move the poisoning back later if needed.

This revision is now accepted and ready to land.Dec 27 2023, 3:18 PM

Closed by commit rG4a0c6403b0ec: inpcb: poison several inpcb pointer in in_pcbfree() (authored by glebius). · Explain WhyDec 27 2023, 4:36 PM

This revision was automatically updated to reflect the committed changes.

glebius added a commit: rG4a0c6403b0ec: inpcb: poison several inpcb pointer in in_pcbfree().