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 <sish...@purdue.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
>
