Re: [Race] data race between dev_ifsioc_locked() and eth_commit_mac_addr_change()
From: Gong, Sishuai <hidden>
Date: 2021-01-22 04:35:03
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
quoted hunk ↗ jump to hunk
On Nov 30, 2020, at 10:20 AM, Gong, Sishuai [off-list ref] 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_ioctlThanks, Sishuai