[PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Eric Dumazet]

Commit da71577545a5 ("rtnetlink: Disallow FDB configuration
for non-Ethernet device") added a test against dev->type.

kmsan was still able to trigger a kernel-infoleak using a gre device,
with a correct device type (ARPHRD_ETHER), but with a not
correct dev->addr_len (4 bytes instead of the expected 6 bytes)

BUG: KMSAN: kernel-infoleak in copyout lib/iov_iter.c:143 [inline]
BUG: KMSAN: kernel-infoleak in _copy_to_iter+0x4c0/0x2700 lib/iov_iter.c:576
CPU: 0 PID: 6697 Comm: syz-executor310 Not tainted 4.20.0-rc3+ #95
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x32d/0x480 lib/dump_stack.c:113
 kmsan_report+0x12c/0x290 mm/kmsan/kmsan.c:683
 kmsan_internal_check_memory+0x32a/0xa50 mm/kmsan/kmsan.c:743
 kmsan_copy_to_user+0x78/0xd0 mm/kmsan/kmsan_hooks.c:634
 copyout lib/iov_iter.c:143 [inline]
 _copy_to_iter+0x4c0/0x2700 lib/iov_iter.c:576
 copy_to_iter include/linux/uio.h:143 [inline]
 skb_copy_datagram_iter+0x4e2/0x1070 net/core/datagram.c:431
 skb_copy_datagram_msg include/linux/skbuff.h:3316 [inline]
 netlink_recvmsg+0x6f9/0x19d0 net/netlink/af_netlink.c:1975
 sock_recvmsg_nosec net/socket.c:794 [inline]
 sock_recvmsg+0x1d1/0x230 net/socket.c:801
 ___sys_recvmsg+0x444/0xae0 net/socket.c:2278
 __sys_recvmsg net/socket.c:2327 [inline]
 __do_sys_recvmsg net/socket.c:2337 [inline]
 __se_sys_recvmsg+0x2fa/0x450 net/socket.c:2334
 __x64_sys_recvmsg+0x4a/0x70 net/socket.c:2334
 do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x441119
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 db 0a fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fffc7f008a8 EFLAGS: 00000207 ORIG_RAX: 000000000000002f
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000441119
RDX: 0000000000000040 RSI: 00000000200005c0 RDI: 0000000000000003
RBP: 00000000006cc018 R08: 0000000000000100 R09: 0000000000000100
R10: 0000000000000100 R11: 0000000000000207 R12: 0000000000402080
R13: 0000000000402110 R14: 0000000000000000 R15: 0000000000000000

Uninit was stored to memory at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:246 [inline]
 kmsan_save_stack mm/kmsan/kmsan.c:261 [inline]
 kmsan_internal_chain_origin+0x13d/0x240 mm/kmsan/kmsan.c:469
 kmsan_memcpy_memmove_metadata+0x1a9/0xf70 mm/kmsan/kmsan.c:344
 kmsan_memcpy_metadata+0xb/0x10 mm/kmsan/kmsan.c:362
 __msan_memcpy+0x61/0x70 mm/kmsan/kmsan_instr.c:162
 __nla_put lib/nlattr.c:744 [inline]
 nla_put+0x20a/0x2d0 lib/nlattr.c:802
 nlmsg_populate_fdb_fill+0x444/0x810 net/core/rtnetlink.c:3466
 nlmsg_populate_fdb net/core/rtnetlink.c:3775 [inline]
 ndo_dflt_fdb_dump+0x73a/0x960 net/core/rtnetlink.c:3807
 rtnl_fdb_dump+0x1318/0x1cb0 net/core/rtnetlink.c:3979
 netlink_dump+0xc79/0x1c90 net/netlink/af_netlink.c:2244
 __netlink_dump_start+0x10c4/0x11d0 net/netlink/af_netlink.c:2352
 netlink_dump_start include/linux/netlink.h:216 [inline]
 rtnetlink_rcv_msg+0x141b/0x1540 net/core/rtnetlink.c:4910
 netlink_rcv_skb+0x394/0x640 net/netlink/af_netlink.c:2477
 rtnetlink_rcv+0x50/0x60 net/core/rtnetlink.c:4965
 netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
 netlink_unicast+0x1699/0x1740 net/netlink/af_netlink.c:1336
 netlink_sendmsg+0x13c7/0x1440 net/netlink/af_netlink.c:1917
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe3b/0x1240 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:246 [inline]
 kmsan_internal_poison_shadow+0x6d/0x130 mm/kmsan/kmsan.c:170
 kmsan_kmalloc+0xa1/0x100 mm/kmsan/kmsan_hooks.c:186
 __kmalloc+0x14c/0x4d0 mm/slub.c:3825
 kmalloc include/linux/slab.h:551 [inline]
 __hw_addr_create_ex net/core/dev_addr_lists.c:34 [inline]
 __hw_addr_add_ex net/core/dev_addr_lists.c:80 [inline]
 __dev_mc_add+0x357/0x8a0 net/core/dev_addr_lists.c:670
 dev_mc_add+0x6d/0x80 net/core/dev_addr_lists.c:687
 ip_mc_filter_add net/ipv4/igmp.c:1128 [inline]
 igmp_group_added+0x4d4/0xb80 net/ipv4/igmp.c:1311
 __ip_mc_inc_group+0xea9/0xf70 net/ipv4/igmp.c:1444
 ip_mc_inc_group net/ipv4/igmp.c:1453 [inline]
 ip_mc_up+0x1c3/0x400 net/ipv4/igmp.c:1775
 inetdev_event+0x1d03/0x1d80 net/ipv4/devinet.c:1522
 notifier_call_chain kernel/notifier.c:93 [inline]
 __raw_notifier_call_chain kernel/notifier.c:394 [inline]
 raw_notifier_call_chain+0x13d/0x240 kernel/notifier.c:401
 __dev_notify_flags+0x3da/0x860 net/core/dev.c:1733
 dev_change_flags+0x1ac/0x230 net/core/dev.c:7569
 do_setlink+0x165f/0x5ea0 net/core/rtnetlink.c:2492
 rtnl_newlink+0x2ad7/0x35a0 net/core/rtnetlink.c:3111
 rtnetlink_rcv_msg+0x1148/0x1540 net/core/rtnetlink.c:4947
 netlink_rcv_skb+0x394/0x640 net/netlink/af_netlink.c:2477
 rtnetlink_rcv+0x50/0x60 net/core/rtnetlink.c:4965
 netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
 netlink_unicast+0x1699/0x1740 net/netlink/af_netlink.c:1336
 netlink_sendmsg+0x13c7/0x1440 net/netlink/af_netlink.c:1917
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe3b/0x1240 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7

Bytes 36-37 of 105 are uninitialized
Memory access of size 105 starts at ffff88819686c000
Data copied to user address 0000000020000380

Fixes: da71577545a5 ("rtnetlink: Disallow FDB configuration for non-Ethernet device")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Ido Schimmel <idosch@mellanox.com>
Cc: David Ahern <dsahern@gmail.com>
---
 net/core/rtnetlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 33d9227a8b8077a8cf6edbcaaa9f5b92d4fee48e..54cfbda0b58be34dcb164028f17cdde2826c857b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3600,7 +3600,7 @@ static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 	}
 
