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

[sobomax]

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.

[sobomax]

Fix mismerge.

[sobomax]

Fix error message.

[sobomax]

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

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

[markj]

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

[sobomax]

sys/netgraph/ng_nat.c

822

Well, arguably it's used below. I just wanted to draw attention of a human reader/writer that it's the same value we reading pretty much, but code assumes that it can be different before and after. Compilers nowadays don't care what and when you declare anyways.

857

I suppose it's for generality, at least. I suspect as part of the NATing of complex application-level protocols that might embed IP address in its string representation (say FTP), there might be cases when processed frame needs to be slightly larger/smaller in terms of number of bytes, so that the IP frame length might need to be adjusted to match. I was spelunking various NAT protocol handlers while investigating this issue and I don't think I spotted any clear cases of that happening. But that's the only logical conclusion I found as to why do we read mbuf length back from the frame.

[sobomax]

In D49677#1132641, @tuexen wrote:

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.

Might do for the in_delayed_cksum(), probably isn't worth the effort for the ng_nat. Feels a bit of an overkill.