Page MenuHomeFreeBSD

Don't use uninitialized timestamp echo reply value.
ClosedPublic

Authored by pkelsey on Jul 12 2015, 7:00 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 9, 1:03 AM
Unknown Object (File)
Dec 19 2023, 9:57 PM
Unknown Object (File)
Nov 1 2023, 12:51 PM
Unknown Object (File)
Jul 5 2023, 11:25 AM
Unknown Object (File)
Jun 17 2023, 9:26 PM
Unknown Object (File)
May 12 2023, 6:41 PM
Unknown Object (File)
Mar 18 2023, 4:49 AM
Unknown Object (File)
Jan 15 2023, 9:25 AM
Subscribers

Details

Summary

If a TCP segment does not contain a timestamp option, the
auto receive buffer scaling code will take action based on an
uninitialized timestamp echo reply value off of the stack.

Check the timestamp option flag so this does not occur.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

pkelsey retitled this revision from to Don't use uninitialized timestamp echo reply value..
pkelsey updated this object.
pkelsey edited the test plan for this revision. (Show Details)
pkelsey added reviewers: lstewart, hiren.
hiren added a subscriber: network.

https://www.ietf.org/rfc/rfc1323.txt mentions:

The Timestamp Echo Reply field (TSecr) is only valid if the ACK
bit is set in the TCP header; if it is valid, it echos a times-
tamp value that was sent by the remote TCP in the TSval field
of a Timestamps option.  When TSecr is not valid, its value
must be zero.

Which I brought up to Patrick but then he mentioned that while following this and always initializing the tsecr field early in tcpopt processing would be the correct thing to do but the code is already established elsewhere to 1) check the option flag for the timestamp option and then 2) check if the value is not zero.

And I agree that because of that reasoning, the proposed fix seems sufficient to me.

Also adding network so someone else can also review this.

hiren edited edge metadata.
This revision is now accepted and ready to land.Jul 13 2015, 1:04 AM
In D3060#60525, @hiren wrote:

https://www.ietf.org/rfc/rfc1323.txt mentions:

The Timestamp Echo Reply field (TSecr) is only valid if the ACK
bit is set in the TCP header; if it is valid, it echos a times-
tamp value that was sent by the remote TCP in the TSval field
of a Timestamps option.  When TSecr is not valid, its value
must be zero.

Which I brought up to Patrick but then he mentioned that while following this and always initializing the tsecr field early in tcpopt processing would be the correct thing to do but the code is already established elsewhere to 1) check the option flag for the timestamp option and then 2) check if the value is not zero.

And I agree that because of that reasoning, the proposed fix seems sufficient to me.

Also adding network so someone else can also review this.

Just to be clear (because it seems to me the above could be read to say we are not following the spec), regarding the quoted bit from the RFC, we are adhering to the requirement that ACK be set on the packet in order to have a valid tsecr field - we do not take any action based on tsecr if ACK is not set.

What I believed Hiren was asking me was whether we could address the issue I am patching here by ensuring to.to_tsecr is always initialized to zero prior to options processing. We could, as the code always treats to.to_tsecr == 0 as an invalid timestamp (because reasons, even though that condition does not imply an invalid timestamp per the spec), but the convention in all other existing to.to_tsecr handling code is to establish its validity by first testing that the TS option was set on the segment and second testing that to.to_tsecr is not zero, so for consistency I think adding the TS option flag check is the way to go here over the alternative to.to_tsecr initialization approach.

This revision was automatically updated to reflect the committed changes.