-	if (dev->type != ARPHRD_ETHER) {
+	if (dev->type != ARPHRD_ETHER || dev->addr_len != ETH_ALEN) {
 		NL_SET_ERR_MSG(extack, "FDB add only supported for Ethernet devices");
 		return -EINVAL;
 	}
@@ -3709,7 +3709,7 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 	}
 
-	if (dev->type != ARPHRD_ETHER) {
+	if (dev->type != ARPHRD_ETHER || dev->addr_len != ETH_ALEN) {
 		NL_SET_ERR_MSG(extack, "FDB delete only supported for Ethernet devices");
 		return -EINVAL;
 	}
-- 
2.20.0.rc1.387.gf8505762e3-goog

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Ido Schimmel]

On Fri, Nov 30, 2018 at 05:35:01AM -0800, 'Eric Dumazet' via syzkaller wrote:
> Commit da71577545a5 ("rtnetlink: Disallow FDB configuration
> for non-Ethernet device") added a test against dev->type.
> 
> kmsan was still able to trigger a kernel-infoleak using a gre device,
> with a correct device type (ARPHRD_ETHER), but with a not
> correct dev->addr_len (4 bytes instead of the expected 6 bytes)

Hi,

Can you please share the reproducer (assuming it exists)? I don't really
understand the fix. None of the functions you patched are in the trace.
Also, looking at IPv4 GRE code, while GRE device has dev->addr_len set
to 4, dev->type is set to ARPHRD_IPGRE.

Thanks

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Eric Dumazet]

