lkml.org 
[lkml]   [2021]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH net-next v6 2/6] ethtool: improve compat ioctl handling
From
Date
On 7/22/21 7:28 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The ethtool compat ioctl handling is hidden away in net/socket.c,
> which introduces a couple of minor oddities:
>
> - The implementation may end up diverging, as seen in the RXNFC
> extension in commit 84a1d9c48200 ("net: ethtool: extend RXNFC
> API to support RSS spreading of filter matches") that does not work
> in compat mode.
>
> - Most architectures do not need the compat handling at all
> because u64 and compat_u64 have the same alignment.
>
> - On x86, the conversion is done for both x32 and i386 user space,
> but it's actually wrong to do it for x32 and cannot work there.
>
> - On 32-bit Arm, it never worked for compat oabi user space, since
> that needs to do the same conversion but does not.
>
> - It would be nice to get rid of both compat_alloc_user_space()
> and copy_in_user() throughout the kernel.
>
> None of these actually seems to be a serious problem that real
> users are likely to encounter, but fixing all of them actually
> leads to code that is both shorter and more readable.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Something's odd here...  I didn't dive in to find the actual problem,
but here's what I did find.  This doesn't happen before the patchset,
but does happen after the patchset.  I built the kernel on a RHEL 8.4
box using their config file as a basis.

[root@sln-dev ~]# ethtool --version
ethtool version 5.8

[root@sln-dev ~]# ethtool -i eno1
driver: tg3
version: 5.14.0-rc2-net-next-sln+
firmware-version: 5719-v1.46 NCSI v1.5.18.0
expansion-rom-version:
bus-info: 0000:02:00.0
supports-statistics: yes
supports-test: yes
supports-eeprom-access: yes
supports-register-dump: yes
supports-priv-flags: no

[root@sln-dev ~]# ethtool -x eno1
Cannot get RX ring count: Bad address

And we get a stack trace here:
[ 1221.631085] ------------[ cut here ]------------
[ 1221.631105] Buffer overflow detected (8 < 192)!
[ 1221.631125] WARNING: CPU: 27 PID: 2363 at
include/linux/thread_info.h:200 ethtool_rxnfc_copy_to_user+0x2b/0xb0
[ 1221.631150] Modules linked in: xt_CHECKSUM xt_MASQUERADE xt_conntrack
ipt_REJECT nf_reject_ipv4 nft_compat nft_counter nft_chain_nat nf_nat
nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink tun
bridge stp llc intel_rapl_msr intel_rapl_common isst_if_common rpcrdma
rdma_ucm ib_srpt ib_isert iscsi_target_mod target_core_mod nfit ib_iser
libnvdimm libiscsi scsi_transport_iscsi ib_umad ib_ipoib rdma_cm rfkill
x86_pkg_temp_thermal intel_powerclamp iw_cm ib_cm coretemp kvm_intel kvm
sunrpc mlx5_ib irqbypass crct10dif_pclmul crc32_pclmul
ghash_clmulni_intel ib_uverbs rapl ipmi_ssif intel_cstate ib_core
intel_uncore mei_me pcspkr ses mei enclosure acpi_ipmi hpwdt hpilo
ioatdma intel_pch_thermal ipmi_si lpc_ich dca ipmi_devintf
ipmi_msghandler acpi_tad acpi_power_meter ip_tables xfs libcrc32c sd_mod
t10_pi sg mlx5_core mgag200 drm_kms_helper syscopyarea sysfillrect
sysimgblt fb_sys_fops drm mlxfw pci_hyperv_intf ionic tls crc32c_intel
smartpqi serio_raw tg3 scsi_transport_sas
[ 1221.631199]  psample i2c_algo_bit wmi dm_mirror dm_region_hash dm_log
dm_mod fuse
[ 1221.631364] CPU: 27 PID: 2363 Comm: ethtool Tainted: G S W        
5.14.0-rc2-net-next-sln+ #7
[ 1221.631384] Hardware name: HPE ProLiant DL360 Gen10/ProLiant DL360
Gen10, BIOS U32 01/23/2021
[ 1221.631401] RIP: 0010:ethtool_rxnfc_copy_to_user+0x2b/0xb0
[ 1221.631416] Code: 1f 44 00 00 41 55 65 48 8b 04 25 00 6f 01 00 41 54
55 53 f6 40 10 02 75 23 be 08 00 00 00 48 c7 c7 20 f2 f1 89 e8 32 7d 14
00 <0f> 0b 41 bd f2 ff ff ff 5b 44 89 e8 5d 41 5c 41 5d c3 48 89 fd 48
[ 1221.631451] RSP: 0018:ffffadc906e5bbd8 EFLAGS: 00010286
[ 1221.631463] RAX: 0000000000000000 RBX: ffffadc906e5bc00 RCX:
0000000000000027
[ 1221.631478] RDX: 0000000000000027 RSI: ffff91163f857c80 RDI:
ffff91163f857c88
[ 1221.631492] RBP: 0000000000000000 R08: 0000000000000000 R09:
c0000000ffff7fff
[ 1221.631507] R10: 0000000000000001 R11: ffffadc906e5b9e8 R12:
0000000000000000
[ 1221.631535] R13: 00007ffc5be4c730 R14: 00000000000000c0 R15:
ffff90f83b558000
[ 1221.631551] FS:  00007fb10a942740(0000) GS:ffff91163f840000(0000)
knlGS:0000000000000000
[ 1221.631584] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1221.631598] CR2: 000055ae4763a6f0 CR3: 0000001053530001 CR4:
00000000007706e0
[ 1221.631621] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[ 1221.631636] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[ 1221.631667] PKRU: 55555554
[ 1221.631676] Call Trace:
[ 1221.631686]  ethtool_get_rxnfc+0xe8/0x1a0
[ 1221.631700]  dev_ethtool+0xb1a/0x2a20
[ 1221.631711]  ? do_set_pte+0xcb/0x110
[ 1221.632313]  ? inet_ioctl+0x158/0x1a0
[ 1221.632849]  ? page_counter_try_charge+0x57/0xc0
[ 1221.633374]  ? __cond_resched+0x15/0x30
[ 1221.633880]  dev_ioctl+0xb5/0x4e0
[ 1221.634374]  sock_do_ioctl+0x92/0xd0
[ 1221.634867]  sock_ioctl+0x246/0x340
[ 1221.635335]  __x64_sys_ioctl+0x81/0xc0
[ 1221.635817]  do_syscall_64+0x37/0x80
[ 1221.636281]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 1221.636760] RIP: 0033:0x7fb109ed062b
[ 1221.637210] Code: 0f 1e fa 48 8b 05 5d b8 2c 00 64 c7 00 26 00 00 00
48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 2d b8 2c 00 f7 d8 64 89 01 48
[ 1221.638151] RSP: 002b:00007ffc5be4c6f8 EFLAGS: 00000246 ORIG_RAX:
0000000000000010
[ 1221.638649] RAX: ffffffffffffffda RBX: 0000000000000000 RCX:
00007fb109ed062b
[ 1221.639135] RDX: 00007ffc5be4c870 RSI: 0000000000008946 RDI:
0000000000000003
[ 1221.639647] RBP: 00007ffc5be4c860 R08: 00007ffc5be4c870 R09:
0000000000000001
[ 1221.640154] R10: 000055ae4764a934 R11: 0000000000000246 R12:
000055ae47613b60
[ 1221.640647] R13: 00007ffc5be4c870 R14: 000055ae4764718e R15:
000055ae47647196
[ 1221.641124] ---[ end trace 422c6846895775bd ]---



