* [PATCH net] ipv6/addrconf: fix potential NULL deref in inet6_set_link_af() @ 2020-02-05 16:55 Eric Dumazet 2020-02-06 13:13 ` David Miller 0 siblings, 1 reply; 5+ messages in thread From: Eric Dumazet @ 2020-02-05 16:55 UTC (permalink / raw) To: David S . Miller Cc: netdev, Eric Dumazet, Eric Dumazet, syzbot, Maxim Mikityanskiy __in6_dev_get(dev) called from inet6_set_link_af() can return NULL. The needed check has been recently removed, let's add it back. IPv6: ADDRCONF(NETDEV_CHANGE): lo: link becomes ready general protection fault, probably for non-canonical address 0xdffffc0000000056: 0000 [#1] PREEMPT SMP KASAN KASAN: null-ptr-deref in range [0x00000000000002b0-0x00000000000002b7] CPU: 0 PID: 9698 Comm: syz-executor712 Not tainted 5.5.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:inet6_set_link_af+0x66e/0xae0 net/ipv6/addrconf.c:5733 Code: 38 d0 7f 08 84 c0 0f 85 20 03 00 00 48 8d bb b0 02 00 00 45 0f b6 64 24 04 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 1a 03 00 00 44 89 a3 b0 02 00 RSP: 0018:ffffc90005b06d40 EFLAGS: 00010206 RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff86df39a6 RDX: 0000000000000056 RSI: ffffffff86df3e74 RDI: 00000000000002b0 RBP: ffffc90005b06e70 R08: ffff8880a2ac0380 R09: ffffc90005b06db0 R10: fffff52000b60dbe R11: ffffc90005b06df7 R12: 0000000000000000 R13: 0000000000000000 R14: ffff8880a1fcc424 R15: dffffc0000000000 FS: 0000000000c46880(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000055f0494ca0d0 CR3: 000000009e4ac000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: do_setlink+0x2a9f/0x3720 net/core/rtnetlink.c:2754 rtnl_group_changelink net/core/rtnetlink.c:3103 [inline] __rtnl_newlink+0xdd1/0x1790 net/core/rtnetlink.c:3257 rtnl_newlink+0x69/0xa0 net/core/rtnetlink.c:3377 rtnetlink_rcv_msg+0x45e/0xaf0 net/core/rtnetlink.c:5438 netlink_rcv_skb+0x177/0x450 net/netlink/af_netlink.c:2477 rtnetlink_rcv+0x1d/0x30 net/core/rtnetlink.c:5456 netlink_unicast_kernel net/netlink/af_netlink.c:1302 [inline] netlink_unicast+0x59e/0x7e0 net/netlink/af_netlink.c:1328 netlink_sendmsg+0x91c/0xea0 net/netlink/af_netlink.c:1917 sock_sendmsg_nosec net/socket.c:652 [inline] sock_sendmsg+0xd7/0x130 net/socket.c:672 ____sys_sendmsg+0x753/0x880 net/socket.c:2343 ___sys_sendmsg+0x100/0x170 net/socket.c:2397 __sys_sendmsg+0x105/0x1d0 net/socket.c:2430 __do_sys_sendmsg net/socket.c:2439 [inline] __se_sys_sendmsg net/socket.c:2437 [inline] __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2437 do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x4402e9 Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007fffd62fbcf8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 00000000004402e9 RDX: 0000000000000000 RSI: 0000000020000080 RDI: 0000000000000003 RBP: 00000000006ca018 R08: 0000000000000008 R09: 00000000004002c8 R10: 0000000000000005 R11: 0000000000000246 R12: 0000000000401b70 R13: 0000000000401c00 R14: 0000000000000000 R15: 0000000000000000 Modules linked in: ---[ end trace cfa7664b8fdcdff3 ]--- RIP: 0010:inet6_set_link_af+0x66e/0xae0 net/ipv6/addrconf.c:5733 Code: 38 d0 7f 08 84 c0 0f 85 20 03 00 00 48 8d bb b0 02 00 00 45 0f b6 64 24 04 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 1a 03 00 00 44 89 a3 b0 02 00 RSP: 0018:ffffc90005b06d40 EFLAGS: 00010206 RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff86df39a6 RDX: 0000000000000056 RSI: ffffffff86df3e74 RDI: 00000000000002b0 RBP: ffffc90005b06e70 R08: ffff8880a2ac0380 R09: ffffc90005b06db0 R10: fffff52000b60dbe R11: ffffc90005b06df7 R12: 0000000000000000 R13: 0000000000000000 R14: ffff8880a1fcc424 R15: dffffc0000000000 FS: 0000000000c46880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020000004 CR3: 000000009e4ac000 CR4: 00000000001406e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Fixes: 7dc2bccab0ee ("Validate required parameters in inet6_validate_link_af") Signed-off-by: Eric Dumazet <edumazet@google.com> Bisected-and-reported-by: syzbot <syzkaller@googlegroups.com> Cc: Maxim Mikityanskiy <maximmi@mellanox.com> --- net/ipv6/addrconf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 39d861d0037719ecb02a3341b923d412b5cdc0c1..cb493e15959c4d1bb68cf30f4099a8daa785bb84 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -5718,6 +5718,9 @@ static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla) struct nlattr *tb[IFLA_INET6_MAX + 1]; int err; + if (!idev) + return -EAFNOSUPPORT; + if (nla_parse_nested_deprecated(tb, IFLA_INET6_MAX, nla, NULL, NULL) < 0) BUG(); -- 2.25.0.341.g760bfbb309-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] ipv6/addrconf: fix potential NULL deref in inet6_set_link_af() 2020-02-05 16:55 [PATCH net] ipv6/addrconf: fix potential NULL deref in inet6_set_link_af() Eric Dumazet @ 2020-02-06 13:13 ` David Miller 2020-02-06 15:18 ` Eric Dumazet 0 siblings, 1 reply; 5+ messages in thread From: David Miller @ 2020-02-06 13:13 UTC (permalink / raw) To: edumazet; +Cc: netdev, eric.dumazet, syzkaller, maximmi From: Eric Dumazet <edumazet@google.com> Date: Wed, 5 Feb 2020 08:55:44 -0800 > __in6_dev_get(dev) called from inet6_set_link_af() can return NULL. > > The needed check has been recently removed, let's add it back. I am having trouble understanding this one. When we have a do_setlink operation the flow is that we first validate the AFs and then invoke setlink operations after that validation. do_setlink() { .. err = validate_linkmsg(dev, tb); if (err < 0) return err; .. if (tb[IFLA_AF_SPEC]) { ... err = af_ops->set_link_af(dev, af); if (err < 0) { rcu_read_unlock(); goto errout; } By definition, we only get to ->set_link_af() if there is an IFLA_AF_SPEC nested attribute and if we look at the validation performed by validate_linkmsg() it goes: if (tb[IFLA_AF_SPEC]) { ... if (af_ops->validate_link_af) { err = af_ops->validate_link_af(dev, af); ... And validate_link_af in net/ipv6/addrconf.c clearly does the following: static int inet6_validate_link_af(const struct net_device *dev, const struct nlattr *nla) ... if (dev) { idev = __in6_dev_get(dev); if (!idev) return -EAFNOSUPPORT; } ... It checks the idev and makes sure it is not-NULL. I therefore cannot find a path by which we arrive at inet6_set_link_af with a NULL idev. The above validation code should trap it. Please explain. Thank you. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ipv6/addrconf: fix potential NULL deref in inet6_set_link_af() 2020-02-06 13:13 ` David Miller @ 2020-02-06 15:18 ` Eric Dumazet 2020-02-07 2:50 ` Eric Dumazet 0 siblings, 1 reply; 5+ messages in thread From: Eric Dumazet @ 2020-02-06 15:18 UTC (permalink / raw) To: David Miller; +Cc: netdev, Eric Dumazet, syzbot, maximmi On Thu, Feb 6, 2020 at 5:13 AM David Miller <davem@davemloft.net> wrote: > > From: Eric Dumazet <edumazet@google.com> > Date: Wed, 5 Feb 2020 08:55:44 -0800 > > > __in6_dev_get(dev) called from inet6_set_link_af() can return NULL. > > > > The needed check has been recently removed, let's add it back. > > I am having trouble understanding this one. > > When we have a do_setlink operation the flow is that we first validate > the AFs and then invoke setlink operations after that validation. > > do_setlink() { > .. > err = validate_linkmsg(dev, tb); > if (err < 0) > return err; > .. > if (tb[IFLA_AF_SPEC]) { > ... > err = af_ops->set_link_af(dev, af); > if (err < 0) { > rcu_read_unlock(); > goto errout; > } > > By definition, we only get to ->set_link_af() if there is an > IFLA_AF_SPEC nested attribute and if we look at the validation > performed by validate_linkmsg() it goes: > > if (tb[IFLA_AF_SPEC]) { > ... > if (af_ops->validate_link_af) { > err = af_ops->validate_link_af(dev, af); > ... > > And validate_link_af in net/ipv6/addrconf.c clearly does the > following: > > static int inet6_validate_link_af(const struct net_device *dev, > const struct nlattr *nla) > ... > if (dev) { > idev = __in6_dev_get(dev); > if (!idev) > return -EAFNOSUPPORT; > } > ... > > It checks the idev and makes sure it is not-NULL. > > I therefore cannot find a path by which we arrive at inet6_set_link_af > with a NULL idev. The above validation code should trap it. > > Please explain. > I can give a repro if that helps. (I have to run, I might have more time later) // autogenerated by syzkaller (https://github.com/google/syzkaller) #define _GNU_SOURCE #include <endian.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/syscall.h> #include <sys/types.h> #include <unistd.h> #define BITMASK(bf_off, bf_len) (((1ull << (bf_len)) - 1) << (bf_off)) #define STORE_BY_BITMASK(type, htobe, addr, val, bf_off, bf_len) \ *(type*)(addr) = \ htobe((htobe(*(type*)(addr)) & ~BITMASK((bf_off), (bf_len))) | \ (((type)(val) << (bf_off)) & BITMASK((bf_off), (bf_len)))) uint64_t r[1] = {0xffffffffffffffff}; int main(void) { syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 3ul, 0x32ul, -1, 0); intptr_t res = 0; res = syscall(__NR_socket, 0x10ul, 3ul, 0ul); if (res != -1) r[0] = res; *(uint64_t*)0x20000080 = 0; *(uint32_t*)0x20000088 = 0; *(uint64_t*)0x20000090 = 0x20000200; *(uint64_t*)0x20000200 = 0x20000000; *(uint32_t*)0x20000000 = 0x40; *(uint16_t*)0x20000004 = 0x10; *(uint16_t*)0x20000006 = 0x801; *(uint32_t*)0x20000008 = 0; *(uint32_t*)0x2000000c = 0; *(uint8_t*)0x20000010 = 0; *(uint8_t*)0x20000011 = 0; *(uint16_t*)0x20000012 = 0; *(uint32_t*)0x20000014 = 0; *(uint32_t*)0x20000018 = 0; *(uint32_t*)0x2000001c = 0; *(uint16_t*)0x20000020 = 0x10; STORE_BY_BITMASK(uint16_t, , 0x20000022, 0x1a, 0, 14); STORE_BY_BITMASK(uint16_t, , 0x20000023, 0, 6, 1); STORE_BY_BITMASK(uint16_t, , 0x20000023, 1, 7, 1); *(uint16_t*)0x20000024 = 0xc; STORE_BY_BITMASK(uint16_t, , 0x20000026, 0xa, 0, 14); STORE_BY_BITMASK(uint16_t, , 0x20000027, 0, 6, 1); STORE_BY_BITMASK(uint16_t, , 0x20000027, 1, 7, 1); *(uint16_t*)0x20000028 = 5; *(uint16_t*)0x2000002a = 8; *(uint8_t*)0x2000002c = 0; *(uint16_t*)0x20000030 = 8; *(uint16_t*)0x20000032 = 0x1b; *(uint32_t*)0x20000034 = 0; *(uint16_t*)0x20000038 = 8; *(uint16_t*)0x2000003a = 4; *(uint32_t*)0x2000003c = 0x3ff; *(uint64_t*)0x20000208 = 0x40; *(uint64_t*)0x20000098 = 1; *(uint64_t*)0x200000a0 = 0; *(uint64_t*)0x200000a8 = 0; *(uint32_t*)0x200000b0 = 0x54; syscall(__NR_sendmsg, r[0], 0x20000080ul, 0ul); return 0; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ipv6/addrconf: fix potential NULL deref in inet6_set_link_af() 2020-02-06 15:18 ` Eric Dumazet @ 2020-02-07 2:50 ` Eric Dumazet 2020-02-07 9:36 ` David Miller 0 siblings, 1 reply; 5+ messages in thread From: Eric Dumazet @ 2020-02-07 2:50 UTC (permalink / raw) To: David Miller; +Cc: netdev, Eric Dumazet, syzbot, maximmi On Thu, Feb 6, 2020 at 7:18 AM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, Feb 6, 2020 at 5:13 AM David Miller <davem@davemloft.net> wrote: > > > > From: Eric Dumazet <edumazet@google.com> > > Date: Wed, 5 Feb 2020 08:55:44 -0800 > > > > > __in6_dev_get(dev) called from inet6_set_link_af() can return NULL. > > > > > > The needed check has been recently removed, let's add it back. > > > > I am having trouble understanding this one. > > > > When we have a do_setlink operation the flow is that we first validate > > the AFs and then invoke setlink operations after that validation. > > > > do_setlink() { > > .. > > err = validate_linkmsg(dev, tb); > > if (err < 0) > > return err; > > .. > > if (tb[IFLA_AF_SPEC]) { > > ... > > err = af_ops->set_link_af(dev, af); > > if (err < 0) { > > rcu_read_unlock(); > > goto errout; > > } > > > > By definition, we only get to ->set_link_af() if there is an > > IFLA_AF_SPEC nested attribute and if we look at the validation > > performed by validate_linkmsg() it goes: > > > > if (tb[IFLA_AF_SPEC]) { > > ... > > if (af_ops->validate_link_af) { > > err = af_ops->validate_link_af(dev, af); > > ... > > > > And validate_link_af in net/ipv6/addrconf.c clearly does the > > following: > > > > static int inet6_validate_link_af(const struct net_device *dev, > > const struct nlattr *nla) > > ... > > if (dev) { > > idev = __in6_dev_get(dev); > > if (!idev) > > return -EAFNOSUPPORT; > > } > > ... > > > > It checks the idev and makes sure it is not-NULL. > > > > I therefore cannot find a path by which we arrive at inet6_set_link_af > > with a NULL idev. The above validation code should trap it. > > > > Please explain. > > > > I can give a repro if that helps. > > (I have to run, I might have more time later) > If I understood the repro well enough, it seems it sets the device MTU to 1023, thus IPV6 is automatically disabled. (as mtu < 1280) do_setlink() ... err = validate_linkmsg(dev, tb); /* OK at this point */ ... if (tb[IFLA_MTU]) { err = dev_set_mtu_ext(dev, nla_get_u32(tb[IFLA_MTU]), extack); if (err < 0) goto errout; status |= DO_SETLINK_MODIFIED; } if (tb[IFLA_AF_SPEC]) { ... err = af_ops->set_link_af(dev, af); ->inet6_set_link_af() // CRASH because idev is NULL Please note that IPv4 is immune to the bug since inet_set_link_af() does : struct in_device *in_dev = __in_dev_get_rcu(dev); if (!in_dev) return -EAFNOSUPPORT; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ipv6/addrconf: fix potential NULL deref in inet6_set_link_af() 2020-02-07 2:50 ` Eric Dumazet @ 2020-02-07 9:36 ` David Miller 0 siblings, 0 replies; 5+ messages in thread From: David Miller @ 2020-02-07 9:36 UTC (permalink / raw) To: edumazet; +Cc: netdev, eric.dumazet, syzkaller, maximmi From: Eric Dumazet <edumazet@google.com> Date: Thu, 6 Feb 2020 18:50:40 -0800 > If I understood the repro well enough, it seems it sets the device MTU > to 1023, thus IPV6 is automatically disabled. (as mtu < 1280) Now I understand. Can you add some text about this to the commit message? Just saying that if the request also lowers the mtu below the ipv6 min mtu then we will see the NULL idev. Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-07 9:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-05 16:55 [PATCH net] ipv6/addrconf: fix potential NULL deref in inet6_set_link_af() Eric Dumazet 2020-02-06 13:13 ` David Miller 2020-02-06 15:18 ` Eric Dumazet 2020-02-07 2:50 ` Eric Dumazet 2020-02-07 9:36 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.