On Fri, Nov 30, 2018 at 6:58 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> On Fri, Nov 30, 2018 at 05:35:01AM -0800, 'Eric Dumazet' via syzkaller wrote:
> > Commit da71577545a5 ("rtnetlink: Disallow FDB configuration
> > for non-Ethernet device") added a test against dev->type.
> >
> > kmsan was still able to trigger a kernel-infoleak using a gre device,
> > with a correct device type (ARPHRD_ETHER), but with a not
> > correct dev->addr_len (4 bytes instead of the expected 6 bytes)
>
> Hi,
>
> Can you please share the reproducer (assuming it exists)? I don't really
> understand the fix. None of the functions you patched are in the trace.
> Also, looking at IPv4 GRE code, while GRE device has dev->addr_len set
> to 4, dev->type is set to ARPHRD_IPGRE.
>
> Thanks

// autogenerated by syzkaller (https://github.com/google/syzkaller)

#define _GNU_SOURCE

#include <arpa/inet.h>
#include <endian.h>
#include <errno.h>
#include <fcntl.h>
#include <net/if_arp.h>
#include <sched.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/mount.h>
#include <sys/prctl.h>
#include <sys/resource.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/time.h>
#include <sys/types.h>
#include <sys/uio.h>
#include <sys/wait.h>
#include <unistd.h>

#include <linux/if.h>
#include <linux/if_ether.h>
#include <linux/if_tun.h>
#include <linux/ip.h>
#include <linux/tcp.h>

static void vsnprintf_check(char* str, size_t size, const char* format,
                            va_list args)
{
  int rv;
  rv = vsnprintf(str, size, format, args);
  if (rv < 0)
    exit(1);
  if ((size_t)rv >= size)
    exit(1);
}

#define COMMAND_MAX_LEN 128
#define PATH_PREFIX                                                            \
  "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin "
#define PATH_PREFIX_LEN (sizeof(PATH_PREFIX) - 1)

static void execute_command(bool panic, const char* format, ...)
{
  va_list args;
  char command[PATH_PREFIX_LEN + COMMAND_MAX_LEN];
  int rv;
  va_start(args, format);
  memcpy(command, PATH_PREFIX, PATH_PREFIX_LEN);
  vsnprintf_check(command + PATH_PREFIX_LEN, COMMAND_MAX_LEN, format, args);
  va_end(args);
  rv = system(command);
  if (rv) {
    if (panic)
      exit(1);
  }
}

#define DEV_IPV4 "172.20.20.%d"
#define DEV_IPV6 "fe80::%02hx"
#define DEV_MAC "aa:aa:aa:aa:aa:%02hx"

static void snprintf_check(char* str, size_t size, const char* format, ...)
{
  va_list args;
  va_start(args, format);
  vsnprintf_check(str, size, format, args);
  va_end(args);
}
static void initialize_netdevices(void)
{
  unsigned i;
  const char* devtypes[] = {"ip6gretap", "bridge", "vcan", "bond", "team"};
  const char* devnames[] = {"lo",
                            "sit0",
                            "bridge0",
                            "vcan0",
                            "tunl0",
                            "gre0",
                            "gretap0",
                            "ip_vti0",
                            "ip6_vti0",
                            "ip6tnl0",
                            "ip6gre0",
                            "ip6gretap0",
                            "erspan0",
                            "bond0",
                            "veth0",
                            "veth1",
                            "team0",
                            "veth0_to_bridge",
                            "veth1_to_bridge",
                            "veth0_to_bond",
                            "veth1_to_bond",
                            "veth0_to_team",
                            "veth1_to_team"};
  const char* devmasters[] = {"bridge", "bond", "team"};
  for (i = 0; i < sizeof(devtypes) / (sizeof(devtypes[0])); i++)
    execute_command(0, "ip link add dev %s0 type %s", devtypes[i], devtypes[i]);
  execute_command(0, "ip link add type veth");
  for (i = 0; i < sizeof(devmasters) / (sizeof(devmasters[0])); i++) {
    execute_command(
        0, "ip link add name %s_slave_0 type veth peer name veth0_to_%s",
        devmasters[i], devmasters[i]);
    execute_command(
        0, "ip link add name %s_slave_1 type veth peer name veth1_to_%s",
        devmasters[i], devmasters[i]);
    execute_command(0, "ip link set %s_slave_0 master %s0", devmasters[i],
                    devmasters[i]);
    execute_command(0, "ip link set %s_slave_1 master %s0", devmasters[i],
                    devmasters[i]);
    execute_command(0, "ip link set veth0_to_%s up", devmasters[i]);
    execute_command(0, "ip link set veth1_to_%s up", devmasters[i]);
  }
  execute_command(0, "ip link set bridge_slave_0 up");
  execute_command(0, "ip link set bridge_slave_1 up");
  for (i = 0; i < sizeof(devnames) / (sizeof(devnames[0])); i++) {
    char addr[32];
    snprintf_check(addr, sizeof(addr), DEV_IPV4, i + 10);
    execute_command(0, "ip -4 addr add %s/24 dev %s", addr, devnames[i]);
    snprintf_check(addr, sizeof(addr), DEV_IPV6, i + 10);
    execute_command(0, "ip -6 addr add %s/120 dev %s", addr, devnames[i]);
    snprintf_check(addr, sizeof(addr), DEV_MAC, i + 10);
    execute_command(0, "ip link set dev %s address %s", devnames[i], addr);
    execute_command(0, "ip link set dev %s up", devnames[i]);
  }
}

static void setup_common()
{
  if (mount(0, "/sys/fs/fuse/connections", "fusectl", 0, 0)) {
  }
}

static void loop();

static void sandbox_common()
{
  prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
  setpgrp();
  setsid();
  struct rlimit rlim;
  rlim.rlim_cur = rlim.rlim_max = 200 << 20;
  setrlimit(RLIMIT_AS, &rlim);
  rlim.rlim_cur = rlim.rlim_max = 32 << 20;
  setrlimit(RLIMIT_MEMLOCK, &rlim);
  rlim.rlim_cur = rlim.rlim_max = 136 << 20;
  setrlimit(RLIMIT_FSIZE, &rlim);
  rlim.rlim_cur = rlim.rlim_max = 1 << 20;
  setrlimit(RLIMIT_STACK, &rlim);
  rlim.rlim_cur = rlim.rlim_max = 0;
  setrlimit(RLIMIT_CORE, &rlim);
  rlim.rlim_cur = rlim.rlim_max = 256;
  setrlimit(RLIMIT_NOFILE, &rlim);
  if (unshare(CLONE_NEWNS)) {
  }
  if (unshare(CLONE_NEWIPC)) {
  }
  if (unshare(0x02000000)) {
  }
  if (unshare(CLONE_NEWUTS)) {
  }
  if (unshare(CLONE_SYSVSEM)) {
  }
}

int wait_for_loop(int pid)
{
  if (pid < 0)
    exit(1);
  int status = 0;
  while (waitpid(-1, &status, __WALL) != pid) {
  }
  return WEXITSTATUS(status);
}

static int do_sandbox_none(void)
{
  if (unshare(CLONE_NEWPID)) {
  }
  int pid = fork();
  if (pid != 0)
    return wait_for_loop(pid);
  setup_common();
  sandbox_common();
  if (unshare(CLONE_NEWNET)) {
  }
  initialize_netdevices();
  loop();
  exit(1);
}

uint64_t r[1] = {0xffffffffffffffff};

void loop(void)
{
  long res = 0;
  res = syscall(__NR_socket, 0x10, 0x100000002, 0);
  if (res != -1)
    r[0] = res;
  *(uint64_t*)0x20000080 = 0;
  *(uint32_t*)0x20000088 = 0;
  *(uint64_t*)0x20000090 = 0x20000000;
  *(uint64_t*)0x20000000 = 0x200000c0;
  memcpy((void*)0x200000c0, "\x23\x00\x00\x00\x1e\x00\x81\xae\xe4\x05\x0c\x00"
                            "\x00\x0f\x00\xfe\x07\x01\x01\x00\x00\x00\x00\x00"
                            "\x63\xda\xc3\x7b\x74\x03\x24\x21\x89\xc6\x09",
         35);
  *(uint64_t*)0x20000008 = 0x23;
  *(uint64_t*)0x20000098 = 1;
  *(uint64_t*)0x200000a0 = 0;
  *(uint64_t*)0x200000a8 = 0;
  *(uint32_t*)0x200000b0 = 0;
  syscall(__NR_sendmsg, r[0], 0x20000080, 0);
  *(uint64_t*)0x200005c0 = 0;
  *(uint32_t*)0x200005c8 = 0;
  *(uint64_t*)0x200005d0 = 0x20000040;
  *(uint64_t*)0x20000040 = 0x20000380;
  *(uint64_t*)0x20000048 = 0x69;
  *(uint64_t*)0x20000050 = 0;
  *(uint64_t*)0x20000058 = 0;
  *(uint64_t*)0x200005d8 = 2;
  *(uint64_t*)0x200005e0 = 0;
  *(uint64_t*)0x200005e8 = 0;
  *(uint32_t*)0x200005f0 = 0;
  syscall(__NR_recvmsg, r[0], 0x200005c0, 0x40);
}
int main(void)
{
  syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
  do_sandbox_none();
  return 0;
}

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[David Ahern]

On 11/30/18 7:58 AM, Ido Schimmel wrote:
> Can you please share the reproducer (assuming it exists)? I don't really
> understand the fix. None of the functions you patched are in the trace.
> Also, looking at IPv4 GRE code, while GRE device has dev->addr_len set
> to 4, dev->type is set to ARPHRD_IPGRE.

I had the same reaction ... you can not claim to be an ethernet device
and have a hw address that is not 6 bytes.

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Eric Dumazet]

