netdev - Re: [Race] data race between dev_ifsioc_locked() and eth_commit_mac_addr

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-prev] [day] [month] [year] [list]

Message-ID: <DE84C07A-ABA3-431E-9ED4-874DCD3055EC@purdue.edu>
Date:   Fri, 22 Jan 2021 04:33:56 +0000
From:   "Gong, Sishuai" <sishuai@...due.edu>
To:     "davem@...emloft.net" <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [Race] data race between dev_ifsioc_locked() and
 eth_commit_mac_addr_change()

Hi,

We found this data race can corrupt the variable ifr->ifr_hwaddr.sa_data as only partially updated, so it should be harmful.

Under the following interleaving, the writer and reader from these 2 memcpy() can interleave with each other on the variable dev->dev_addr. Thus, ifr->ifr_hwaddr.sa_data may end up being only partly updated and passed to userspace as the return value of the syscall.

Thread 1													Thread 2
// eth_commit_mac_addr_change()
memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
														// dev_ifsioc_locked() inside rcu_reader_lock()
														memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr, min(sizeof(ifr->ifr_hwaddr.sa_data),(size_t)dev->addr_len));

Thanks,
Sishuai

> On Nov 30, 2020, at 10:20 AM, Gong, Sishuai <sishuai@...due.edu> wrote:
> 
> Hi,
> 
> We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. Currently, we are not sure about the consequence of this race but it seems that the two memcpy can lead to some inconsistency.
> 
> ------------------------------------------
> Writer site
> 
> /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/net/ethernet/eth.c:307
>        298  /**
>        299   * eth_commit_mac_addr_change - commit mac change
>        300   * @dev: network device
>        301   * @p: socket address
>        302   */
>        303  void eth_commit_mac_addr_change(struct net_device *dev, void *p)
>        304  {
>        305      struct sockaddr *addr = p;
>        306
> ==>    307      memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
>        308  }
> 
> ------------------------------------------
> Reader site
> 
> /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/net/core/dev_ioctl.c:130
>        110
>        111      switch (cmd) {
>        112      case SIOCGIFFLAGS:  /* Get interface flags */
>        113          ifr->ifr_flags = (short) dev_get_flags(dev);
>        114          return 0;
>        115
>        116      case SIOCGIFMETRIC: /* Get the metric on the interface
>        117                     (currently unused) */
>        118          ifr->ifr_metric = 0;
>        119          return 0;
>        120
>        121      case SIOCGIFMTU:    /* Get the MTU of a device */
>        122          ifr->ifr_mtu = dev->mtu;
>        123          return 0;
>        124
>        125      case SIOCGIFHWADDR:
>        126          if (!dev->addr_len)
>        127              memset(ifr->ifr_hwaddr.sa_data, 0,
>        128                     sizeof(ifr->ifr_hwaddr.sa_data));
>        129          else
> ==>    130              memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
>        131                     min(sizeof(ifr->ifr_hwaddr.sa_data),
>        132                     (size_t)dev->addr_len));
>        133          ifr->ifr_hwaddr.sa_family = dev->type;
>        134          return 0;
>        135
>        136      case SIOCGIFSLAVE:
>        137          err = -EINVAL;
>        138          break;
>        139
>        140      case SIOCGIFMAP:
>        141          ifr->ifr_map.mem_start = dev->mem_start;
>        142          ifr->ifr_map.mem_end   = dev->mem_end;
>        143          ifr->ifr_map.base_addr = dev->base_addr;
>        144          ifr->ifr_map.irq       = dev->irq;
>        145          ifr->ifr_map.dma       = dev->dma;
>        146          ifr->ifr_map.port      = dev->if_port;
>        147          return 0;
>        148
>        149      case SIOCGIFINDEX:
>        150          ifr->ifr_ifindex = dev->ifindex;
> 
> 
> ------------------------------------------
> Writer calling trace
> 
> - __sys_sendmsg 
> -- ___sys_sendmsg 
> --- sock_sendmsg
> ---- netlink_unicast
> ----- netlink_rcv_skb
> ------ __rtnl_newlink
> ------- do_setlink
> -------- dev_set_mac_address
> --------- eth_commit_mac_addr_change
> 
> ------------------------------------------
> Reader calling trace
> 
> - ksys_ioctl
> -- do_vfs_ioctl
> --- vfs_ioctl
> ---- dev_ioctl
> 
> 
> 
> Thanks,
> Sishuai
>