netdev - safe skb resetting after decapsulation and encapsulation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHmME9qBtB_Vwxzs+fry2UPVQ6yCWNrmVttX4wXu+RiNq3A7sw@mail.gmail.com>
Date:   Sat, 12 May 2018 00:56:53 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Netdev <netdev@...r.kernel.org>
Subject: safe skb resetting after decapsulation and encapsulation

Hey Netdev,

A UDP skb comes in via the encap_rcv interface. I do a lot of wild
things to the bytes in the skb -- change where the head starts, modify
a few fragments, decrypt some stuff, trim off some things at the end,
etc. In other words, I'm decapsulating the skb in a pretty intense
way. I benefit from reusing the same skb, performance wise, but after
I'm done processing it, it's really a totally new skb. Eventually it's
time to pass off my skb to netif_receive_skb/netif_rx, but before I do
that, I need to "reinitialize" the skb. (The same goes for when
sending out an skb -- I get it from userspace via ndo_start_xmit, do
crazy things to it, and eventually pass it off to the udp_tunnel send
functions, but first "reinitializing" it.)

At the moment I'm using a function that looks like this:

static void jasons_wild_and_crazy_skb_reset(struct sk_buff *skb)
{
    skb_scrub_packet(skb, true); //1
    memset(&skb->headers_start, 0, offsetof(struct sk_buff,
headers_end) - offsetof(struct sk_buff, headers_start)); //2
    skb->queue_mapping = 0; //3
    skb->nohdr = 0; //4
    skb->peeked = 0; //5
    skb->mac_len = 0; //6
    skb->dev = NULL; //7
#ifdef CONFIG_NET_SCHED
    skb->tc_index = 0; //8
    skb_reset_tc(skb); //9
#endif
    skb->hdr_len = skb_headroom(skb); //10
    skb_reset_mac_header(skb); //11
    skb_reset_network_header(skb); //12
    skb_probe_transport_header(skb, 0); //13
    skb_reset_inner_headers(skb); //14
}

I'm sure that some of this is wrong. Most of it is based on part of an
Octeon ethernet driver I read a few years ago. I numbered each
statement above, hoping to go through it with you all in detail here,
and see what we can cut away and see what we can approve.

1. Obviously correct and required.
2. This is probably wrong. At least it causes crashes when receiving
packets from RHEL 7.5's latest i40e driver in their vendor
frankenkernel, because those flags there have some critical bits
related to allocation. But there are a lot flags in there that I might
consider going through one by one and zeroing out.
3-5. Fields that should be zero, I assume, after
decapsulating/decrypting (and encapsulating/encrypting).
6. WireGuard is layer 3, so there's no mac.
7. We're later going to change the dev this came in on.
8-9: Same flakey rationale as 2,3-5.
10: Since the headroom has changed during the various modifications, I
need to let the packet field know about it.
11-14: The beginning of the headers has changed, and so resetting and
probing is necessary for this to work at all.

So I'm wondering - how much of this is necessary? How much am I
unnecessarily reinventing things that exist elsewhere? I'm pretty sure
in most cases the driver would work with only 1,10-14, but I worry
that bad things would happen in more unusual configurations. I've
tried to systematically go through the entire stack and see where
these might be used or not used, but it seems really inconsistent.

So, I'm writing wondering if somebody has an easy simplification or
rule for handling this kind of intense decapsulation/decryption (and
encapsulation/encryption operation on the other way) operation. I'd
like to make sure I get this down solid.

Thanks,
Jason