On Fri, Nov 30, 2018 at 7:36 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 11/30/18 7:58 AM, Ido Schimmel wrote:
> > Can you please share the reproducer (assuming it exists)? I don't really
> > understand the fix. None of the functions you patched are in the trace.
> > Also, looking at IPv4 GRE code, while GRE device has dev->addr_len set
> > to 4, dev->type is set to ARPHRD_IPGRE.
>
> I had the same reaction ... you can not claim to be an ethernet device
> and have a hw address that is not 6 bytes.

This has been discussed a number of times.... TUNSETLINK can do that.
(I have not checked what the repro does )

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Eric Dumazet]

On Fri, Nov 30, 2018 at 7:40 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Nov 30, 2018 at 7:36 AM David Ahern <dsahern@gmail.com> wrote:
> >
> > On 11/30/18 7:58 AM, Ido Schimmel wrote:
> > > Can you please share the reproducer (assuming it exists)? I don't really
> > > understand the fix. None of the functions you patched are in the trace.
> > > Also, looking at IPv4 GRE code, while GRE device has dev->addr_len set
> > > to 4, dev->type is set to ARPHRD_IPGRE.
> >
> > I had the same reaction ... you can not claim to be an ethernet device
> > and have a hw address that is not 6 bytes.
>
> This has been discussed a number of times.... TUNSETLINK can do that.
> (I have not checked what the repro does )


