wireguard: queueing: account for skb->protocol==0
authorJason A. Donenfeld <Jason@zx2c4.com>
Thu, 19 Mar 2020 00:30:45 +0000 (18:30 -0600)
committerDavid S. Miller <davem@davemloft.net>
Thu, 19 Mar 2020 01:51:43 +0000 (18:51 -0700)
commita5588604af448664e796daf3c1d5a4523c60667b
treec46e3cd33a702ee79364d2272acaf51eeb526bf4
parent551599edbfff2431cef943a772fbde1c3e26eaf8
wireguard: queueing: account for skb->protocol==0

We carry out checks to the effect of:

  if (skb->protocol != wg_examine_packet_protocol(skb))
    goto err;

By having wg_skb_examine_untrusted_ip_hdr return 0 on failure, this
means that the check above still passes in the case where skb->protocol
is zero, which is possible to hit with AF_PACKET:

  struct sockaddr_pkt saddr = { .spkt_device = "wg0" };
  unsigned char buffer[5] = { 0 };
  sendto(socket(AF_PACKET, SOCK_PACKET, /* skb->protocol = */ 0),
         buffer, sizeof(buffer), 0, (const struct sockaddr *)&saddr, sizeof(saddr));

Additional checks mean that this isn't actually a problem in the code
base, but I could imagine it becoming a problem later if the function is
used more liberally.

I would prefer to fix this by having wg_examine_packet_protocol return a
32-bit ~0 value on failure, which will never match any value of
skb->protocol, which would simply change the generated code from a mov
to a movzx. However, sparse complains, and adding __force casts doesn't
seem like a good idea, so instead we just add a simple helper function
to check for the zero return value. Since wg_examine_packet_protocol
itself gets inlined, this winds up not adding an additional branch to
the generated code, since the 0 return value already happens in a
mergable branch.

Reported-by: Fabian Freyer <fabianfreyer@radicallyopensecurity.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
drivers/net/wireguard/device.c
drivers/net/wireguard/queueing.h
drivers/net/wireguard/receive.c