mbox series

[net,0/5] support AF_PACKET for layer 3 devices

Message ID 20200627080713.179883-1-Jason@zx2c4.com
Headers show
Series support AF_PACKET for layer 3 devices | expand

Message

Jason A. Donenfeld June 27, 2020, 8:07 a.m. UTC
Hans reported that packets injected by a correct-looking and trivial
libpcap-based program was not being accepted by wireguard. In
investigating that, I noticed that a few devices weren't properly
handling AF_PACKET- injected packes, and so this series introduces a bit
of shared infrastructure to support that.

The basic problem begins with socket(AF_PACKET, SOCK_RAW,
htons(ETH_P_ALL)) sockets. When sendto is called, AF_PACKET examines the
headers of the packet with this logic:

static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
{
    if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
        sock->type == SOCK_RAW) {
        skb_reset_mac_header(skb);
        skb->protocol = dev_parse_header_protocol(skb);
    }

    skb_probe_transport_header(skb);
}

The middle condition there triggers, and we jump to
dev_parse_header_protocol. Note that this is the only caller of
dev_parse_header_protocol in the kernel, and I assume it was designed
for this purpose:

static inline __be16 dev_parse_header_protocol(const struct sk_buff *skb)
{
    const struct net_device *dev = skb->dev;

    if (!dev->header_ops || !dev->header_ops->parse_protocol)
        return 0;
    return dev->header_ops->parse_protocol(skb);
}

Since AF_PACKET already knows which netdev the packet is going to, the
dev_parse_header_protocol function can see if that netdev has a way it
prefers to figure out the protocol from the header. This, again, is the
only use of parse_protocol in the kernel. At the moment, it's only used
with ethernet devices, via eth_header_parse_protocol. This makes sense,
as mostly people are used to AF_PACKET-injecting ethernet frames rather
than layer 3 frames. But with nothing in place for layer 3 netdevs, this
function winds up returning 0, and skb->protocol then is set to 0, and
then by the time it hits the netdev's ndo_start_xmit, the driver doesn't
know what to do with it.

This is a problem because drivers very much rely on skb->protocol being
correct, and routinely reject packets where it's incorrect. That's why
having this parsing happen for injected packets is quite important. In
wireguard, ipip, and ipip6, for example, packets from AF_PACKET are just
dropped entirely. For tun devices, it's sort of uglier, with the tun
"packet information" header being passed to userspace containing a bogus
protocol value. Some userspace programs are ill-equipped to deal with
that. (But of course, that doesn't happen with tap devices, which
benefit from the similar shared infrastructure for layer 2 netdevs,
further motiviating this patchset for layer 3 netdevs.)

This patchset addresses the issue by first adding a layer 3 header parse
function, much akin to the existing one for layer 2 packets, and then
adds a shared header_ops structure that, also much akin to the existing
one for layer 2 packets. Then it wires it up to a few immediate places
that stuck out as requiring it, and does a bit of cleanup.

This patchset seems like it's fixing real bugs, so it might be
appropriate for stable. But they're also very old bugs, so if you'd
rather not backport to stable, that'd make sense to me too.

Jason A. Donenfeld (5):
  net: ip_tunnel: add header_ops for layer 3 devices
  net: ipip: implement header_ops->parse_protocol for AF_PACKET
  wireguard: implement header_ops->parse_protocol for AF_PACKET
  wireguard: queueing: make use of ip_tunnel_parse_protocol
  tun: implement header_ops->parse_protocol for AF_PACKET

 drivers/net/tun.c                |  2 ++
 drivers/net/wireguard/device.c   |  1 +
 drivers/net/wireguard/queueing.h | 19 ++-----------------
 drivers/net/wireguard/receive.c  |  2 +-
 include/net/ip_tunnels.h         |  3 +++
 net/ipv4/ip_tunnel_core.c        | 19 +++++++++++++++++++
 net/ipv4/ipip.c                  |  1 +
 net/ipv6/ip6_tunnel.c            |  1 +
 8 files changed, 30 insertions(+), 18 deletions(-)

Cc: Hans Wippel <ndev@hwipl.net>
-- 
2.27.0

Comments

David Miller June 30, 2020, 12:17 a.m. UTC | #1
We customarily don't add new Copyright lines for every change and
fix made to existing files, please get rid of that.

Also please add the sit.c code handling as indicated by Willem.

Thank you.
Jason A. Donenfeld June 30, 2020, 12:41 a.m. UTC | #2
Hey Dave, Willem,

Sorry for the delay in getting back to you; this weekend was spent
moving across the state again, so not much time for emails.

> We customarily don't add new Copyright lines for every change and

> fix made to existing files, please get rid of that.


Sure, no problem. I had done that out of an abundance of caution
because the code came out of another file bearing the copyright, but I
don't care much about it, so it'll be removed in v2.

> Also please add the sit.c code handling as indicated by Willem.


Nice catch, Willem. I'll do just that. Looks like I forgot about vti
as well. I'll send a v2 shortly with those and will do another pass
through net/ and drivers/net/. And if you think of others, let me
know.

Jason