[PATCH net] geneve: pull IP header before ECN decapsulation

[Eric Dumazet]

From: Eric Dumazet <edum...@google.com>

IP_ECN_decapsulate() and IP6_ECN_decapsulate() assume
IP header is already pulled.

geneve does not ensure this yet.

Fixing this generically in IP_ECN_decapsulate() and
IP6_ECN_decapsulate() is not possible, since callers
pass a pointer that might be freed by pskb_may_pull()

syzbot reported :

BUG: KMSAN: uninit-value in __INET_ECN_decapsulate include/net/inet_ecn.h:238 [inline]
BUG: KMSAN: uninit-value in INET_ECN_decapsulate+0x345/0x1db0 include/net/inet_ecn.h:260
CPU: 1 PID: 8941 Comm: syz-executor.0 Not tainted 5.10.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x21c/0x280 lib/dump_stack.c:118
kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:118
__msan_warning+0x5f/0xa0 mm/kmsan/kmsan_instr.c:197
__INET_ECN_decapsulate include/net/inet_ecn.h:238 [inline]
INET_ECN_decapsulate+0x345/0x1db0 include/net/inet_ecn.h:260
geneve_rx+0x2103/0x2980 include/net/inet_ecn.h:306
geneve_udp_encap_recv+0x105c/0x1340 drivers/net/geneve.c:377
udp_queue_rcv_one_skb+0x193a/0x1af0 net/ipv4/udp.c:2093
udp_queue_rcv_skb+0x282/0x1050 net/ipv4/udp.c:2167
udp_unicast_rcv_skb net/ipv4/udp.c:2325 [inline]
__udp4_lib_rcv+0x399d/0x5880 net/ipv4/udp.c:2394
udp_rcv+0x5c/0x70 net/ipv4/udp.c:2564
ip_protocol_deliver_rcu+0x572/0xc50 net/ipv4/ip_input.c:204
ip_local_deliver_finish net/ipv4/ip_input.c:231 [inline]
NF_HOOK include/linux/netfilter.h:301 [inline]
ip_local_deliver+0x583/0x8d0 net/ipv4/ip_input.c:252
dst_input include/net/dst.h:449 [inline]
ip_rcv_finish net/ipv4/ip_input.c:428 [inline]
NF_HOOK include/linux/netfilter.h:301 [inline]
ip_rcv+0x5c3/0x840 net/ipv4/ip_input.c:539
__netif_receive_skb_one_core net/core/dev.c:5315 [inline]
__netif_receive_skb+0x1ec/0x640 net/core/dev.c:5429
process_backlog+0x523/0xc10 net/core/dev.c:6319
napi_poll+0x420/0x1010 net/core/dev.c:6763
net_rx_action+0x35c/0xd40 net/core/dev.c:6833
__do_softirq+0x1a9/0x6fa kernel/softirq.c:298
asm_call_irq_on_stack+0xf/0x20
</IRQ>
__run_on_irqstack arch/x86/include/asm/irq_stack.h:26 [inline]
run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:77 [inline]
do_softirq_own_stack+0x6e/0x90 arch/x86/kernel/irq_64.c:77
do_softirq kernel/softirq.c:343 [inline]
__local_bh_enable_ip+0x184/0x1d0 kernel/softirq.c:195
local_bh_enable+0x36/0x40 include/linux/bottom_half.h:32
rcu_read_unlock_bh include/linux/rcupdate.h:730 [inline]
__dev_queue_xmit+0x3a9b/0x4520 net/core/dev.c:4167
dev_queue_xmit+0x4b/0x60 net/core/dev.c:4173
packet_snd net/packet/af_packet.c:2992 [inline]
packet_sendmsg+0x86f9/0x99d0 net/packet/af_packet.c:3017
sock_sendmsg_nosec net/socket.c:651 [inline]
sock_sendmsg net/socket.c:671 [inline]
__sys_sendto+0x9dc/0xc80 net/socket.c:1992
__do_sys_sendto net/socket.c:2004 [inline]
__se_sys_sendto+0x107/0x130 net/socket.c:2000
__x64_sys_sendto+0x6e/0x90 net/socket.c:2000
do_syscall_64+0x9f/0x140 arch/x86/entry/common.c:48
entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: 2d07dc79fe04 ("geneve: add initial netdev driver for GENEVE tunnels")
Signed-off-by: Eric Dumazet <edum...@google.com>
Reported-by: syzbot <syzk...@googlegroups.com>
---
drivers/net/geneve.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 1426bfc009bc39a61e2cf039c57b56e48abc3fdc..8ae9ce2014a4a3ba7b962a209e28d1f65d4a83bd 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -257,11 +257,21 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
skb_dst_set(skb, &tun_dst->dst);

