diff mbox series

[net,2/2] bareudp: Fix use of incorrect min_headroom size

Message ID 20201226171308.4226-1-ap420073@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series bareudp: fix several issues | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: willemb@google.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Taehee Yoo Dec. 26, 2020, 5:13 p.m. UTC
In the bareudp6_xmit_skb(), it calculates min_headroom.
At that point, it uses struct iphdr, but it's not correct.
So panic could occur.
The struct ipv6hdr should be used.

Test commands:
    ip netns add A
    ip netns add B
    ip link add veth0 type veth peer name veth1
    ip link set veth0 netns A
    ip netns exec A ip link set veth0 up
    ip netns exec A ip a a 2001:db8:0::1/64 dev veth0
    ip netns exec B ip link set veth1 up
    ip netns exec B ip a a 2001:db8:0::2/64 dev veth1

    for i in {10..1}
    do
            let A=$i-1
            ip netns exec A ip link add bareudp$i type bareudp dstport $i \
		    ethertype 0x86dd
            ip netns exec A ip link set bareudp$i up
            ip netns exec A ip -6 a a 2001:db8:$i::1/64 dev bareudp$i
            ip netns exec A ip -6 r a 2001:db8:$i::2 encap ip6 src \
		    2001:db8:$A::1 dst 2001:db8:$A::2 via 2001:db8:$i::2 \
		    dev bareudp$i

            ip netns exec B ip link add bareudp$i type bareudp dstport $i \
		    ethertype 0x86dd
            ip netns exec B ip link set bareudp$i up
            ip netns exec B ip -6 a a 2001:db8:$i::2/64 dev bareudp$i
            ip netns exec B ip -6 r a 2001:db8:$i::1 encap ip6 src \
		    2001:db8:$A::2 dst 2001:db8:$A::1 via 2001:db8:$i::1 \
		    dev bareudp$i
    done
    ip netns exec A ping 2001:db8:7::2

Splat looks like:
[   66.436679][    C2] skbuff: skb_under_panic: text:ffffffff928614c8 len:454 put:14 head:ffff88810abb4000 data:ffff88810abb3ffa tail:0x1c0 end:0x3ec0 dev:veth0
[   66.441626][    C2] ------------[ cut here ]------------
[   66.443458][    C2] kernel BUG at net/core/skbuff.c:109!
[   66.445313][    C2] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
[   66.447606][    C2] CPU: 2 PID: 913 Comm: ping Not tainted 5.10.0+ #819
[   66.450251][    C2] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[   66.453713][    C2] RIP: 0010:skb_panic+0x15d/0x15f
[   66.455345][    C2] Code: 98 fe 4c 8b 4c 24 10 53 8b 4d 70 45 89 e0 48 c7 c7 60 8b 78 93 41 57 41 56 41 55 48 8b 54 24 20 48 8b 74 24 28 e8 b5 40 f9 ff <0f> 0b 48 8b 6c 24 20 89 34 24 e8 08 c9 98 fe 8b 34 24 48 c7 c1 80
[   66.462314][    C2] RSP: 0018:ffff888119209648 EFLAGS: 00010286
[   66.464281][    C2] RAX: 0000000000000089 RBX: ffff888003159000 RCX: 0000000000000000
[   66.467216][    C2] RDX: 0000000000000089 RSI: 0000000000000008 RDI: ffffed10232412c0
[   66.469768][    C2] RBP: ffff88810a53d440 R08: ffffed102328018d R09: ffffed102328018d
[   66.472297][    C2] R10: ffff888119400c67 R11: ffffed102328018c R12: 000000000000000e
[   66.474833][    C2] R13: ffff88810abb3ffa R14: 00000000000001c0 R15: 0000000000003ec0
[   66.477361][    C2] FS:  00007f37c0c72f00(0000) GS:ffff888119200000(0000) knlGS:0000000000000000
[   66.480214][    C2] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   66.482296][    C2] CR2: 000055a058808570 CR3: 000000011039e002 CR4: 00000000003706e0
[   66.484811][    C2] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   66.487793][    C2] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   66.490424][    C2] Call Trace:
[   66.491469][    C2]  <IRQ>
[   66.492374][    C2]  ? eth_header+0x28/0x190
[   66.494054][    C2]  ? eth_header+0x28/0x190
[   66.495401][    C2]  skb_push.cold.99+0x22/0x22
[   66.496700][    C2]  eth_header+0x28/0x190
[   66.497867][    C2]  neigh_resolve_output+0x3de/0x720
[   66.499615][    C2]  ? __neigh_update+0x7e8/0x20a0
[   66.501176][    C2]  __neigh_update+0x8bd/0x20a0
[   66.502749][    C2]  ndisc_update+0x34/0xc0
[   66.504010][    C2]  ndisc_recv_na+0x8da/0xb80
[   66.505041][    C2]  ? pndisc_redo+0x20/0x20
[   66.505888][    C2]  ? rcu_read_lock_sched_held+0xc0/0xc0
[   66.506965][    C2]  ndisc_rcv+0x3a0/0x470
[   66.507797][    C2]  icmpv6_rcv+0xad9/0x1b00
[   66.508645][    C2]  ip6_protocol_deliver_rcu+0xcd6/0x1560
[   66.509719][    C2]  ip6_input_finish+0x5b/0xf0
[   66.510615][    C2]  ip6_input+0xcd/0x2d0
[   66.511406][    C2]  ? ip6_input_finish+0xf0/0xf0
[   66.512327][    C2]  ? rcu_read_lock_held+0x91/0xa0
[   66.513279][    C2]  ? ip6_protocol_deliver_rcu+0x1560/0x1560
[   66.514414][    C2]  ipv6_rcv+0xe8/0x300
[ ... ]

