Before sending a challenge ACK, check the sequence number more properly

[htibosch]

Description

This PR comes in response to issue #216: "Possible overflow in xProcessReceivedTCPPacket". @bernd-edlinger : thanks a lot for reporting this.

When a RST packet is received for an existing TCP connection, 3 things can happen:

• The peer knows exactly what seq number the device is expecting: accept the RST and close the socket.
• The peer comes with a seq number that is within the RX window: send a challenge ACK
• The peer doesn't have a clue about the seq number: ignore the RST packet

The check of the sequence number was not totally exact: it would fail if the sequence number is about to pass through zero.
Solution: use the functions xSequenceGreaterThan() and xSequenceLessThan(). They are defined in FreeRTOS_TCP_WIN.c and now made public:

Test Steps

It is a bit difficult to do an integration test for this change, it needs a bit of hacking. Start a TCP connection while the sequence number is e.g. 0xFFFFFF00. Do not send data, but in stead let the peer send a RST packet with a sequence number of e.g. 0x01 or 0x100.

In stead, I did a simulation:

{
    uint32_t ulSequenceNumber, ulCurrentSequenceNumber, ulRxWindowLength;
    bool r1, r2, r3, r4;

    /* A normal situation, no roll-over: */
    ulCurrentSequenceNumber = 0x0000F000U;
    ulSequenceNumber = ulCurrentSequenceNumber + 512;
    ulRxWindowLength = 1460U;
    r1 = xSequenceGreaterThan( ulSequenceNumber, ulCurrentSequenceNumber );
    r2 = xSequenceLessThan( ulSequenceNumber, ( ulCurrentSequenceNumber + ulRxWindowLength ) );
    r3 = ( ulSequenceNumber > ulCurrentSequenceNumber ) ? 1 : 0;
    r4 = ( ulSequenceNumber < ( ulCurrentSequenceNumber + ulRxWindowLength ) ) ? 1 : 0;
    logPrintf("rc = %d %d versus %d %d\n", r1, r2, r3, r4);

    /* Rolling over but within the expected range: */
    ulCurrentSequenceNumber = 0xFFFFFF00U;
    ulSequenceNumber = ulCurrentSequenceNumber + 512;
    ulRxWindowLength = 1460U;
    r1 = xSequenceGreaterThan( ulSequenceNumber, ulCurrentSequenceNumber );
    r2 = xSequenceLessThan( ulSequenceNumber, ( ulCurrentSequenceNumber + ulRxWindowLength ) );
    r3 = ( ulSequenceNumber > ulCurrentSequenceNumber ) ? 1 : 0;
    r4 = ( ulSequenceNumber < ( ulCurrentSequenceNumber + ulRxWindowLength ) ) ? 1 : 0;
    logPrintf("rc = %d %d versus %d %d\n", r1, r2, r3, r4);

    /* Not rolling over, but still within the expected range: */
    ulCurrentSequenceNumber = 0xFFFFFF00U;
    ulSequenceNumber = ulCurrentSequenceNumber + 1;
    ulRxWindowLength = 1460U;
    r1 = xSequenceGreaterThan( ulSequenceNumber, ulCurrentSequenceNumber );
    r2 = xSequenceLessThan( ulSequenceNumber, ( ulCurrentSequenceNumber + ulRxWindowLength ) );
    r3 = ( ulSequenceNumber > ulCurrentSequenceNumber ) ? 1 : 0;
    r4 = ( ulSequenceNumber < ( ulCurrentSequenceNumber + ulRxWindowLength ) ) ? 1 : 0;
    logPrintf("rc = %d %d versus %d %d\n", r1, r2, r3, r4);
}

/*
    The output is:
    rc = 1 1 versus 1 1 // good
    rc = 1 1 versus 0 1 // wrong
    rc = 1 1 versus 1 0 // wrong
    The results on the left hand are all correct.
*/

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[bernd-edlinger]

you could also do something like this:

else if ( ulSequenceNumber - pxSocket->u.xTCP.xTCPWindow.rx.ulCurrentSequenceNumber <
          pxSocket->u.xTCP.xTCPWindow.xSize.ulRxWindowLength )

[htibosch]

Yes I believe that. indirectly, that test has been done. Formally ( i.e. without the overflows ), the following two staments are equal:

• else if ( ulSequenceNumber - ulCurrentSequenceNumber < ulRxWindowLength )
• else if ( ulSequenceNumber < ulRxWindowLength + ulCurrentSequenceNumber )

Only I have expressed it as:

( xSequenceLessThan( ulSequenceNumber, ( ulCurrentSequenceNumber + ulRxWindowLength ) ) )

[n9wxu]

xSequenceLessThan is not linking. Did a file get missed in the commit?

[bernd-edlinger]

right, both ways it is correct as long as ulRxWindowLength <= 2^31
it's just that your solution needs two checks. (this is the response to @htibosch 's comment above)

[bernd-edlinger]

actually to prove the equivalence I need 1 <= ulRxWindowLength && ulRxWindowLength <= 2^31+2
Are we sure that ulRxWindowLength is not zero?

[htibosch]

@n9wxu

xSequenceLessThan is not linking. Did a file get missed in the commit?

Sorry yes, I forgot to upload the TCP_WIN source files. One moment.

[AniruddhaKanhere]

Are we sure that ulRxWindowLength is not zero?

I gave a quick look to the source and it seems that ulRxWindowLength is never 0. Even when the windowing is disabled for TCP, the value of ulRxWindowLength == ipconfigTCP_MSS.

If someone was to mess with ipconfigTCP_MSS and make it 0, then the overflow of sequence numbers would be the least of their problems. :)

[htibosch]

About commit: "Make xSequenceLessThan and xSequenceGreaterThan public functions"

These 2 functions will now compile as global functions, independently of the macro ipconfigUSE_TCP_WIN.

[gkwicker]

Approved with a few minor suggestions.

[gkwicker]

nit: code might be clearer as:

BaseType_t xResult = pdFALSE;

/* Test if a < b */
if( ( ( b - ( a + 1UL ) ) & 0x80000000UL ) == 0UL )
{
    xResult = pdTRUE;
}
return xResult;

[gkwicker]

nit: see previous comment in xSequenceLessThan().

[gkwicker]

nits: should these use portINLINE? Also, the names indicate that these are to be used with TCP sequence numbers but they could be used with any 32-bit rolling counter. Are these good names? Is there anywhere else they might be useful?

[gkwicker]

nit: we might add a little more detail in this description – it could be helpful to know a little more about how a and b are used. This helper function is for rolling counter applications like TCP sequence numbers.

[bernd-edlinger]

I just saw an error from the "spell check"

tcp/include/FreeRTOS_TCP_WIN.h

   240	 * @note GreaterThan is calculated as "( a - ( b + 1U ) ) < 0x80000000".

tcp/include/FreeRTOS_TCP_WIN.h

   253	 * @note LessThan is implemented as "( b - ( a + 1 ) ) < 0x80000000".
Error: Process completed with exit code 1.

IOW it does not like "GreaterThan" and "LessThan".

[htibosch]

About "Although no changes were made to MPS2_AN385, some formatting changes to that driver"
It seems to solve the formatting errors.