/* Ignore packet loops (and multicast echo) */
- if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr)) {
- geneve->dev->stats.rx_errors++;
- goto drop;
- }
+ if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr))
+ goto rx_error;

+ switch (skb_protocol(skb, true)) {
+ case htons(ETH_P_IP):
+ if (pskb_may_pull(skb, sizeof(struct iphdr)))
+ goto rx_error;
+ break;
+ case htons(ETH_P_IPV6):
+ if (pskb_may_pull(skb, sizeof(struct ipv6hdr)))
+ goto rx_error;
+ break;
+ default:
+ goto rx_error;
+ }
oiph = skb_network_header(skb);
skb_reset_network_header(skb);

@@ -298,6 +308,8 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
dev_sw_netstats_rx_add(geneve->dev, len);

return;
+rx_error:
+ geneve->dev->stats.rx_errors++;
drop:
/* Consume bad packet */
kfree_skb(skb);
--
2.29.2.454.gaff20da3a2-goog

[Jakub Kicinski]

On Tue, 1 Dec 2020 01:05:07 -0800 Eric Dumazet wrote:
> From: Eric Dumazet <edum...@google.com>
>
> IP_ECN_decapsulate() and IP6_ECN_decapsulate() assume
> IP header is already pulled.
>
> geneve does not ensure this yet.
>
> Fixing this generically in IP_ECN_decapsulate() and
> IP6_ECN_decapsulate() is not possible, since callers
> pass a pointer that might be freed by pskb_may_pull()
>
> syzbot reported :
>
> BUG: KMSAN: uninit-value in __INET_ECN_decapsulate include/net/inet_ecn.h:238 [inline]
> BUG: KMSAN: uninit-value in INET_ECN_decapsulate+0x345/0x1db0 include/net/inet_ecn.h:260

>

> Fixes: 2d07dc79fe04 ("geneve: add initial netdev driver for GENEVE tunnels")
> Signed-off-by: Eric Dumazet <edum...@google.com>
> Reported-by: syzbot <syzk...@googlegroups.com>

Applied, thanks!

[Greg Kroah-Hartman]

From: Eric Dumazet <edum...@google.com>

[ Upstream commit 4179b00c04d18ea7013f68d578d80f3c9d13150a ]

IP_ECN_decapsulate() and IP6_ECN_decapsulate() assume
IP header is already pulled.

geneve does not ensure this yet.

Fixing this generically in IP_ECN_decapsulate() and
IP6_ECN_decapsulate() is not possible, since callers
pass a pointer that might be freed by pskb_may_pull()

syzbot reported :

BUG: KMSAN: uninit-value in __INET_ECN_decapsulate include/net/inet_ecn.h:238 [inline]
BUG: KMSAN: uninit-value in INET_ECN_decapsulate+0x345/0x1db0 include/net/inet_ecn.h:260

Fixes: 2d07dc79fe04 ("geneve: add initial netdev driver for GENEVE tunnels")
Signed-off-by: Eric Dumazet <edum...@google.com>
Reported-by: syzbot <syzk...@googlegroups.com>

Link: https://lore.kernel.org/r/20201201090507.413...@gmail.com
Signed-off-by: Jakub Kicinski <ku...@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

---
drivers/net/geneve.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -256,11 +256,21 @@ static void geneve_rx(struct geneve_dev

skb_dst_set(skb, &tun_dst->dst);

/* Ignore packet loops (and multicast echo) */
- if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr)) {
- geneve->dev->stats.rx_errors++;
- goto drop;
- }
+ if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr))
+ goto rx_error;

