Page MenuHomeFreeBSD

Fix potential crash calling TCP delayed checksum function when ng_nat is attached to the layer 2
AcceptedPublic

Authored by sobomax on Sat, Apr 5, 11:32 PM.
Referenced Files
F114074262: D49677.id153196.diff
Mon, Apr 7, 4:09 PM
F114067045: D49677.id153188.diff
Mon, Apr 7, 1:46 PM
F114061760: D49677.id.diff
Mon, Apr 7, 12:06 PM
F114050912: D49677.id153190.diff
Mon, Apr 7, 8:41 AM
F114048357: D49677.diff
Mon, Apr 7, 8:00 AM
F114042435: D49677.id153189.diff
Mon, Apr 7, 6:20 AM
Unknown Object (File)
Sun, Apr 6, 6:47 AM

Details

Summary

Fix potential crash in the ng_nat module when attaching directly to the layer 2 (ethernet) while calculating TCP checksum.

The issue is due to in_delayed_cksum() expecting to access IP header at the offset 0 from the mbuf start, while if we are attached to the L2 directly, the IP header at going to be at the certain offset. As a result, in_delayed_cksum() will read some bogus value and try to use it as ip_len, obviously screwing up the calculation but also potentially causing assertion panic trying to read past allocated mbuf boundary.

Provide a specialized version of the in_delayed_cksum() kernel API to DTRT in such case.

Test Plan

We have this patch deployed on a fleet of systems for about 2 months and have not seen any of those crashes since.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

printf() -> log(). Fix type mismatch.

tuexen added a subscriber: tuexen.

If I understand the patch correctly, it contains three components:

  • the fix by replacing in_delayed_cksum(m) with in_delayed_cksum_o(m, ipofs) in ng_nat.c.
  • adding protection code which logs a warning.
  • refactoring the code to remove indentation by using goto send.

It might be useful to separate these three when committing the code.

This revision is now accepted and ready to land.Sun, Apr 6, 6:01 AM
sys/netgraph/ng_nat.c
822

Since ip_len isn't used elsewhere, just check m->m_pkthdr.len < ipofs + ntohs(ip->ip_len)?

857

When does libalias change the packet length? I see that ipfw updates the mbuf length as well, but there's no explanation for why.

859
882

Variables should be declared at the beginning of a scope.

sys/netinet/ip_output.c
1053

We should add an assertion here KASSERT(iph_offset + sizeof(*ip) <= m->m_len, ("%s: truncated mbuf %p", __func__, m));