tcp: improve MAC error handling for SYN segments
ClosedPublic
Actions

Authored by tuexen on Sep 23 2024, 8:46 PM.

Details

Reviewers

markj
glebius
rrs
rscheff
peter.lei_ieee.org
lstewart

Group Reviewers

transport

Commits

rGcdd0ab2374a0: tcp: improve MAC error handling for SYN segments
rG2f5ac48d9b6d: tcp: improve MAC error handling for SYN segments
rG78e1b031d2e8: tcp: improve MAC error handling for SYN segments

Summary

Don't leak a maclabel when SYN segments are processed which results in an error due to MD5 signature handling.
Tweak the #idef MAC to allow additional upcoming changes.
This patch depends on D46701 to avoid double frees.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

tuexen created this revision.Sep 23 2024, 8:46 PM

Herald added a reviewer: transport. · View Herald TranscriptSep 23 2024, 8:46 PM

Herald added subscribers: melifaro, imp. · View Herald Transcript

tuexen requested review of this revision.Sep 23 2024, 8:46 PM

cy added a subscriber: cy.Sep 24 2024, 7:00 AM

cy added inline comments.

sys/netinet/tcp_syncache.c
1766	I'm not sure if I'm missing something here but if this evaluates true when MAC is undefined it results in what is essentially a NOOP. Should this be moved to inside #ifdef MAC?
1770	ditto

tuexen marked 2 inline comments as done.Sep 24 2024, 7:46 AM

tuexen added inline comments.

sys/netinet/tcp_syncache.c
1766	You are right. But upcoming changes there will add two more statements in to block: Calling `crfree(cred)` if `cred != NULL`. Calling `m_free(ipopts) if` ipopts != NULL`. Both of these will not be protected by `#if MAC`. That is what I wanted to point out in the Summary by stating: "Tweak the `#idef MAC` to allow additional upcoming changes."

markj added inline comments.Sep 24 2024, 8:07 AM

sys/netinet/tcp_syncache.c
1766	The parentheses are redundant.
1766	I don't really see the value of submitting this as a separate patch. It's fine, but as Cy says, it's a no-op that could be included together with some functional change.

Remove parentheses, which are not necessary, as suggested by Mark.

tuexen marked 2 inline comments as done.Sep 24 2024, 8:41 AM

tuexen added inline comments.

sys/netinet/tcp_syncache.c
1766	Removed.
1766	I'm bundling moving the `#ifdef MAC` with fixing the maclabel leak. Please note that I'm changing the condition from `sc == &scs` to `sc == NULL \|\| sc == &scs`. If you prefer to not bundle the `#ifdef MAC` change with this commit, I can bundle it with the next one, where it is necessary. Whatever you prefer. The final code (after fixing all three leaks) will be the same.

markj accepted this revision.Sep 24 2024, 8:59 AM

markj added inline comments.

sys/netinet/tcp_syncache.c
1375	It's not necessary now, but I'd consider initializing this to NULL, to avoid any surprises if a new goto is introduced.
1766	I see now, the coffee is still kicking in.

Initialize maclabel to NULL, which is not need right now, but avoids using an uninitialized variable in case someone adds a goto in the future before mac_syncache_init() is called. This was suggested by Mark.

tuexen marked 2 inline comments as done.Sep 24 2024, 9:10 AM

tuexen added inline comments.