irdma: improve TCP header validation
Needs ReviewPublic
Actions

Authored by rscheff on Sat, Jul 18, 1:29 PM.

Details

Reviewers

tuexen
markj
kgalazka

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
transport

Summary

iWARP handling code deals with TCP header processing outside of the normal TCP stack.
Certain validation checks to verify the internal integrity of the TCP header were missing.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 74913
Build 71796: arc lint + arc unit

Event Timeline

rscheff created this revision.Sat, Jul 18, 1:29 PM

Owners added a reviewer: Restricted Owners Package.Sat, Jul 18, 1:29 PM

Herald added a reviewer: transport. · View Herald TranscriptSat, Jul 18, 1:29 PM

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

Harbormaster completed remote builds in B74912: Diff 182175.Sat, Jul 18, 1:29 PM

rscheff requested review of this revision.Sat, Jul 18, 1:29 PM

updating phabricator to show proper diff

Harbormaster completed remote builds in B74913: Diff 182176.Sat, Jul 18, 1:34 PM

tuexen accepted this revision.Sat, Jul 18, 1:34 PM

To whom it may concern: the same problem seems to exist in the Linux source code: cm.c.

markj added inline comments.Mon, Jul 20, 1:26 PM

sys/dev/irdma/irdma_cm.c
2679	Indentation here looks wrong, same below.
sys/dev/irdma/irdma_utils.c
1544	Why only check this here, and not in the !gen1 case in irdma_puda_get_tcpip_info()? If we're validating the offset here, why do it again in the functions above?

add a tcp_hlen check oversight and indentation

Harbormaster completed remote builds in B74955: Diff 182292.Mon, Jul 20, 3:21 PM

rscheff marked 2 inline comments as done.Mon, Jul 20, 3:22 PM

rscheff added inline comments.

sys/dev/irdma/irdma_utils.c
1544	Indeed, missed the other occurance initially.

rscheff mentioned this in D58325: irdma: improve validation checks further.Mon, Jul 20, 3:24 PM

markj added inline comments.Thu, Jul 23, 2:40 PM

sys/dev/irdma/irdma_cm.c
837	Why not make it an error?
sys/dev/irdma/irdma_utils.c
1544	My second comment still stands: what's the purpose of checking `th_off < 5` above? That case is already handled here and in irdma_puda_get_tcpip_info().
1591	I think you still need to check that this subtraction won't underflow?