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
| ||
|
Message-ID: <20230406150157.rwpmghgao77lkdny@skbuf> Date: Thu, 6 Apr 2023 18:01:57 +0300 From: Vladimir Oltean <vladimir.oltean@....com> To: Max Georgiev <glipus@...il.com> Cc: Jakub Kicinski <kuba@...nel.org>, kory.maincent@...tlin.com, netdev@...r.kernel.org, maxime.chevallier@...tlin.com, vadim.fedorenko@...ux.dev, richardcochran@...il.com, gerhard@...leder-embedded.com Subject: Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path On Thu, Apr 06, 2023 at 12:21:37AM -0600, Max Georgiev wrote: > I tried my best to follow the discussion, and convert it to compilable code. > Here is what I have in mind for generic_hwtstamp_get_lower(): > > int generic_hwtstamp_get_lower(struct net_dev *dev, > struct kernel_hwtstamp_config *kernel_cfg, > struct netlink_ext_ack *extack) > { > const struct net_device_ops *ops = dev->netdev_ops; > struct hwtstamp_config cfg; > int err; > > if (!netif_device_present(dev)) > return -ENODEV; > > if (ops->ndo_hwtstamp_get) > return ops->ndo_hwtstamp_get(dev, cfg, extack); > > if (!cfg->ifr) > return -EOPNOTSUPP; > > err = dev_eth_ioctl(dev, cfg->ifr, SIOCGHWTSTAMP); > if (err) > return err; > > if (copy_from_user(&cfg, cfg->ifr->ifr_data, sizeof(cfg))) > return -EFAULT; > > hwtstamp_config_to_kernel(kernel_cfg, &cfg); > > return 0; > } Side note, it doesn't look like this code is particularly compilable either - "cfg" is used in some places instead of "kernel_cfg". > > It looks like there is a possibility that the returned hwtstamp_config structure > will be copied twice to ifr and copied once from ifr on the return path > in case if the underlying driver does not implement ndo_hwtstamp_get(): > - the underlying driver calls copy_to_user() inside its ndo_eth_ioctl() > implementation to return the data to generic_hwtstamp_get_lower(); > - then generic_hwtstamp_get_lower() calls copy_from_user() to copy it > back out of the ifr to kernel_hwtstamp_config structure; > - then dev_get_hwtstamp() calls copy_to_user() again to update > the same ifr with the same data the ifr already contains. > > Should we consider this acceptable? Thanks for laying this out. I guess with a table it's going to be clearer, so to summarize, I believe this is the status: Assuming we convert *vlan to ndo_hwtstamp_set(): =================== If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev driver uses ndo_eth_ioctl(), we have: - one copy_from_user() in dev_set_hwtstamp() - one copy_from_user() in the real_dev's ndo_eth_ioctl() - one copy_to_user() in the real_dev's ndo_eth_ioctl() - one copy_from_user() in generic_hwtstamp_get_lower() - one copy_to_user() in dev_set_hwtstamp() If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev is converted too, we have: - one copy_from_user() in dev_set_hwtstamp() - one copy_to_user() in dev_set_hwtstamp() =================== Assuming we don't convert *vlan to ndo_hwtstamp_set(): =================== If the vlan driver isn't converted to ndo_hwtstamp_set() and the real_dev driver isn't converted either, we have: - one copy_from_user() in dev_set_hwtstamp() - one copy_from_user() in the real_dev's ndo_eth_ioctl() - one copy_to_user() in the real_dev's ndo_eth_ioctl() If the vlan driver isn't converted to ndo_hwtstamp_set(), but the real_dev driver is, we have: - one copy_from_user() in dev_set_hwtstamp() - one copy_from_user() in the vlan's ndo_eth_ioctl() - one copy_to_user() in the vlan's ndo_eth_ioctl() =================== So between converting and not converting the *vlans to ndo_hwtstamp_set(), the worst case is going to be worse (with a mix of new API in *vlan and old API in real_dev) and the best case is going to be better (with new API in both *vlan and real_dev). OTOH, with old API in *vlan, the number of copies to/from the user buffer is going to be constant at 3, which is not the best, not the worst. I guess the data indicates that we should convert the *vlans to ndo_hwtstamp_set() at the very end of the process, and for now, just make them compatible with a real_dev that uses the new API? Note that I haven't done the math for the "get" operation yet, but I believe it to be similar.
Powered by blists - more mailing lists