Page MenuHomeFreeBSD

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

Authored by rrs on Thu, Jul 15, 4:01 PM.

Details

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.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rrs requested review of this revision.Thu, Jul 15, 4:01 PM

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

This revision is now accepted and ready to land.Thu, Jul 15, 4:56 PM

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

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 ;-)

Add Hans predict false around the hopefully never exercised returns

This revision now requires review to proceed.Thu, Jul 15, 5:32 PM

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?

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.

sys/netinet/tcp_lro.c
407

Or:
m->m_len - po->total_hdr_len

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
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.

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!

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.

This revision is now accepted and ready to land.Thu, Jul 15, 7:33 PM