On Tue, Jun 30, 2020 at 09:22:18AM +1200, richard.n.proc...@gmail.com wrote:
> Hi Jason,
> On 27/06/2020, at 10:09 PM, Jason A. Donenfeld <ja...@zx2c4.com> wrote:
> > [...] I thought I'd lay out my understanding of the matter,
> > and you can let me know whether or not this corresponds with what you  
> > had in mind:
> > [...]
> My main concern is the end-to-end TCP or UDP transport checksum of the
> tunneled (or inner, or encapsulated) packet. Your argument seems to
> concern instead the per-hop IPv4 header checksum (which is also
> interesting to look at, but first things first).
> As I read it, wg_decap() tells the stack to ignore the transport checksum
> of the tunneled packet, and I think this is incorrect for the following
> reason: the packet may have been corrupted outside of the tunnel, because
> the tunnel needn't cover the entire path the packet took through the
> network.
> So a tunneled packet may be corrupt, and one addressed to the tunnel
> endpoint will be delivered with its end-to-end transport checksum
> unverified. Higher layers may receive corrupt data as as result.[*]  
> (Routers, as intermediate network elements, are under no obligation to
> check end-to-end transport checksums. It isn't their job.)
> One might argue (I do not) that this isn't a concern, because encryption
> these days is ubiquitous and comes with far stronger integrity checks.
> Nonetheless, even here transport checksums remain relevant for two
> reasons. Firsly, because networks remain unreliable: I see non-zero
> numbers of failed checksums on my systems. And, secondly, because higher
> layers require a reliable transport: TLS for instance will drop the
> connection when the MAC check fails[1].
> Let's turn to the per-hop IPv4 checksum, which wg_decap() also tells the 
> stack to ignore. I suspect the cost of verifying that checksum -- 20 bytes 
> on a hot cache line -- is negligible both absolutely and relative to the 
> cost of decrypting the packet. And as the optimisation appears nowhere 
> else in the tree that I can find, removing it would make wg(4) no worse 
> than the other tunnels. Do you have evidence that the cost is in fact 
> significant?
> Further, as Theo pointed out, the link layer has in any case no business 
> with the IPv4 checksum, being part of the network layer. Layer violations 
> are problematic for at least two reasons. Firstly, they constrict the 
> design space. For instance, I suppose the IPv4 checksum needn't be 
> per-hop. It might be updated incrementally, increasing its scope. But this 
> would be nullified by any link layer that, acting on a false assumption 
> about the layer above it, told the stack to ignore that layer's checksum. 
> Secondly, layer violations, an optimisation, which almost by definition 
> rely on additional premises, burden the correctness obligation and so 
> trade less work for the computer in narrow circumstances for (often much) 
> more work for the fallible human to show that the process remains sound.
> On that balance I now think that skipping the IPv4 check a false economy,
> too. Hopefully I have managed to make my reasons clearer this time around, 
> please let me know if not, or if you disagree. 
> See updated patch below, looking for oks to commit that.
> cheers,
> Richard.
> [*] By way of background, I understand the transport checksum was added to
> the early ARPANET primarily to guard against corruption inside its
> routers[0], after a router fault which "[brought] the network to its
> knees". In other words, end-to-end checks are necessary to catch faults in
> the routers; link-level checks aren't enough.
> More generally, transport checksums set a lower bound on the reliability
> of the end-to-end transport, which decouples it from the reliability of  
> the underlying network.
> [0] Kleinrock "Queuing Systems, Volume 2: Computer Applications",
> John Wiley and Sons (1976), p441
> [1] RFC5246 TLS v1.2 (2008), section 7.2.2.
> TLS's requirement is reasonable: it would otherwise be obliged to
> reimplement for itself retransmission, which would drag with it other
> transport layer mechanisms like acknowledgements and (for performance)
> sequence numbers and sliding windows.

I agree with all of this and think that this is the right fix.
OK claudio@
> Index: net/if_wg.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_wg.c,v
> retrieving revision 1.7
> diff -u -p -u -p -r1.7 if_wg.c
> --- net/if_wg.c       23 Jun 2020 10:03:49 -0000      1.7
> +++ net/if_wg.c       28 Jun 2020 20:17:07 -0000
> @@ -1660,14 +1660,8 @@ wg_decap(struct wg_softc *sc, struct mbu
>               goto error;
>       }
> -     /*
> -      * We can mark incoming packet csum OK. We mark all flags OK
> -      * irrespective to the packet type.
> -      */
> -     m->m_pkthdr.csum_flags |= (M_IPV4_CSUM_IN_OK | M_TCP_CSUM_IN_OK |
> -         M_UDP_CSUM_IN_OK | M_ICMP_CSUM_IN_OK);
> -     m->m_pkthdr.csum_flags &= ~(M_IPV4_CSUM_IN_BAD | M_TCP_CSUM_IN_BAD |
> +     /* tunneled packet was not offloaded */
> +     m->m_pkthdr.csum_flags = 0;
>       m->m_pkthdr.ph_ifidx = sc->sc_if.if_index;
>       m->m_pkthdr.ph_rtableid = sc->sc_if.if_rdomain;

:wq Claudio

Reply via email to