Oh well...ndo_dflt_fdb_dump() seems to be used on a gre device.

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Eric Dumazet]

On Fri, Nov 30, 2018 at 7:46 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Nov 30, 2018 at 7:40 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 7:36 AM David Ahern <dsahern@gmail.com> wrote:
> > >
> > > On 11/30/18 7:58 AM, Ido Schimmel wrote:
> > > > Can you please share the reproducer (assuming it exists)? I don't really
> > > > understand the fix. None of the functions you patched are in the trace.
> > > > Also, looking at IPv4 GRE code, while GRE device has dev->addr_len set
> > > > to 4, dev->type is set to ARPHRD_IPGRE.
> > >
> > > I had the same reaction ... you can not claim to be an ethernet device
> > > and have a hw address that is not 6 bytes.
> >
> > This has been discussed a number of times.... TUNSETLINK can do that.
> > (I have not checked what the repro does )
>
>
> Oh well...ndo_dflt_fdb_dump() seems to be used on a gre device.

What about :

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 54cfbda0b58be34dcb164028f17cdde2826c857b..b4cfc139c8b05b19564e02f29bd030c5ff85b51b
100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3800,6 +3800,9 @@ int ndo_dflt_fdb_dump(struct sk_buff *skb,
 {
        int err;

+       if (dev->type != ARPHRD_ETHER || dev->addr_len != ETH_ALEN)
+               return -EINVAL;
+
        netif_addr_lock_bh(dev);
        err = nlmsg_populate_fdb(skb, cb, dev, idx, &dev->uc);
        if (err)

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[David Ahern]

On 11/30/18 8:14 AM, Eric Dumazet wrote:
> 
> // autogenerated by syzkaller (https://github.com/google/syzkaller)
> 
> #define _GNU_SOURCE
> 
> #include <arpa/inet.h>
> #include <endian.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <net/if_arp.h>
> #include <sched.h>
> #include <stdarg.h>
> #include <stdbool.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/ioctl.h>
> #include <sys/mount.h>
> #include <sys/prctl.h>
> #include <sys/resource.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/time.h>
> #include <sys/types.h>
> #include <sys/uio.h>
> #include <sys/wait.h>
> #include <unistd.h>
> 
> #include <linux/if.h>
> #include <linux/if_ether.h>
> #include <linux/if_tun.h>
> #include <linux/ip.h>
> #include <linux/tcp.h>
> 
> static void vsnprintf_check(char* str, size_t size, const char* format,
>                             va_list args)
> {
>   int rv;
>   rv = vsnprintf(str, size, format, args);
>   if (rv < 0)
>     exit(1);
>   if ((size_t)rv >= size)
>     exit(1);
> }
> 
> #define COMMAND_MAX_LEN 128
> #define PATH_PREFIX                                                            \
>   "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin "
> #define PATH_PREFIX_LEN (sizeof(PATH_PREFIX) - 1)
> 
> static void execute_command(bool panic, const char* format, ...)
> {
>   va_list args;
>   char command[PATH_PREFIX_LEN + COMMAND_MAX_LEN];
>   int rv;
>   va_start(args, format);
>   memcpy(command, PATH_PREFIX, PATH_PREFIX_LEN);
>   vsnprintf_check(command + PATH_PREFIX_LEN, COMMAND_MAX_LEN, format, args);
>   va_end(args);
>   rv = system(command);
>   if (rv) {
>     if (panic)
>       exit(1);
>   }
> }
> 
> #define DEV_IPV4 "172.20.20.%d"
> #define DEV_IPV6 "fe80::%02hx"
> #define DEV_MAC "aa:aa:aa:aa:aa:%02hx"
> 
> static void snprintf_check(char* str, size_t size, const char* format, ...)
> {
>   va_list args;
>   va_start(args, format);
>   vsnprintf_check(str, size, format, args);
>   va_end(args);
> }
> static void initialize_netdevices(void)
> {
>   unsigned i;
>   const char* devtypes[] = {"ip6gretap", "bridge", "vcan", "bond", "team"};
>   const char* devnames[] = {"lo",
>                             "sit0",
>                             "bridge0",
>                             "vcan0",
>                             "tunl0",
>                             "gre0",
>                             "gretap0",
>                             "ip_vti0",
>                             "ip6_vti0",
>                             "ip6tnl0",
>                             "ip6gre0",
>                             "ip6gretap0",
>                             "erspan0",
>                             "bond0",
>                             "veth0",
>                             "veth1",
>                             "team0",
>                             "veth0_to_bridge",
>                             "veth1_to_bridge",
>                             "veth0_to_bond",
>                             "veth1_to_bond",
>                             "veth0_to_team",
>                             "veth1_to_team"};
>   const char* devmasters[] = {"bridge", "bond", "team"};
>   for (i = 0; i < sizeof(devtypes) / (sizeof(devtypes[0])); i++)
>     execute_command(0, "ip link add dev %s0 type %s", devtypes[i], devtypes[i]);
>   execute_command(0, "ip link add type veth");
>   for (i = 0; i < sizeof(devmasters) / (sizeof(devmasters[0])); i++) {
>     execute_command(
>         0, "ip link add name %s_slave_0 type veth peer name veth0_to_%s",
>         devmasters[i], devmasters[i]);
>     execute_command(
>         0, "ip link add name %s_slave_1 type veth peer name veth1_to_%s",
>         devmasters[i], devmasters[i]);
>     execute_command(0, "ip link set %s_slave_0 master %s0", devmasters[i],
>                     devmasters[i]);
>     execute_command(0, "ip link set %s_slave_1 master %s0", devmasters[i],
>                     devmasters[i]);
>     execute_command(0, "ip link set veth0_to_%s up", devmasters[i]);
>     execute_command(0, "ip link set veth1_to_%s up", devmasters[i]);
>   }
>   execute_command(0, "ip link set bridge_slave_0 up");
>   execute_command(0, "ip link set bridge_slave_1 up");
>   for (i = 0; i < sizeof(devnames) / (sizeof(devnames[0])); i++) {
>     char addr[32];
>     snprintf_check(addr, sizeof(addr), DEV_IPV4, i + 10);
>     execute_command(0, "ip -4 addr add %s/24 dev %s", addr, devnames[i]);
>     snprintf_check(addr, sizeof(addr), DEV_IPV6, i + 10);
>     execute_command(0, "ip -6 addr add %s/120 dev %s", addr, devnames[i]);
>     snprintf_check(addr, sizeof(addr), DEV_MAC, i + 10);
>     execute_command(0, "ip link set dev %s address %s", devnames[i], addr);
>     execute_command(0, "ip link set dev %s up", devnames[i]);
>   }
> }
> 
> static void setup_common()
> {
>   if (mount(0, "/sys/fs/fuse/connections", "fusectl", 0, 0)) {
>   }
> }
> 
> static void loop();
> 
> static void sandbox_common()
> {
>   prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
>   setpgrp();
>   setsid();
>   struct rlimit rlim;
>   rlim.rlim_cur = rlim.rlim_max = 200 << 20;
>   setrlimit(RLIMIT_AS, &rlim);
>   rlim.rlim_cur = rlim.rlim_max = 32 << 20;
>   setrlimit(RLIMIT_MEMLOCK, &rlim);
>   rlim.rlim_cur = rlim.rlim_max = 136 << 20;
>   setrlimit(RLIMIT_FSIZE, &rlim);
>   rlim.rlim_cur = rlim.rlim_max = 1 << 20;
>   setrlimit(RLIMIT_STACK, &rlim);
>   rlim.rlim_cur = rlim.rlim_max = 0;
>   setrlimit(RLIMIT_CORE, &rlim);
>   rlim.rlim_cur = rlim.rlim_max = 256;
>   setrlimit(RLIMIT_NOFILE, &rlim);
>   if (unshare(CLONE_NEWNS)) {
>   }
>   if (unshare(CLONE_NEWIPC)) {
>   }
>   if (unshare(0x02000000)) {
>   }
>   if (unshare(CLONE_NEWUTS)) {
>   }
>   if (unshare(CLONE_SYSVSEM)) {
>   }
> }
> 
> int wait_for_loop(int pid)
> {
>   if (pid < 0)
>     exit(1);
>   int status = 0;
>   while (waitpid(-1, &status, __WALL) != pid) {
>   }
>   return WEXITSTATUS(status);
> }
> 
> static int do_sandbox_none(void)
> {
>   if (unshare(CLONE_NEWPID)) {
>   }
>   int pid = fork();
>   if (pid != 0)
>     return wait_for_loop(pid);
>   setup_common();
>   sandbox_common();
>   if (unshare(CLONE_NEWNET)) {
>   }
>   initialize_netdevices();
>   loop();
>   exit(1);
> }
> 
> uint64_t r[1] = {0xffffffffffffffff};
> 
> void loop(void)
> {
>   long res = 0;
>   res = syscall(__NR_socket, 0x10, 0x100000002, 0);
>   if (res != -1)
>     r[0] = res;
>   *(uint64_t*)0x20000080 = 0;
>   *(uint32_t*)0x20000088 = 0;
>   *(uint64_t*)0x20000090 = 0x20000000;
>   *(uint64_t*)0x20000000 = 0x200000c0;
>   memcpy((void*)0x200000c0, "\x23\x00\x00\x00\x1e\x00\x81\xae\xe4\x05\x0c\x00"
>                             "\x00\x0f\x00\xfe\x07\x01\x01\x00\x00\x00\x00\x00"
>                             "\x63\xda\xc3\x7b\x74\x03\x24\x21\x89\xc6\x09",
>          35);
>   *(uint64_t*)0x20000008 = 0x23;
>   *(uint64_t*)0x20000098 = 1;
>   *(uint64_t*)0x200000a0 = 0;
>   *(uint64_t*)0x200000a8 = 0;
>   *(uint32_t*)0x200000b0 = 0;
>   syscall(__NR_sendmsg, r[0], 0x20000080, 0);
>   *(uint64_t*)0x200005c0 = 0;
>   *(uint32_t*)0x200005c8 = 0;
>   *(uint64_t*)0x200005d0 = 0x20000040;
>   *(uint64_t*)0x20000040 = 0x20000380;
>   *(uint64_t*)0x20000048 = 0x69;
>   *(uint64_t*)0x20000050 = 0;
>   *(uint64_t*)0x20000058 = 0;
>   *(uint64_t*)0x200005d8 = 2;
>   *(uint64_t*)0x200005e0 = 0;
>   *(uint64_t*)0x200005e8 = 0;
>   *(uint32_t*)0x200005f0 = 0;
>   syscall(__NR_recvmsg, r[0], 0x200005c0, 0x40);
> }
> int main(void)
> {
>   syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
>   do_sandbox_none();
>   return 0;
> }
> 

This does not repro for me:
# ./a.out
Invalid address length 6 - must be 4 bytes
RTNETLINK answers: No buffer space available
RTNETLINK answers: Operation not supported
Invalid address length 6 - must be 4 bytes
Invalid address length 6 - must be 4 bytes
Invalid address length 6 - must be 4 bytes
Invalid address length 6 - must be 16 bytes
Invalid address length 6 - must be 16 bytes
Invalid address length 6 - must be 16 bytes

config available>?

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Ido Schimmel]

On Fri, Nov 30, 2018 at 07:51:34AM -0800, Eric Dumazet wrote:
> On Fri, Nov 30, 2018 at 7:46 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 7:40 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Fri, Nov 30, 2018 at 7:36 AM David Ahern <dsahern@gmail.com> wrote:
> > > >
> > > > On 11/30/18 7:58 AM, Ido Schimmel wrote:
> > > > > Can you please share the reproducer (assuming it exists)? I don't really
> > > > > understand the fix. None of the functions you patched are in the trace.
> > > > > Also, looking at IPv4 GRE code, while GRE device has dev->addr_len set
> > > > > to 4, dev->type is set to ARPHRD_IPGRE.
> > > >
> > > > I had the same reaction ... you can not claim to be an ethernet device
> > > > and have a hw address that is not 6 bytes.
> > >
> > > This has been discussed a number of times.... TUNSETLINK can do that.
> > > (I have not checked what the repro does )
> >
> >
> > Oh well...ndo_dflt_fdb_dump() seems to be used on a gre device.
> 
> What about :
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 54cfbda0b58be34dcb164028f17cdde2826c857b..b4cfc139c8b05b19564e02f29bd030c5ff85b51b
> 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3800,6 +3800,9 @@ int ndo_dflt_fdb_dump(struct sk_buff *skb,
>  {
>         int err;
> 
> +       if (dev->type != ARPHRD_ETHER || dev->addr_len != ETH_ALEN)
> +               return -EINVAL;

This makes more sense. I'm still compiling the kmsan kernel. I'll be
able to test the patch after compilation is done.

> +
>         netif_addr_lock_bh(dev);
>         err = nlmsg_populate_fdb(skb, cb, dev, idx, &dev->uc);
>         if (err)

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Ido Schimmel]

On Fri, Nov 30, 2018 at 08:59:09AM -0700, David Ahern wrote:
> This does not repro for me:
> # ./a.out
> Invalid address length 6 - must be 4 bytes
> RTNETLINK answers: No buffer space available
> RTNETLINK answers: Operation not supported
> Invalid address length 6 - must be 4 bytes
> Invalid address length 6 - must be 4 bytes
> Invalid address length 6 - must be 4 bytes
> Invalid address length 6 - must be 16 bytes
> Invalid address length 6 - must be 16 bytes
> Invalid address length 6 - must be 16 bytes
> 
> config available>?

You need a kernel with kmsan. See:
https://github.com/google/kmsan

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Dmitry Vyukov]

On Fri, Nov 30, 2018 at 4:02 PM, Ido Schimmel <idosch@idosch.org> wrote:
> On Fri, Nov 30, 2018 at 08:59:09AM -0700, David Ahern wrote:
>> This does not repro for me:
>> # ./a.out
>> Invalid address length 6 - must be 4 bytes
>> RTNETLINK answers: No buffer space available
>> RTNETLINK answers: Operation not supported
>> Invalid address length 6 - must be 4 bytes
>> Invalid address length 6 - must be 4 bytes
>> Invalid address length 6 - must be 4 bytes
>> Invalid address length 6 - must be 16 bytes
>> Invalid address length 6 - must be 16 bytes
>> Invalid address length 6 - must be 16 bytes
>>
>> config available>?
>
> You need a kernel with kmsan. See:
> https://github.com/google/kmsan


Another option may be to spray memory at the allocation stack with
some distinctive byte pattern and then check this pattern at the use
stack. Not 100% sounds, but should be enough for testing.

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Eric Dumazet]