+ switch (skb_protocol(skb, true)) {
+ case htons(ETH_P_IP):
+ if (pskb_may_pull(skb, sizeof(struct iphdr)))
+ goto rx_error;
+ break;
+ case htons(ETH_P_IPV6):
+ if (pskb_may_pull(skb, sizeof(struct ipv6hdr)))
+ goto rx_error;
+ break;
+ default:
+ goto rx_error;
+ }
oiph = skb_network_header(skb);
skb_reset_network_header(skb);

@@ -301,6 +311,8 @@ static void geneve_rx(struct geneve_dev
u64_stats_update_end(&stats->syncp);

[Greg Kroah-Hartman]

@@ -254,11 +254,21 @@ static void geneve_rx(struct geneve_dev

skb_dst_set(skb, &tun_dst->dst);

/* Ignore packet loops (and multicast echo) */
- if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr)) {
- geneve->dev->stats.rx_errors++;
- goto drop;
- }
+ if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr))
+ goto rx_error;

+ switch (skb_protocol(skb, true)) {
+ case htons(ETH_P_IP):
+ if (pskb_may_pull(skb, sizeof(struct iphdr)))
+ goto rx_error;
+ break;
+ case htons(ETH_P_IPV6):
+ if (pskb_may_pull(skb, sizeof(struct ipv6hdr)))
+ goto rx_error;
+ break;
+ default:
+ goto rx_error;
+ }
oiph = skb_network_header(skb);
skb_reset_network_header(skb);

@@ -299,6 +309,8 @@ static void geneve_rx(struct geneve_dev

[Greg Kroah-Hartman]

@@ -258,11 +258,21 @@ static void geneve_rx(struct geneve_dev

skb_dst_set(skb, &tun_dst->dst);

/* Ignore packet loops (and multicast echo) */
- if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr)) {
- geneve->dev->stats.rx_errors++;
- goto drop;
- }
+ if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr))
+ goto rx_error;

+ switch (skb_protocol(skb, true)) {
+ case htons(ETH_P_IP):
+ if (pskb_may_pull(skb, sizeof(struct iphdr)))
+ goto rx_error;
+ break;
+ case htons(ETH_P_IPV6):
+ if (pskb_may_pull(skb, sizeof(struct ipv6hdr)))
+ goto rx_error;
+ break;
+ default:
+ goto rx_error;
+ }
oiph = skb_network_header(skb);
skb_reset_network_header(skb);

@@ -303,6 +313,8 @@ static void geneve_rx(struct geneve_dev

[Greg Kroah-Hartman]

From: Eric Dumazet <edum...@google.com>

---
drivers/net/geneve.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index ee38299f9c578..e0384609fb84a 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -231,8 +231,20 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)

/* Ignore packet loops (and multicast echo) */

if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr))

- goto drop;
-
+ goto rx_error;
+

+ switch (skb_protocol(skb, true)) {
+ case htons(ETH_P_IP):
+ if (pskb_may_pull(skb, sizeof(struct iphdr)))
+ goto rx_error;
+ break;
+ case htons(ETH_P_IPV6):
+ if (pskb_may_pull(skb, sizeof(struct ipv6hdr)))
+ goto rx_error;
+ break;
+ default:
+ goto rx_error;
+ }

skb_reset_network_header(skb);

if (iph)
@@ -269,6 +281,8 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)

gro_cells_receive(&geneve->gro_cells, skb);

return;
+rx_error:
+ geneve->dev->stats.rx_errors++;
drop:
/* Consume bad packet */
kfree_skb(skb);

--
2.27.0

[Greg Kroah-Hartman]

drivers/net/geneve.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index d89995f4bd433..e6f9fe7fa2a40 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -249,11 +249,21 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
skb_dst_set(skb, &tun_dst->dst);

/* Ignore packet loops (and multicast echo) */

- if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr)) {
- geneve->dev->stats.rx_errors++;
- goto drop;

+ if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr))

