netdev - [RFC PATCH net] vrf: fix incorrect dereferencing of skb->cb

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]

Date:   Mon, 14 Feb 2022 21:10:29 +0800
From:   "D. Wythe" <alibuda@...ux.alibaba.com>
To:     dsahern@...nel.org
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org
Subject: [RFC PATCH net] vrf: fix incorrect dereferencing of skb->cb

From: "D. Wythe" <alibuda@...ux.alibaba.com>

There is a failed test case in vrf scenario, it can be reproduced
with script:

./vrf_route_leaking.sh -t ipv6_ping_frag

Which output is similar to following:

TEST: Basic IPv6 connectivity			[ OK ]
TEST: Ping received ICMP Packet too big		[FAIL]

The direct cause of this issue is because the Packet too big in ICMPv6
is sent by the message whose source address is IPv6 loopback address and
is discarded dues to its input interface is not the local loopback device.
But the root cause is there's a bug that affects the correct source
address selection.

In the above case the calling stack is as follows:

    icmp6_send+1
    ip6_fragment+543
    ip6_output+98
    vrf_ip6_local_out+78
    vrf_xmit+424
    dev_hard_start_xmit+221
    __dev_queue_xmit+2792
    ip6_finish_output2+705
    ip6_output+98
    ip6_forward+1464
    ipv6_rcv+67

To be more specific:

ipv6_rcv()
	init skb->cb as struct inet6_skb_parm
...
__dev_queue_xmit()
	qdisc_pkt_len_init()
	init skb->cb as struct qdisc_skb_cb
...
vrf_xmit
	fill skb->cb to zero.
...
ip6_fragment()
icmp6_send()
	treats skb->cb as struct inet6_skb_parm.

icmp6_send() will try to use original input interface in IP6CB for
selecting a more meaningful source address, we should keep the old IP6CB
rather than overwrite it.

This patch try remove the memset and make the struct qdisc_skb_cb
contains inet_skb_parm/inet6_skb_parm, which makes it possible to read
the correct IP6CB in icmp6_send() without any changes in that case.

One problem is that the size of struct qdisc_skb_cb has increased,
the original skb->cb cannnot fit struct bpf_skb_data_end anymore, which
requires increase the size of skb->cb. This patch try increase it by the
union size of inet_skb_parm/inet6_skb_parm, which is 24.

struct bpf_skb_data_end {
	struct qdisc_skb_cb qdisc_cb;
	void *data_meta;
	void *data_end;
};

Fixes: ee201011c1e1 ("vrf: Reset IPCB/IP6CB when processing outbound pkts in vrf dev xmit")
Fixes: 35402e313663 ("net: Add IPv6 support to VRF device")
Signed-off-by: D. Wythe <alibuda@...ux.alibaba.com>
---
 drivers/net/vrf.c         | 2 --
 include/linux/skbuff.h    | 2 +-
 include/net/sch_generic.h | 7 +++++++
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index e0b1ab9..a3601d0 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -498,7 +498,6 @@ static netdev_tx_t vrf_process_v6_outbound(struct sk_buff *skb,
 	/* strip the ethernet header added for pass through VRF device */
 	__skb_pull(skb, skb_network_offset(skb));

-	memset(IP6CB(skb), 0, sizeof(*IP6CB(skb)));
 	ret = vrf_ip6_local_out(net, skb->sk, skb);
 	if (unlikely(net_xmit_eval(ret)))
 		dev->stats.tx_errors++;
@@ -581,7 +580,6 @@ static netdev_tx_t vrf_process_v4_outbound(struct sk_buff *skb,
 					       RT_SCOPE_LINK);
 	}

-	memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
 	ret = vrf_ip_local_out(dev_net(skb_dst(skb)->dev), skb->sk, skb);
 	if (unlikely(net_xmit_eval(ret)))
 		vrf_dev->stats.tx_errors++;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a5adbf6..0328d05 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -811,7 +811,7 @@ struct sk_buff {
 	 * want to keep them across layers you have to do a skb_clone()
 	 * first. This is owned by whoever has the skb queued ATM.
 	 */
-	char			cb[48] __aligned(8);
+	char			cb[72] __aligned(8);

 	union {
 		struct {
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 9bab396..64cb250 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -19,6 +19,7 @@
 #include <net/gen_stats.h>
 #include <net/rtnetlink.h>
 #include <net/flow_offload.h>
+#include <net/ip.h>

 struct Qdisc_ops;
 struct qdisc_walker;
@@ -440,6 +441,12 @@ struct tcf_proto {
 };

 struct qdisc_skb_cb {
+	union {
+		struct inet_skb_parm	h4;
+#if IS_ENABLED(CONFIG_IPV6)
+		struct inet6_skb_parm	h6;
+#endif
+	} header;
 	struct {
 		unsigned int		pkt_len;
 		u16			slave_dev_queue_mapping;
-- 
1.8.3.1