On Fri, Nov 30, 2018 at 8:10 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Fri, Nov 30, 2018 at 4:02 PM, Ido Schimmel <idosch@idosch.org> wrote:
> > On Fri, Nov 30, 2018 at 08:59:09AM -0700, David Ahern wrote:
> >> This does not repro for me:
> >> # ./a.out
> >> Invalid address length 6 - must be 4 bytes
> >> RTNETLINK answers: No buffer space available
> >> RTNETLINK answers: Operation not supported
> >> Invalid address length 6 - must be 4 bytes
> >> Invalid address length 6 - must be 4 bytes
> >> Invalid address length 6 - must be 4 bytes
> >> Invalid address length 6 - must be 16 bytes
> >> Invalid address length 6 - must be 16 bytes
> >> Invalid address length 6 - must be 16 bytes
> >>
> >> config available>?
> >
> > You need a kernel with kmsan. See:
> > https://github.com/google/kmsan
>
>
> Another option may be to spray memory at the allocation stack with
> some distinctive byte pattern and then check this pattern at the use
> stack. Not 100% sounds, but should be enough for testing.

Well, no need for kmsan here, once you know the problem

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a498bb41c9aa9b8976eb8d87f71489695cb019f2..5ce53215d622a670b0dcf06113b01174d49e6efc
100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3471,6 +3471,7 @@ static int nlmsg_populate_fdb_fill(struct sk_buff *skb,
        ndm->ndm_ifindex = dev->ifindex;
        ndm->ndm_state   = ndm_state;

+       WARN_ON_ONCE(dev->addr_len != ETH_ALEN);
        if (nla_put(skb, NDA_LLADDR, ETH_ALEN, addr))
                goto nla_put_failure;
        if (vid)

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Ido Schimmel]

