tcp: Lro needs to validate that it does not go beyond the end of the mbuf as it parses.

[rrs]

Summary

Currently the LRO parser, if given a packet that say has ETH+IP header but the TCP header
is in the next mbuf (split), would walk garbage. Lets make sure we keep track as we
parse of the length and return NULL anytime we exceed the length of the mbuf.

Test Plan

Make sure the parser still works with LRO when valid input is given.

[rrs]

Ok just got through testing this, and it works great.. no problems as I saw before ;-)

[• hselasky]

Hi Randall,

CPUs do speculative reads. Why can't we do the same for mbufs?

Typically the m_data has at least MHLEN bytes of valid read area.

The logic here is to speculate on the reads avoiding lots of if's, and in the end we check "pi->total_hdr_len > m->m_len" to only proceed if all the reads we based decisions on were within the bounds.

I'm also fine if you want to add if's, but then maybe use __predict_false() to optimize the code?

--HPS

[rrs]

We can add predict false here, since the majority of times we would
never go those paths. That makes sense, but we need to have the
checks. If we don't then at some point a driver writer that is not
as good as you will send in packets in an invalid way and we
won't catch it.

Let me add your predict_false's ;-)

[rrs]

Add Hans predict false around the hopefully never exercised returns

[• hselasky]

Hi Randall,

The following simple check at entry of packets for LRO, would ensure that the speculative reads are safe, if that's what you are worried about:

int max_read_space = M_SIZE(m) - (m->m_data - M_START(m));
if (max_read_space < XXX)
return (LRO_CANNOT);

Where XXX can be computed from the code.

What do you think?

[• hselasky]

sys/netinet/tcp_lro.c

407

You would need to adjust the length here:

m->m_len - (data_ptr - m->m_data)

This is the inner header.

[• hselasky]

sys/netinet/tcp_lro.c

407

Or:
m->m_len - po->total_hdr_len

[• hselasky]

sys/netinet/tcp_lro.c

408

Yes, one bug here:

pi->total_hdr_len > m->m_len

Should be:

po->total_hdr_len + pi->total_hdr_len > m->m_len

[rrs]

sys/netinet/tcp_lro.c

407

Ahh good point about the inner header ...

408

We might not even need the condition if we just monitor the m->m_len as the parser runs, it will return NULL
if we run past the length of the mbuf.

[• hselasky]

sys/netinet/tcp_lro.c

408

Yes, if the length is checked for all cases in tcp_lro_low_level_parser() including VXLAN, then you can remove those checks!

[rrs]

Correct the bug in the calculation as well as subtracting out the outer
header size from the m_len on the second inner header parse.