Skip to content

Commit cb9f1b7

Browse files
wdebruijdavem330
wdebruij
authored and
davem330
committed
ip: validate header length on virtual device xmit
KMSAN detected read beyond end of buffer in vti and sit devices when passing truncated packets with PF_PACKET. The issue affects additional ip tunnel devices. Extend commit 76c0ddd ("ip6_tunnel: be careful when accessing the inner header") and commit ccfec9e ("ip_tunnel: be careful when accessing the inner header"). Move the check to a separate helper and call at the start of each ndo_start_xmit function in net/ipv4 and net/ipv6. Minor changes: - convert dev_kfree_skb to kfree_skb on error path, as dev_kfree_skb calls consume_skb which is not for error paths. - use pskb_network_may_pull even though that is pedantic here, as the same as pskb_may_pull for devices without llheaders. - do not cache ipv6 hdrs if used only once (unsafe across pskb_may_pull, was more relevant to earlier patch) Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: Willem de Bruijn <willemb@google.com> Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent 8c76e77 commit cb9f1b7

File tree

9 files changed

+66
-32
lines changed

9 files changed

+66
-32
lines changed

‎include/net/ip_tunnels.h

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,26 @@ int ip_tunnel_encap_del_ops(const struct ip_tunnel_encap_ops *op,
308308
int ip_tunnel_encap_setup(struct ip_tunnel *t,
309309
struct ip_tunnel_encap *ipencap);
310310

311+
static inline bool pskb_inet_may_pull(struct sk_buff *skb)
312+
{
313+
int nhlen;
314+
315+
switch (skb->protocol) {
316+
#if IS_ENABLED(CONFIG_IPV6)
317+
case htons(ETH_P_IPV6):
318+
nhlen = sizeof(struct ipv6hdr);
319+
break;
320+
#endif
321+
case htons(ETH_P_IP):
322+
nhlen = sizeof(struct iphdr);
323+
break;
324+
default:
325+
nhlen = 0;
326+
}
327+
328+
return pskb_network_may_pull(skb, nhlen);
329+
}
330+
311331
static inline int ip_encap_hlen(struct ip_tunnel_encap *e)
312332
{
313333
const struct ip_tunnel_encap_ops *ops;

‎net/ipv4/ip_gre.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -676,6 +676,9 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
676676
struct ip_tunnel *tunnel = netdev_priv(dev);
677677
const struct iphdr *tnl_params;
678678

679+
if (!pskb_inet_may_pull(skb))
680+
goto free_skb;
681+
679682
if (tunnel->collect_md) {
680683
gre_fb_xmit(skb, dev, skb->protocol);
681684
return NETDEV_TX_OK;
@@ -719,6 +722,9 @@ static netdev_tx_t erspan_xmit(struct sk_buff *skb,
719722
struct ip_tunnel *tunnel = netdev_priv(dev);
720723
bool truncate = false;
721724

725+
if (!pskb_inet_may_pull(skb))
726+
goto free_skb;
727+
722728
if (tunnel->collect_md) {
723729
erspan_fb_xmit(skb, dev, skb->protocol);
724730
return NETDEV_TX_OK;
@@ -762,6 +768,9 @@ static netdev_tx_t gre_tap_xmit(struct sk_buff *skb,
762768
{
763769
struct ip_tunnel *tunnel = netdev_priv(dev);
764770

771+
if (!pskb_inet_may_pull(skb))
772+
goto free_skb;
773+
765774
if (tunnel->collect_md) {
766775
gre_fb_xmit(skb, dev, htons(ETH_P_TEB));
767776
return NETDEV_TX_OK;

‎net/ipv4/ip_tunnel.c

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -627,7 +627,6 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
627627
const struct iphdr *tnl_params, u8 protocol)
628628
{
629629
struct ip_tunnel *tunnel = netdev_priv(dev);
630-
unsigned int inner_nhdr_len = 0;
631630
const struct iphdr *inner_iph;
632631
struct flowi4 fl4;
633632
u8 tos, ttl;
@@ -637,14 +636,6 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
637636
__be32 dst;
638637
bool connected;
639638

640-
/* ensure we can access the inner net header, for several users below */
641-
if (skb->protocol == htons(ETH_P_IP))
642-
inner_nhdr_len = sizeof(struct iphdr);
643-
else if (skb->protocol == htons(ETH_P_IPV6))
644-
inner_nhdr_len = sizeof(struct ipv6hdr);
645-
if (unlikely(!pskb_may_pull(skb, inner_nhdr_len)))
646-
goto tx_error;
647-
648639
inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
649640
connected = (tunnel->parms.iph.daddr != 0);
650641

‎net/ipv4/ip_vti.c

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,9 @@ static netdev_tx_t vti_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
241241
struct ip_tunnel *tunnel = netdev_priv(dev);
242242
struct flowi fl;
243243

244+
if (!pskb_inet_may_pull(skb))
245+
goto tx_err;
246+
244247
memset(&fl, 0, sizeof(fl));
245248

246249
switch (skb->protocol) {
@@ -253,15 +256,18 @@ static netdev_tx_t vti_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
253256
memset(IP6CB(skb), 0, sizeof(*IP6CB(skb)));
254257
break;
255258
default:
256-
dev->stats.tx_errors++;
257-
dev_kfree_skb(skb);
258-
return NETDEV_TX_OK;
259+
goto tx_err;
259260
}
260261

261262
/* override mark with tunnel output key */
262263
fl.flowi_mark = be32_to_cpu(tunnel->parms.o_key);
263264

264265
return vti_xmit(skb, dev, &fl);
266+
267+
tx_err:
268+
dev->stats.tx_errors++;
269+
kfree_skb(skb);
270+
return NETDEV_TX_OK;
265271
}
266272

267273
static int vti4_err(struct sk_buff *skb, u32 info)

‎net/ipv6/ip6_gre.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -881,6 +881,9 @@ static netdev_tx_t ip6gre_tunnel_xmit(struct sk_buff *skb,
881881
struct net_device_stats *stats = &t->dev->stats;
882882
int ret;
883883

884+
if (!pskb_inet_may_pull(skb))
885+
goto tx_err;
886+
884887
if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr))
885888
goto tx_err;
886889

@@ -923,6 +926,9 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
923926
int nhoff;
924927
int thoff;
925928

929+
if (!pskb_inet_may_pull(skb))
930+
goto tx_err;
931+
926932
if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr))
927933
goto tx_err;
928934

@@ -995,16 +1001,14 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
9951001
goto tx_err;
9961002
}
9971003
} else {
998-
struct ipv6hdr *ipv6h = ipv6_hdr(skb);
999-
10001004
switch (skb->protocol) {
10011005
case htons(ETH_P_IP):
10021006
memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
10031007
prepare_ip6gre_xmit_ipv4(skb, dev, &fl6,
10041008
&dsfield, &encap_limit);
10051009
break;
10061010
case htons(ETH_P_IPV6):
1007-
if (ipv6_addr_equal(&t->parms.raddr, &ipv6h->saddr))
1011+
if (ipv6_addr_equal(&t->parms.raddr, &ipv6_hdr(skb)->saddr))
10081012
goto tx_err;
10091013
if (prepare_ip6gre_xmit_ipv6(skb, dev, &fl6,
10101014
&dsfield, &encap_limit))

‎net/ipv6/ip6_tunnel.c

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1243,10 +1243,6 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
12431243
u8 tproto;
12441244
int err;
12451245

1246-
/* ensure we can access the full inner ip header */
1247-
if (!pskb_may_pull(skb, sizeof(struct iphdr)))
1248-
return -1;
1249-
12501246
iph = ip_hdr(skb);
12511247
memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
12521248

@@ -1321,9 +1317,6 @@ ip6ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
13211317
u8 tproto;
13221318
int err;
13231319

1324-
if (unlikely(!pskb_may_pull(skb, sizeof(*ipv6h))))
1325-
return -1;
1326-
13271320
ipv6h = ipv6_hdr(skb);
13281321
tproto = READ_ONCE(t->parms.proto);
13291322
if ((tproto != IPPROTO_IPV6 && tproto != 0) ||
@@ -1405,6 +1398,9 @@ ip6_tnl_start_xmit(struct sk_buff *skb, struct net_device *dev)
14051398
struct net_device_stats *stats = &t->dev->stats;
14061399
int ret;
14071400

1401+
if (!pskb_inet_may_pull(skb))
1402+
goto tx_err;
1403+
14081404
switch (skb->protocol) {
14091405
case htons(ETH_P_IP):
14101406
ret = ip4ip6_tnl_xmit(skb, dev);

‎net/ipv6/ip6_vti.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -522,18 +522,18 @@ vti6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
522522
{
523523
struct ip6_tnl *t = netdev_priv(dev);
524524
struct net_device_stats *stats = &t->dev->stats;
525-
struct ipv6hdr *ipv6h;
526525
struct flowi fl;
527526
int ret;
528527

528+
if (!pskb_inet_may_pull(skb))
529+
goto tx_err;
530+
529531
memset(&fl, 0, sizeof(fl));
530532

531533
switch (skb->protocol) {
532534
case htons(ETH_P_IPV6):
533-
ipv6h = ipv6_hdr(skb);
534-
535535
if ((t->parms.proto != IPPROTO_IPV6 && t->parms.proto != 0) ||
536-
vti6_addr_conflict(t, ipv6h))
536+
vti6_addr_conflict(t, ipv6_hdr(skb)))
537537
goto tx_err;
538538

539539
xfrm_decode_session(skb, &fl, AF_INET6);

‎net/ipv6/ip6mr.c

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
#include <linux/export.h>
5252
#include <net/ip6_checksum.h>
5353
#include <linux/netconf.h>
54+
#include <net/ip_tunnels.h>
5455

5556
#include <linux/nospec.h>
5657

@@ -599,13 +600,12 @@ static netdev_tx_t reg_vif_xmit(struct sk_buff *skb,
599600
.flowi6_iif = skb->skb_iif ? : LOOPBACK_IFINDEX,
600601
.flowi6_mark = skb->mark,
601602
};
602-
int err;
603603

604-
err = ip6mr_fib_lookup(net, &fl6, &mrt);
605-
if (err < 0) {
606-
kfree_skb(skb);
607-
return err;
608-
}
604+
if (!pskb_inet_may_pull(skb))
605+
goto tx_err;
606+
607+
if (ip6mr_fib_lookup(net, &fl6, &mrt) < 0)
608+
goto tx_err;
609609

610610
read_lock(&mrt_lock);
611611
dev->stats.tx_bytes += skb->len;
@@ -614,6 +614,11 @@ static netdev_tx_t reg_vif_xmit(struct sk_buff *skb,
614614
read_unlock(&mrt_lock);
615615
kfree_skb(skb);
616616
return NETDEV_TX_OK;
617+
618+
tx_err:
619+
dev->stats.tx_errors++;
620+
kfree_skb(skb);
621+
return NETDEV_TX_OK;
617622
}
618623

619624
static int reg_vif_get_iflink(const struct net_device *dev)

‎net/ipv6/sit.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,6 +1021,9 @@ static netdev_tx_t sit_tunnel_xmit__(struct sk_buff *skb,
10211021
static netdev_tx_t sit_tunnel_xmit(struct sk_buff *skb,
10221022
struct net_device *dev)
10231023
{
1024+
if (!pskb_inet_may_pull(skb))
1025+
goto tx_err;
1026+
10241027
switch (skb->protocol) {
10251028
case htons(ETH_P_IP):
10261029
sit_tunnel_xmit__(skb, dev, IPPROTO_IPIP);

0 commit comments

Comments
 (0)