On Fri, Nov 30, 2018 at 08:17:04AM -0800, Eric Dumazet wrote:
> On Fri, Nov 30, 2018 at 8:10 AM Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 4:02 PM, Ido Schimmel <idosch@idosch.org> wrote:
> > > On Fri, Nov 30, 2018 at 08:59:09AM -0700, David Ahern wrote:
> > >> This does not repro for me:
> > >> # ./a.out
> > >> Invalid address length 6 - must be 4 bytes
> > >> RTNETLINK answers: No buffer space available
> > >> RTNETLINK answers: Operation not supported
> > >> Invalid address length 6 - must be 4 bytes
> > >> Invalid address length 6 - must be 4 bytes
> > >> Invalid address length 6 - must be 4 bytes
> > >> Invalid address length 6 - must be 16 bytes
> > >> Invalid address length 6 - must be 16 bytes
> > >> Invalid address length 6 - must be 16 bytes
> > >>
> > >> config available>?
> > >
> > > You need a kernel with kmsan. See:
> > > https://github.com/google/kmsan
> >
> >
> > Another option may be to spray memory at the allocation stack with
> > some distinctive byte pattern and then check this pattern at the use
> > stack. Not 100% sounds, but should be enough for testing.
> 
> Well, no need for kmsan here, once you know the problem

Yes, agree. Patch is good. I'll tag your v2.

Thanks!

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[David Miller]

From: Ido Schimmel <idosch@idosch.org>
Date: Fri, 30 Nov 2018 19:00:24 +0200

> Yes, agree. Patch is good. I'll tag your v2.

This means, I assume, that a new version of this fix is coming.

Eric, is this correct?

Re: [PATCH net] rtnetlink: Refine sanity checks in rtnl_fdb_{add|del}

[Eric Dumazet]

On 12/03/2018 03:52 PM, David Miller wrote:
> From: Ido Schimmel <idosch@idosch.org>
> Date: Fri, 30 Nov 2018 19:00:24 +0200
> 
>> Yes, agree. Patch is good. I'll tag your v2.
> 
> This means, I assume, that a new version of this fix is coming.
> 
> Eric, is this correct?
> 

This is absolutely correct David, I will send a v2 today.