Cheers,
sln

> ---
> Changes in v2:
> - remove extraneous 'inline' keyword (davem)
> - split helper functions into smaller units (hch)
> - remove arm oabi check with missing dependency (0day bot)
> ---
> include/linux/ethtool.h | 4 --
> net/ethtool/ioctl.c | 136 +++++++++++++++++++++++++++++++++++-----
> net/socket.c | 125 +-----------------------------------
> 3 files changed, 121 insertions(+), 144 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 232daaec56e4..4711b96dae0c 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -17,8 +17,6 @@
> #include <linux/compat.h>
> #include <uapi/linux/ethtool.h>
>
> -#ifdef CONFIG_COMPAT
> -
> struct compat_ethtool_rx_flow_spec {
> u32 flow_type;
> union ethtool_flow_union h_u;
> @@ -38,8 +36,6 @@ struct compat_ethtool_rxnfc {
> u32 rule_locs[];
> };
>
> -#endif /* CONFIG_COMPAT */
> -
> #include <linux/rculist.h>
>
> /**
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index baa5d10043cb..6134b180f59f 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -7,6 +7,7 @@
> * the information ethtool needs.
> */
>
> +#include <linux/compat.h>
> #include <linux/module.h>
> #include <linux/types.h>
> #include <linux/capability.h>
> @@ -807,6 +808,120 @@ static noinline_for_stack int ethtool_get_sset_info(struct net_device *dev,
> return ret;
> }
>
> +static noinline_for_stack int
> +ethtool_rxnfc_copy_from_compat(struct ethtool_rxnfc *rxnfc,
> + const struct compat_ethtool_rxnfc __user *useraddr,
> + size_t size)
> +{
> + struct compat_ethtool_rxnfc crxnfc = {};
> +
> + /* We expect there to be holes between fs.m_ext and
> + * fs.ring_cookie and at the end of fs, but nowhere else.
> + * On non-x86, no conversion should be needed.
> + */
> + BUILD_BUG_ON(!IS_ENABLED(CONFIG_X86_64) &&
> + sizeof(struct compat_ethtool_rxnfc) !=
> + sizeof(struct ethtool_rxnfc));
> + BUILD_BUG_ON(offsetof(struct compat_ethtool_rxnfc, fs.m_ext) +
> + sizeof(useraddr->fs.m_ext) !=
> + offsetof(struct ethtool_rxnfc, fs.m_ext) +
> + sizeof(rxnfc->fs.m_ext));
> + BUILD_BUG_ON(offsetof(struct compat_ethtool_rxnfc, fs.location) -
> + offsetof(struct compat_ethtool_rxnfc, fs.ring_cookie) !=
> + offsetof(struct ethtool_rxnfc, fs.location) -
> + offsetof(struct ethtool_rxnfc, fs.ring_cookie));
> +
> + if (copy_from_user(&crxnfc, useraddr, min(size, sizeof(crxnfc))))
> + return -EFAULT;
> +
> + *rxnfc = (struct ethtool_rxnfc) {
> + .cmd = crxnfc.cmd,
> + .flow_type = crxnfc.flow_type,
> + .data = crxnfc.data,
> + .fs = {
> + .flow_type = crxnfc.fs.flow_type,
> + .h_u = crxnfc.fs.h_u,
> + .h_ext = crxnfc.fs.h_ext,
> + .m_u = crxnfc.fs.m_u,
> + .m_ext = crxnfc.fs.m_ext,
> + .ring_cookie = crxnfc.fs.ring_cookie,
> + .location = crxnfc.fs.location,
> + },
> + .rule_cnt = crxnfc.rule_cnt,
> + };
> +
> + return 0;
> +}
> +
> +static int ethtool_rxnfc_copy_from_user(struct ethtool_rxnfc *rxnfc,
> + const void __user *useraddr,
> + size_t size)
> +{
> + if (compat_need_64bit_alignment_fixup())
> + return ethtool_rxnfc_copy_from_compat(rxnfc, useraddr, size);
> +
> + if (copy_from_user(rxnfc, useraddr, size))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int ethtool_rxnfc_copy_to_compat(void __user *useraddr,
> + const struct ethtool_rxnfc *rxnfc,
> + size_t size, const u32 *rule_buf)
> +{
> + struct compat_ethtool_rxnfc crxnfc;
> +
> + memset(&crxnfc, 0, sizeof(crxnfc));
> + crxnfc = (struct compat_ethtool_rxnfc) {
> + .cmd = rxnfc->cmd,
> + .flow_type = rxnfc->flow_type,
> + .data = rxnfc->data,
> + .fs = {
> + .flow_type = rxnfc->fs.flow_type,
> + .h_u = rxnfc->fs.h_u,
> + .h_ext = rxnfc->fs.h_ext,
> + .m_u = rxnfc->fs.m_u,
> + .m_ext = rxnfc->fs.m_ext,
> + .ring_cookie = rxnfc->fs.ring_cookie,
> + .location = rxnfc->fs.location,
> + },
> + .rule_cnt = rxnfc->rule_cnt,
> + };
> +
> + if (copy_to_user(useraddr, &crxnfc, min(size, sizeof(crxnfc))))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int ethtool_rxnfc_copy_to_user(void __user *useraddr,
> + const struct ethtool_rxnfc *rxnfc,
> + size_t size, const u32 *rule_buf)
> +{
> + int ret;
> +
> + if (compat_need_64bit_alignment_fixup()) {
> + ret = ethtool_rxnfc_copy_to_compat(useraddr, rxnfc, size,
> + rule_buf);
> + useraddr += offsetof(struct compat_ethtool_rxnfc, rule_locs);
> + } else {
> + ret = copy_to_user(useraddr, &rxnfc, size);
> + useraddr += offsetof(struct ethtool_rxnfc, rule_locs);
> + }
> +
> + if (ret)
> + return -EFAULT;
> +
> + if (rule_buf) {
> + if (copy_to_user(useraddr, rule_buf,
> + rxnfc->rule_cnt * sizeof(u32)))
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
> u32 cmd, void __user *useraddr)
> {
> @@ -825,7 +940,7 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
> info_size = (offsetof(struct ethtool_rxnfc, data) +
> sizeof(info.data));
>
> - if (copy_from_user(&info, useraddr, info_size))
> + if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
> return -EFAULT;
>
> rc = dev->ethtool_ops->set_rxnfc(dev, &info);
> @@ -833,7 +948,7 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
> return rc;
>
> if (cmd == ETHTOOL_SRXCLSRLINS &&
> - copy_to_user(useraddr, &info, info_size))
> + ethtool_rxnfc_copy_to_user(useraddr, &info, info_size, NULL))
> return -EFAULT;
>
> return 0;
> @@ -859,7 +974,7 @@ static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
> info_size = (offsetof(struct ethtool_rxnfc, data) +
> sizeof(info.data));
>
> - if (copy_from_user(&info, useraddr, info_size))
> + if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
> return -EFAULT;
>
> /* If FLOW_RSS was requested then user-space must be using the
> @@ -867,7 +982,7 @@ static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
> */
> if (cmd == ETHTOOL_GRXFH && info.flow_type & FLOW_RSS) {
> info_size = sizeof(info);
> - if (copy_from_user(&info, useraddr, info_size))
> + if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
> return -EFAULT;
> /* Since malicious users may modify the original data,
> * we need to check whether FLOW_RSS is still requested.
> @@ -893,18 +1008,7 @@ static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
> if (ret < 0)
> goto err_out;
>
> - ret = -EFAULT;
> - if (copy_to_user(useraddr, &info, info_size))
> - goto err_out;
> -
> - if (rule_buf) {
> - useraddr += offsetof(struct ethtool_rxnfc, rule_locs);
> - if (copy_to_user(useraddr, rule_buf,
> - info.rule_cnt * sizeof(u32)))
> - goto err_out;
> - }
> - ret = 0;
> -
> + ret = ethtool_rxnfc_copy_to_user(useraddr, &info, info_size, rule_buf);
> err_out:
> kfree(rule_buf);
>
> diff --git a/net/socket.c b/net/socket.c
> index 0b2dad3bdf7f..ec63cf6de33e 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -3152,128 +3152,6 @@ static int compat_dev_ifconf(struct net *net, struct compat_ifconf __user *uifc3
> return 0;
> }
>
> -static int ethtool_ioctl(struct net *net, struct compat_ifreq __user *ifr32)
> -{
> - struct compat_ethtool_rxnfc __user *compat_rxnfc;
> - bool convert_in = false, convert_out = false;
> - size_t buf_size = 0;
> - struct ethtool_rxnfc __user *rxnfc = NULL;
> - struct ifreq ifr;
> - u32 rule_cnt = 0, actual_rule_cnt;
> - u32 ethcmd;
> - u32 data;
> - int ret;
> -
> - if (get_user(data, &ifr32->ifr_ifru.ifru_data))
> - return -EFAULT;
> -
> - compat_rxnfc = compat_ptr(data);
> -
> - if (get_user(ethcmd, &compat_rxnfc->cmd))
> - return -EFAULT;
> -
> - /* Most ethtool structures are defined without padding.
> - * Unfortunately struct ethtool_rxnfc is an exception.
> - */
> - switch (ethcmd) {
> - default:
> - break;
> - case ETHTOOL_GRXCLSRLALL:
> - /* Buffer size is variable */
> - if (get_user(rule_cnt, &compat_rxnfc->rule_cnt))
> - return -EFAULT;
> - if (rule_cnt > KMALLOC_MAX_SIZE / sizeof(u32))
> - return -ENOMEM;
> - buf_size += rule_cnt * sizeof(u32);
> - fallthrough;
> - case ETHTOOL_GRXRINGS:
> - case ETHTOOL_GRXCLSRLCNT:
> - case ETHTOOL_GRXCLSRULE:
> - case ETHTOOL_SRXCLSRLINS:
> - convert_out = true;
> - fallthrough;
> - case ETHTOOL_SRXCLSRLDEL:
> - buf_size += sizeof(struct ethtool_rxnfc);
> - convert_in = true;
> - rxnfc = compat_alloc_user_space(buf_size);
> - break;
> - }
> -
> - if (copy_from_user(&ifr.ifr_name, &ifr32->ifr_name, IFNAMSIZ))
> - return -EFAULT;
> -
> - ifr.ifr_data = convert_in ? rxnfc : (void __user *)compat_rxnfc;
> -
> - if (convert_in) {
> - /* We expect there to be holes between fs.m_ext and
> - * fs.ring_cookie and at the end of fs, but nowhere else.
> - */
> - BUILD_BUG_ON(offsetof(struct compat_ethtool_rxnfc, fs.m_ext) +
> - sizeof(compat_rxnfc->fs.m_ext) !=
> - offsetof(struct ethtool_rxnfc, fs.m_ext) +
> - sizeof(rxnfc->fs.m_ext));
> - BUILD_BUG_ON(
> - offsetof(struct compat_ethtool_rxnfc, fs.location) -
> - offsetof(struct compat_ethtool_rxnfc, fs.ring_cookie) !=
> - offsetof(struct ethtool_rxnfc, fs.location) -
> - offsetof(struct ethtool_rxnfc, fs.ring_cookie));
> -
> - if (copy_in_user(rxnfc, compat_rxnfc,
> - (void __user *)(&rxnfc->fs.m_ext + 1) -
> - (void __user *)rxnfc) ||
> - copy_in_user(&rxnfc->fs.ring_cookie,
> - &compat_rxnfc->fs.ring_cookie,
> - (void __user *)(&rxnfc->fs.location + 1) -
> - (void __user *)&rxnfc->fs.ring_cookie))
> - return -EFAULT;
> - if (ethcmd == ETHTOOL_GRXCLSRLALL) {
> - if (put_user(rule_cnt, &rxnfc->rule_cnt))
> - return -EFAULT;
> - } else if (copy_in_user(&rxnfc->rule_cnt,
> - &compat_rxnfc->rule_cnt,
> - sizeof(rxnfc->rule_cnt)))
> - return -EFAULT;
> - }
> -
> - ret = dev_ioctl(net, SIOCETHTOOL, &ifr, NULL);
> - if (ret)
> - return ret;
> -
> - if (convert_out) {
> - if (copy_in_user(compat_rxnfc, rxnfc,
> - (const void __user *)(&rxnfc->fs.m_ext + 1) -
> - (const void __user *)rxnfc) ||
> - copy_in_user(&compat_rxnfc->fs.ring_cookie,
> - &rxnfc->fs.ring_cookie,
> - (const void __user *)(&rxnfc->fs.location + 1) -
> - (const void __user *)&rxnfc->fs.ring_cookie) ||
> - copy_in_user(&compat_rxnfc->rule_cnt, &rxnfc->rule_cnt,
> - sizeof(rxnfc->rule_cnt)))
> - return -EFAULT;
> -
> - if (ethcmd == ETHTOOL_GRXCLSRLALL) {
> - /* As an optimisation, we only copy the actual
> - * number of rules that the underlying
> - * function returned. Since Mallory might
> - * change the rule count in user memory, we
> - * check that it is less than the rule count
> - * originally given (as the user buffer size),
> - * which has been range-checked.
> - */
> - if (get_user(actual_rule_cnt, &rxnfc->rule_cnt))
> - return -EFAULT;
> - if (actual_rule_cnt < rule_cnt)
> - rule_cnt = actual_rule_cnt;
> - if (copy_in_user(&compat_rxnfc->rule_locs[0],
> - &rxnfc->rule_locs[0],
> - rule_cnt * sizeof(u32)))
> - return -EFAULT;
> - }
> - }
> -
> - return 0;
> -}
> -
> static int compat_siocwandev(struct net *net, struct compat_ifreq __user *uifr32)
> {
> compat_uptr_t uptr32;
> @@ -3428,8 +3306,6 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
> return old_bridge_ioctl(argp);
> case SIOCGIFCONF:
> return compat_dev_ifconf(net, argp);
> - case SIOCETHTOOL:
> - return ethtool_ioctl(net, argp);
> case SIOCWANDEV:
> return compat_siocwandev(net, argp);
> case SIOCGIFMAP:
> @@ -3442,6 +3318,7 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
> return sock->ops->gettstamp(sock, argp, cmd == SIOCGSTAMP_OLD,
> !COMPAT_USE_64BIT_TIME);
>
> + case SIOCETHTOOL:
> case SIOCBONDSLAVEINFOQUERY:
> case SIOCBONDINFOQUERY:
> case SIOCSHWTSTAMP:

\
 
 \ /
  Last update: 2021-07-24 01:51    [W:0.067 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site