[bug report] net: fix dev_ifsioc_locked() race condition

[bug report] net: fix dev_ifsioc_locked() race condition

[Dan Carpenter]

Hello Cong Wang,

The patch 3b23a32a6321: "net: fix dev_ifsioc_locked() race condition"
from Feb 11, 2021, leads to the following static checker warning:

	drivers/net/tap.c:1095 tap_ioctl()
	warn: check that 'sa.sa_family' doesn't leak information

drivers/net/tap.c
  1084  
  1085          case SIOCGIFHWADDR:
  1086                  rtnl_lock();
  1087                  tap = tap_get_tap_dev(q);
  1088                  if (!tap) {
  1089                          rtnl_unlock();
  1090                          return -ENOLINK;
  1091                  }
  1092                  ret = 0;
  1093                  dev_get_mac_address(&sa, dev_net(tap->dev), tap->dev->name);

How do you want to handle errors from dev_get_mac_address()?

  1094                  if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
  1095                      copy_to_user(&ifr->ifr_hwaddr, &sa, sizeof(sa)))
  1096                          ret = -EFAULT;
  1097                  tap_put_tap_dev(tap);
  1098                  rtnl_unlock();
  1099                  return ret;
  1100  

regards,
dan carpenter

Re: [bug report] net: fix dev_ifsioc_locked() race condition

[Cong Wang]

On Tue, Feb 16, 2021 at 11:40 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Cong Wang,
>
> The patch 3b23a32a6321: "net: fix dev_ifsioc_locked() race condition"
> from Feb 11, 2021, leads to the following static checker warning:
>
>         drivers/net/tap.c:1095 tap_ioctl()
>         warn: check that 'sa.sa_family' doesn't leak information
>
> drivers/net/tap.c
>   1084
>   1085          case SIOCGIFHWADDR:
>   1086                  rtnl_lock();
>   1087                  tap = tap_get_tap_dev(q);
>   1088                  if (!tap) {
>   1089                          rtnl_unlock();
>   1090                          return -ENOLINK;
>   1091                  }
>   1092                  ret = 0;
>   1093                  dev_get_mac_address(&sa, dev_net(tap->dev), tap->dev->name);
>
> How do you want to handle errors from dev_get_mac_address()?

I did have a check there but it turns out unnecessary, because the only
error can happen is when the device name is not found, but here
we already have tap->dev, therefore its name is always there and valid.

Thanks.