+ goto rx_error;
+
+ switch (skb_protocol(skb, true)) {
+ case htons(ETH_P_IP):
+ if (pskb_may_pull(skb, sizeof(struct iphdr)))
+ goto rx_error;
+ break;
+ case htons(ETH_P_IPV6):
+ if (pskb_may_pull(skb, sizeof(struct ipv6hdr)))
+ goto rx_error;
+ break;
+ default:
+ goto rx_error;
}

-
oiph = skb_network_header(skb);
skb_reset_network_header(skb);

@@ -294,6 +304,8 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
u64_stats_update_end(&stats->syncp);

[Greg Kroah-Hartman]

index f48006c22a8a6..5eb7f409dc10b 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -254,11 +254,21 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,

skb_dst_set(skb, &tun_dst->dst);

/* Ignore packet loops (and multicast echo) */
- if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr)) {
- geneve->dev->stats.rx_errors++;
- goto drop;
+ if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr))
+ goto rx_error;
+
+ switch (skb_protocol(skb, true)) {
+ case htons(ETH_P_IP):
+ if (pskb_may_pull(skb, sizeof(struct iphdr)))
+ goto rx_error;
+ break;
+ case htons(ETH_P_IPV6):
+ if (pskb_may_pull(skb, sizeof(struct ipv6hdr)))
+ goto rx_error;
+ break;
+ default:
+ goto rx_error;
}
-
oiph = skb_network_header(skb);
skb_reset_network_header(skb);

@@ -299,6 +309,8 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,

[Eric Dumazet]

On Thu, Dec 10, 2020 at 3:26 PM Greg Kroah-Hartman
<gre...@linuxfoundation.org> wrote:
>
> From: Eric Dumazet <edum...@google.com>
>
> IP_ECN_decapsulate() and IP6_ECN_decapsulate() assume
> IP header is already pulled.
>
> geneve does not ensure this yet.
>
> Fixing this generically in IP_ECN_decapsulate() and
> IP6_ECN_decapsulate() is not possible, since callers
> pass a pointer that might be freed by pskb_may_pull()
>
> syzbot reported :
>

Note that we had to revert this patch, so you can either scratp this
backport, or make sure to backport the revert.

Thanks !

[Greg Kroah-Hartman]

I'll drop it thanks. Odd I lost the upstream git id on this patch, let
me check what went wrong...

greg k-h

[Greg Kroah-Hartman]

What is the git id of the revert? This ended up already in 4.19.y,
5.4.y, and 5.9.y so needs to be reverted there.

thanks,

greg k-h

[Eric Dumazet]

On Thu, Dec 10, 2020 at 3:40 PM Greg Kroah-Hartman

<gre...@linuxfoundation.org> wrote:
>
> On Thu, Dec 10, 2020 at 03:38:44PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Dec 10, 2020 at 03:32:12PM +0100, Eric Dumazet wrote:
> > > On Thu, Dec 10, 2020 at 3:26 PM Greg Kroah-Hartman
> > > <gre...@linuxfoundation.org> wrote:
> > > >
> > > > From: Eric Dumazet <edum...@google.com>
> > > >
> > > > IP_ECN_decapsulate() and IP6_ECN_decapsulate() assume
> > > > IP header is already pulled.
> > > >
> > > > geneve does not ensure this yet.
> > > >
> > > > Fixing this generically in IP_ECN_decapsulate() and
> > > > IP6_ECN_decapsulate() is not possible, since callers
> > > > pass a pointer that might be freed by pskb_may_pull()
> > > >
> > > > syzbot reported :
> > > >
> > >
> > > Note that we had to revert this patch, so you can either scratp this
> > > backport, or make sure to backport the revert.
> >
> > I'll drop it thanks. Odd I lost the upstream git id on this patch, let
> > me check what went wrong...
>
> What is the git id of the revert? This ended up already in 4.19.y,
> 5.4.y, and 5.9.y so needs to be reverted there.
>

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=c02bd115b1d25931159f89c7d9bf47a30f5d4b41

Thanks !

> thanks,
>
> greg k-h

[Greg Kroah-Hartman]

Thanks, I'll drop the patch from 4.4, 4.9, and 4.14, and queue up this
revert for the other trees now.

greg k-h