b.liu | e958203 | 2025-04-17 19:18:16 +0800 | [diff] [blame] | 1 | From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 |
| 2 | From: "Jason A. Donenfeld" <Jason@zx2c4.com> |
| 3 | Date: Wed, 18 Mar 2020 18:30:45 -0600 |
| 4 | Subject: [PATCH] wireguard: queueing: account for skb->protocol==0 |
| 5 | |
| 6 | commit a5588604af448664e796daf3c1d5a4523c60667b upstream. |
| 7 | |
| 8 | We carry out checks to the effect of: |
| 9 | |
| 10 | if (skb->protocol != wg_examine_packet_protocol(skb)) |
| 11 | goto err; |
| 12 | |
| 13 | By having wg_skb_examine_untrusted_ip_hdr return 0 on failure, this |
| 14 | means that the check above still passes in the case where skb->protocol |
| 15 | is zero, which is possible to hit with AF_PACKET: |
| 16 | |
| 17 | struct sockaddr_pkt saddr = { .spkt_device = "wg0" }; |
| 18 | unsigned char buffer[5] = { 0 }; |
| 19 | sendto(socket(AF_PACKET, SOCK_PACKET, /* skb->protocol = */ 0), |
| 20 | buffer, sizeof(buffer), 0, (const struct sockaddr *)&saddr, sizeof(saddr)); |
| 21 | |
| 22 | Additional checks mean that this isn't actually a problem in the code |
| 23 | base, but I could imagine it becoming a problem later if the function is |
| 24 | used more liberally. |
| 25 | |
| 26 | I would prefer to fix this by having wg_examine_packet_protocol return a |
| 27 | 32-bit ~0 value on failure, which will never match any value of |
| 28 | skb->protocol, which would simply change the generated code from a mov |
| 29 | to a movzx. However, sparse complains, and adding __force casts doesn't |
| 30 | seem like a good idea, so instead we just add a simple helper function |
| 31 | to check for the zero return value. Since wg_examine_packet_protocol |
| 32 | itself gets inlined, this winds up not adding an additional branch to |
| 33 | the generated code, since the 0 return value already happens in a |
| 34 | mergable branch. |
| 35 | |
| 36 | Reported-by: Fabian Freyer <fabianfreyer@radicallyopensecurity.com> |
| 37 | Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> |
| 38 | Signed-off-by: David S. Miller <davem@davemloft.net> |
| 39 | Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> |
| 40 | --- |
| 41 | drivers/net/wireguard/device.c | 2 +- |
| 42 | drivers/net/wireguard/queueing.h | 8 +++++++- |
| 43 | drivers/net/wireguard/receive.c | 4 ++-- |
| 44 | 3 files changed, 10 insertions(+), 4 deletions(-) |
| 45 | |
| 46 | --- a/drivers/net/wireguard/device.c |
| 47 | +++ b/drivers/net/wireguard/device.c |
| 48 | @@ -122,7 +122,7 @@ static netdev_tx_t wg_xmit(struct sk_buf |
| 49 | u32 mtu; |
| 50 | int ret; |
| 51 | |
| 52 | - if (unlikely(wg_skb_examine_untrusted_ip_hdr(skb) != skb->protocol)) { |
| 53 | + if (unlikely(!wg_check_packet_protocol(skb))) { |
| 54 | ret = -EPROTONOSUPPORT; |
| 55 | net_dbg_ratelimited("%s: Invalid IP packet\n", dev->name); |
| 56 | goto err; |
| 57 | --- a/drivers/net/wireguard/queueing.h |
| 58 | +++ b/drivers/net/wireguard/queueing.h |
| 59 | @@ -66,7 +66,7 @@ struct packet_cb { |
| 60 | #define PACKET_PEER(skb) (PACKET_CB(skb)->keypair->entry.peer) |
| 61 | |
| 62 | /* Returns either the correct skb->protocol value, or 0 if invalid. */ |
| 63 | -static inline __be16 wg_skb_examine_untrusted_ip_hdr(struct sk_buff *skb) |
| 64 | +static inline __be16 wg_examine_packet_protocol(struct sk_buff *skb) |
| 65 | { |
| 66 | if (skb_network_header(skb) >= skb->head && |
| 67 | (skb_network_header(skb) + sizeof(struct iphdr)) <= |
| 68 | @@ -81,6 +81,12 @@ static inline __be16 wg_skb_examine_untr |
| 69 | return 0; |
| 70 | } |
| 71 | |
| 72 | +static inline bool wg_check_packet_protocol(struct sk_buff *skb) |
| 73 | +{ |
| 74 | + __be16 real_protocol = wg_examine_packet_protocol(skb); |
| 75 | + return real_protocol && skb->protocol == real_protocol; |
| 76 | +} |
| 77 | + |
| 78 | static inline void wg_reset_packet(struct sk_buff *skb) |
| 79 | { |
| 80 | skb_scrub_packet(skb, true); |
| 81 | --- a/drivers/net/wireguard/receive.c |
| 82 | +++ b/drivers/net/wireguard/receive.c |
| 83 | @@ -56,7 +56,7 @@ static int prepare_skb_header(struct sk_ |
| 84 | size_t data_offset, data_len, header_len; |
| 85 | struct udphdr *udp; |
| 86 | |
| 87 | - if (unlikely(wg_skb_examine_untrusted_ip_hdr(skb) != skb->protocol || |
| 88 | + if (unlikely(!wg_check_packet_protocol(skb) || |
| 89 | skb_transport_header(skb) < skb->head || |
| 90 | (skb_transport_header(skb) + sizeof(struct udphdr)) > |
| 91 | skb_tail_pointer(skb))) |
| 92 | @@ -388,7 +388,7 @@ static void wg_packet_consume_data_done( |
| 93 | */ |
| 94 | skb->ip_summed = CHECKSUM_UNNECESSARY; |
| 95 | skb->csum_level = ~0; /* All levels */ |
| 96 | - skb->protocol = wg_skb_examine_untrusted_ip_hdr(skb); |
| 97 | + skb->protocol = wg_examine_packet_protocol(skb); |
| 98 | if (skb->protocol == htons(ETH_P_IP)) { |
| 99 | len = ntohs(ip_hdr(skb)->tot_len); |
| 100 | if (unlikely(len < sizeof(struct iphdr))) |