pf: Handle (*m0)->m_len < sizeof(struct ip) case

[igor.ostapenko_pm.me]

Summary

if_enc(4) can pass IPsec payload to pfil(9) with the outer header or without
it. In case of a small packet like ICMP, when mbuf cluster is not used,
everything works fine. Otherwise, the first mbuf in a chain has m_len == 0
if it is asked to strip the outer header. pf was not handling such case, and
erroneous reading of the outer IP header led to unexpected behavior.

Test Plan

# kldload ipsec if_enc pf
# kyua test -k /usr/tests/sys/netpfil/pf/Kyuafile if_enc

[igor.ostapenko_pm.me]

The first iteration happened here: https://reviews.freebsd.org/D45762.

[kp]

Looks sane at first glance. I'll take a deeper look on Tuesday.

There's probably a few other places we want to do the pull-up too, such as the ethernet rules and the IPv6 rules.
Places like pf_isforlocal() might need an assert.

sys/netpfil/pf/pf.c

8084

I'm a little unsure if it's worth checking this, or if we should just call m_pullup() unconditionally.
Either one is safe, doing it unconditionally is a little less code here, but costs us a function call for every packet.

[igor.ostapenko_pm.me]

In D45780#1044154, @kp wrote:

There's probably a few other places we want to do the pull-up too, such as the ethernet rules and the IPv6 rules.
Places like pf_isforlocal() might need an assert.

Yes, the idea was to have a conclusion on IPv4+pf first, to make sure there is an agreement over the place and the way. Also, I spot the same issue on ipfw side. The next patches could extrapolate the respective new test cases with common code extraction as an option.

Let me know if it's expected to be a single big patch for pf, covering all the sub-cases: IPv4, IPv6, Ethernet, etc.

[igor.ostapenko_pm.me]

sys/netpfil/pf/pf.c

8084

Yeah, my gut feeling is that CPU mechanisms like branch prediction and pipeline reset should be a lesser overhead here. The m_pullup call is expected to do m_get(); m_move_pkthdr(); bcopy(); chain unconditionally for M_EXT mbufS, which is expected to be the most frequent case, I guess.

[glebius]

I'm actually very much surprised that this bug was there was all these years. This means most drivers never create such suboptimal packets.

P.S. We didn't pullup potential IP options. What happens if packet doesn't belong to a known protocol (TCP/UDP/etc) but has options and also options are allowed, are we going to dereference beyond sizeof(struct ip)?

sys/netpfil/pf/pf.c

8084

Just augment it with __predict_false(m->m_len < sizeof(struct ip)).