Fixes: 571912c69f0e ("net: UDP tunnel encapsulation module for tunnelling different protocols like MPLS, IP, NSH etc.")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 drivers/net/bareudp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Guillaume Nault Dec. 27, 2020, 4:33 p.m. UTC | #1
On Sat, Dec 26, 2020 at 05:13:08PM +0000, Taehee Yoo wrote:
> In the bareudp6_xmit_skb(), it calculates min_headroom.
> At that point, it uses struct iphdr, but it's not correct.
> So panic could occur.
> The struct ipv6hdr should be used.
> 
> Test commands:
>     ip netns add A
>     ip netns add B
>     ip link add veth0 type veth peer name veth1
>     ip link set veth0 netns A

Missing "ip link set veth1 netns B", so the reproducer unfortunately
doesn't work.

BTW, you can also simplify the script by creating the veth devices
directly in the right netns:
  ip link add name veth0 netns A type veth peer name veth1 netns B

Apart from that,
Acked-by: Guillaume Nault <gnault@redhat.com>

And thanks a lot for the reproducers!

>     ip netns exec A ip link set veth0 up
>     ip netns exec A ip a a 2001:db8:0::1/64 dev veth0
>     ip netns exec B ip link set veth1 up
>     ip netns exec B ip a a 2001:db8:0::2/64 dev veth1
> 
>     for i in {10..1}
>     do
>             let A=$i-1
>             ip netns exec A ip link add bareudp$i type bareudp dstport $i \
> 		    ethertype 0x86dd
>             ip netns exec A ip link set bareudp$i up
>             ip netns exec A ip -6 a a 2001:db8:$i::1/64 dev bareudp$i
>             ip netns exec A ip -6 r a 2001:db8:$i::2 encap ip6 src \
> 		    2001:db8:$A::1 dst 2001:db8:$A::2 via 2001:db8:$i::2 \
> 		    dev bareudp$i
> 
>             ip netns exec B ip link add bareudp$i type bareudp dstport $i \
> 		    ethertype 0x86dd
>             ip netns exec B ip link set bareudp$i up
>             ip netns exec B ip -6 a a 2001:db8:$i::2/64 dev bareudp$i
>             ip netns exec B ip -6 r a 2001:db8:$i::1 encap ip6 src \
> 		    2001:db8:$A::2 dst 2001:db8:$A::1 via 2001:db8:$i::1 \
> 		    dev bareudp$i
>     done
>     ip netns exec A ping 2001:db8:7::2
Taehee Yoo Dec. 28, 2020, 2:26 a.m. UTC | #2
On Mon, 28 Dec 2020 at 01:33, Guillaume Nault <gnault@redhat.com> wrote:
>

Hi Guillaume,
Thank you for the review!

> On Sat, Dec 26, 2020 at 05:13:08PM +0000, Taehee Yoo wrote:
> > In the bareudp6_xmit_skb(), it calculates min_headroom.
> > At that point, it uses struct iphdr, but it's not correct.
> > So panic could occur.
> > The struct ipv6hdr should be used.
> >
> > Test commands:
> >     ip netns add A
> >     ip netns add B
> >     ip link add veth0 type veth peer name veth1
> >     ip link set veth0 netns A
>
> Missing "ip link set veth1 netns B", so the reproducer unfortunately
> doesn't work.
>

Sorry, This is my mistake.

> BTW, you can also simplify the script by creating the veth devices
> directly in the right netns:
>   ip link add name veth0 netns A type veth peer name veth1 netns B
>

Thanks, It's really useful to me :)

> Apart from that,
> Acked-by: Guillaume Nault <gnault@redhat.com>
>
> And thanks a lot for the reproducers!

Thank you so much for the review,
I will send a v2 patch, which will fix a reproducer script.
diff mbox series

Patch

diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
index aea10196c222..708171c0d628 100644
--- a/drivers/net/bareudp.c
+++ b/drivers/net/bareudp.c
@@ -380,7 +380,7 @@  static int bareudp6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 		goto free_dst;
 
 	min_headroom = LL_RESERVED_SPACE(dst->dev) + dst->header_len +
-		BAREUDP_BASE_HLEN + info->options_len + sizeof(struct iphdr);
+		BAREUDP_BASE_HLEN + info->options_len + sizeof(struct ipv6hdr);
 
 	err = skb_cow_head(skb, min_headroom);
 	if (unlikely(err))