| Message ID | 20210304090933.3538255-1-danieller@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ethtool: Add indicator field for link_mode validity to link_ksettings | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: amitc@mellanox.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3082 this patch: 3082 |
| netdev/kdoc | success | Errors and warnings before: 14 this patch: 14 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: Possible repeated word: 'Google' |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3200 this patch: 3200 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, 4 Mar 2021 11:09:33 +0200 Danielle Ratson wrote: > Some drivers clear the 'ethtool_link_ksettings' struct in their > get_link_ksettings() callback, before populating it with actual values. > Such drivers will set the new 'link_mode' field to zero, resulting in > user space receiving wrong link mode information given that zero is a > valid value for the field. > > Fix this by introducing a new boolean field ('link_mode_valid'), which > indicates whether the 'link_mode' field is valid or not. Set it in mlxsw > which is currently the only driver supporting the new API. > > Another problem is that some drivers (notably tun) can report random > values in the 'link_mode' field. This can result in a general protection > fault when the field is used as an index to the 'link_mode_params' array > [1]. > > This happens because such drivers implement their set_link_ksettings() > callback by simply overwriting their private copy of > 'ethtool_link_ksettings' struct with the one they get from the stack, > which is not always properly initialized. > > Fix this by making sure that the new 'link_mode_valid' field is always > initialized to 'false' before invoking the set_link_ksettings() > callback. > > [1] > general protection fault, probably for non-canonical address 0xdffffc00f14cc32c: 0000 [#1] PREEMPT SMP KASAN > KASAN: probably user-memory-access in range [0x000000078a661960-0x000000078a661967] > CPU: 0 PID: 8452 Comm: syz-executor360 Not tainted 5.11.0-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > RIP: 0010:__ethtool_get_link_ksettings+0x1a3/0x3a0 net/ethtool/ioctl.c:446 > Code: b7 3e fa 83 fd ff 0f 84 30 01 00 00 e8 16 b0 3e fa 48 8d 3c ed 60 d5 69 8a 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 > +38 d0 7c 08 84 d2 0f 85 b9 > RSP: 0018:ffffc900019df7a0 EFLAGS: 00010202 > RAX: dffffc0000000000 RBX: ffff888026136008 RCX: 0000000000000000 > RDX: 00000000f14cc32c RSI: ffffffff873439ca RDI: 000000078a661960 > RBP: 00000000ffff8880 R08: 00000000ffffffff R09: ffff88802613606f > R10: ffffffff873439bc R11: 0000000000000000 R12: 0000000000000000 > R13: ffff88802613606c R14: ffff888011d0c210 R15: ffff888011d0c210 > FS: 0000000000749300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000004b60f0 CR3: 00000000185c2000 CR4: 00000000001506f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > linkinfo_prepare_data+0xfd/0x280 net/ethtool/linkinfo.c:37 > ethnl_default_notify+0x1dc/0x630 net/ethtool/netlink.c:586 > ethtool_notify+0xbd/0x1f0 net/ethtool/netlink.c:656 > ethtool_set_link_ksettings+0x277/0x330 net/ethtool/ioctl.c:620 > dev_ethtool+0x2b35/0x45d0 net/ethtool/ioctl.c:2842 > dev_ioctl+0x463/0xb70 net/core/dev_ioctl.c:440 > sock_do_ioctl+0x148/0x2d0 net/socket.c:1060 > sock_ioctl+0x477/0x6a0 net/socket.c:1177 > vfs_ioctl fs/ioctl.c:48 [inline] > __do_sys_ioctl fs/ioctl.c:753 [inline] > __se_sys_ioctl fs/ioctl.c:739 [inline] > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:739 > do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Fixes: c8907043c6ac9 ("ethtool: Get link mode in use instead of speed and duplex parameters") > Signed-off-by: Danielle Ratson <danieller@nvidia.com> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> Reviewed-by: Jakub Kicinski <kuba@kernel.org> BTW shouldn't __ethtool_get_link_ksettings() be moved to common.c?
On Thu, Mar 4, 2021 at 1:13 AM Danielle Ratson <danieller@nvidia.com> wrote: > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -130,6 +130,7 @@ struct ethtool_link_ksettings { > } link_modes; > u32 lanes; > enum ethtool_link_mode_bit_indices link_mode; > + bool link_mode_valid; > }; Why isn't this handled the same way as is done for lanes, with a cap_link_mode_supported bit in ethtool_ops? This would be more consistent from a driver API perspective. Then, linkmodes_prepare_data() can set link_mode to -1 for drivers that don't claim to supply link_mode. Regards, Edwin Peer
On Thu, Mar 4, 2021 at 2:08 PM Edwin Peer <edwin.peer@broadcom.com> wrote: > On Thu, Mar 4, 2021 at 1:13 AM Danielle Ratson <danieller@nvidia.com> wrote: > > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -130,6 +130,7 @@ struct ethtool_link_ksettings { > > } link_modes; > > u32 lanes; > > enum ethtool_link_mode_bit_indices link_mode; > > + bool link_mode_valid; > > }; > > Why isn't this handled the same way as is done for lanes, with a > cap_link_mode_supported bit in ethtool_ops? This would be more > consistent from a driver API perspective. Then, > linkmodes_prepare_data() can set link_mode to -1 for drivers that > don't claim to supply link_mode. Or rather, since that happens too late, don't set it -1 at all and only set the implied parameters in __ethtool_get_link_ksettings() according to the capability. Regards, Edwin Peer
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, March 4, 2021 8:47 PM > To: Danielle Ratson <danieller@nvidia.com> > Cc: netdev@vger.kernel.org; davem@davemloft.net; eric.dumazet@gmail.com; andrew@lunn.ch; mkubecek@suse.cz; > f.fainelli@gmail.com; acardace@redhat.com; irusskikh@marvell.com; gustavo@embeddedor.com; magnus.karlsson@intel.com; > ecree@solarflare.com; Ido Schimmel <idosch@nvidia.com>; Jiri Pirko <jiri@nvidia.com>; mlxsw <mlxsw@nvidia.com> > Subject: Re: [PATCH net] ethtool: Add indicator field for link_mode validity to link_ksettings > > On Thu, 4 Mar 2021 11:09:33 +0200 Danielle Ratson wrote: > > Some drivers clear the 'ethtool_link_ksettings' struct in their > > get_link_ksettings() callback, before populating it with actual values. > > Such drivers will set the new 'link_mode' field to zero, resulting in > > user space receiving wrong link mode information given that zero is a > > valid value for the field. > > > > Fix this by introducing a new boolean field ('link_mode_valid'), which > > indicates whether the 'link_mode' field is valid or not. Set it in > > mlxsw which is currently the only driver supporting the new API. > > > > Another problem is that some drivers (notably tun) can report random > > values in the 'link_mode' field. This can result in a general > > protection fault when the field is used as an index to the > > 'link_mode_params' array [1]. > > > > This happens because such drivers implement their set_link_ksettings() > > callback by simply overwriting their private copy of > > 'ethtool_link_ksettings' struct with the one they get from the stack, > > which is not always properly initialized. > > > > Fix this by making sure that the new 'link_mode_valid' field is always > > initialized to 'false' before invoking the set_link_ksettings() > > callback. > > > > [1] > > general protection fault, probably for non-canonical address > > 0xdffffc00f14cc32c: 0000 [#1] PREEMPT SMP KASAN > > KASAN: probably user-memory-access in range > > [0x000000078a661960-0x000000078a661967] > > CPU: 0 PID: 8452 Comm: syz-executor360 Not tainted 5.11.0-syzkaller #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, > > BIOS Google 01/01/2011 > > RIP: 0010:__ethtool_get_link_ksettings+0x1a3/0x3a0 > > net/ethtool/ioctl.c:446 > > Code: b7 3e fa 83 fd ff 0f 84 30 01 00 00 e8 16 b0 3e fa 48 8d 3c ed > > 60 d5 69 8a 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 > > 14 02 48 89 f8 83 e0 07 83 c0 03 > > +38 d0 7c 08 84 d2 0f 85 b9 > > RSP: 0018:ffffc900019df7a0 EFLAGS: 00010202 > > RAX: dffffc0000000000 RBX: ffff888026136008 RCX: 0000000000000000 > > RDX: 00000000f14cc32c RSI: ffffffff873439ca RDI: 000000078a661960 > > RBP: 00000000ffff8880 R08: 00000000ffffffff R09: ffff88802613606f > > R10: ffffffff873439bc R11: 0000000000000000 R12: 0000000000000000 > > R13: ffff88802613606c R14: ffff888011d0c210 R15: ffff888011d0c210 > > FS: 0000000000749300(0000) GS:ffff8880b9c00000(0000) > > knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 00000000004b60f0 CR3: 00000000185c2000 CR4: 00000000001506f0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call > > Trace: > > linkinfo_prepare_data+0xfd/0x280 net/ethtool/linkinfo.c:37 > > ethnl_default_notify+0x1dc/0x630 net/ethtool/netlink.c:586 > > ethtool_notify+0xbd/0x1f0 net/ethtool/netlink.c:656 > > ethtool_set_link_ksettings+0x277/0x330 net/ethtool/ioctl.c:620 > > dev_ethtool+0x2b35/0x45d0 net/ethtool/ioctl.c:2842 > > dev_ioctl+0x463/0xb70 net/core/dev_ioctl.c:440 > > sock_do_ioctl+0x148/0x2d0 net/socket.c:1060 > > sock_ioctl+0x477/0x6a0 net/socket.c:1177 vfs_ioctl fs/ioctl.c:48 > > [inline] __do_sys_ioctl fs/ioctl.c:753 [inline] __se_sys_ioctl > > fs/ioctl.c:739 [inline] > > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:739 > > do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 > > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > > > Fixes: c8907043c6ac9 ("ethtool: Get link mode in use instead of speed > > and duplex parameters") > > Signed-off-by: Danielle Ratson <danieller@nvidia.com> > > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > > Reviewed-by: Jakub Kicinski <kuba@kernel.org> > > BTW shouldn't __ethtool_get_link_ksettings() be moved to common.c? I'll send a patch for net-next, thanks.
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c index 0bd64169bf81..66eb1cd17bc4 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c @@ -1232,14 +1232,14 @@ mlxsw_sp1_from_ptys_link_mode(struct mlxsw_sp *mlxsw_sp, bool carrier_ok, { int i; - cmd->link_mode = -1; - if (!carrier_ok) return; for (i = 0; i < MLXSW_SP1_PORT_LINK_MODE_LEN; i++) { - if (ptys_eth_proto & mlxsw_sp1_port_link_mode[i].mask) + if (ptys_eth_proto & mlxsw_sp1_port_link_mode[i].mask) { cmd->link_mode = mlxsw_sp1_port_link_mode[i].mask_ethtool; + cmd->link_mode_valid = true; + } } } @@ -1672,8 +1672,6 @@ mlxsw_sp2_from_ptys_link_mode(struct mlxsw_sp *mlxsw_sp, bool carrier_ok, struct mlxsw_sp2_port_link_mode link; int i; - cmd->link_mode = -1; - if (!carrier_ok) return; @@ -1681,6 +1679,7 @@ mlxsw_sp2_from_ptys_link_mode(struct mlxsw_sp *mlxsw_sp, bool carrier_ok, if (ptys_eth_proto & mlxsw_sp2_port_link_mode[i].mask) { link = mlxsw_sp2_port_link_mode[i]; cmd->link_mode = link.mask_ethtool[1]; + cmd->link_mode_valid = true; } } } diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index ec4cd3921c67..1f11577dc189 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -130,6 +130,7 @@ struct ethtool_link_ksettings { } link_modes; u32 lanes; enum ethtool_link_mode_bit_indices link_mode; + bool link_mode_valid; }; /** diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 24783b71c584..5aafa30f07ad 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -436,12 +436,11 @@ int __ethtool_get_link_ksettings(struct net_device *dev, memset(link_ksettings, 0, sizeof(*link_ksettings)); - link_ksettings->link_mode = -1; err = dev->ethtool_ops->get_link_ksettings(dev, link_ksettings); if (err) return err; - if (link_ksettings->link_mode != -1) { + if (link_ksettings->link_mode_valid) { link_info = &link_mode_params[link_ksettings->link_mode]; link_ksettings->base.speed = link_info->speed; link_ksettings->lanes = link_info->lanes; @@ -615,6 +614,8 @@ static int ethtool_set_link_ksettings(struct net_device *dev, link_ksettings.base.master_slave_state) return -EINVAL; + link_ksettings.link_mode_valid = false; + err = dev->ethtool_ops->set_link_ksettings(dev, &link_ksettings); if (err >= 0) { ethtool_notify(dev, ETHTOOL_MSG_LINKINFO_NTF, NULL); @@ -701,6 +702,7 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr) if (!convert_legacy_settings_to_link_ksettings(&link_ksettings, &cmd)) return -EINVAL; + link_ksettings.link_mode_valid = false; link_ksettings.base.link_mode_masks_nwords = __ETHTOOL_LINK_MODE_MASK_NU32; ret = dev->ethtool_ops->set_link_ksettings(dev, &link_ksettings);
Some drivers clear the 'ethtool_link_ksettings' struct in their get_link_ksettings() callback, before populating it with actual values. Such drivers will set the new 'link_mode' field to zero, resulting in user space receiving wrong link mode information given that zero is a valid value for the field. Fix this by introducing a new boolean field ('link_mode_valid'), which indicates whether the 'link_mode' field is valid or not. Set it in mlxsw which is currently the only driver supporting the new API. Another problem is that some drivers (notably tun) can report random values in the 'link_mode' field. This can result in a general protection fault when the field is used as an index to the 'link_mode_params' array [1]. This happens because such drivers implement their set_link_ksettings() callback by simply overwriting their private copy of 'ethtool_link_ksettings' struct with the one they get from the stack, which is not always properly initialized. Fix this by making sure that the new 'link_mode_valid' field is always initialized to 'false' before invoking the set_link_ksettings() callback. [1] general protection fault, probably for non-canonical address 0xdffffc00f14cc32c: 0000 [#1] PREEMPT SMP KASAN KASAN: probably user-memory-access in range [0x000000078a661960-0x000000078a661967] CPU: 0 PID: 8452 Comm: syz-executor360 Not tainted 5.11.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:__ethtool_get_link_ksettings+0x1a3/0x3a0 net/ethtool/ioctl.c:446 Code: b7 3e fa 83 fd ff 0f 84 30 01 00 00 e8 16 b0 3e fa 48 8d 3c ed 60 d5 69 8a 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 +38 d0 7c 08 84 d2 0f 85 b9 RSP: 0018:ffffc900019df7a0 EFLAGS: 00010202 RAX: dffffc0000000000 RBX: ffff888026136008 RCX: 0000000000000000 RDX: 00000000f14cc32c RSI: ffffffff873439ca RDI: 000000078a661960 RBP: 00000000ffff8880 R08: 00000000ffffffff R09: ffff88802613606f R10: ffffffff873439bc R11: 0000000000000000 R12: 0000000000000000 R13: ffff88802613606c R14: ffff888011d0c210 R15: ffff888011d0c210 FS: 0000000000749300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000004b60f0 CR3: 00000000185c2000 CR4: 00000000001506f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: linkinfo_prepare_data+0xfd/0x280 net/ethtool/linkinfo.c:37 ethnl_default_notify+0x1dc/0x630 net/ethtool/netlink.c:586 ethtool_notify+0xbd/0x1f0 net/ethtool/netlink.c:656 ethtool_set_link_ksettings+0x277/0x330 net/ethtool/ioctl.c:620 dev_ethtool+0x2b35/0x45d0 net/ethtool/ioctl.c:2842 dev_ioctl+0x463/0xb70 net/core/dev_ioctl.c:440 sock_do_ioctl+0x148/0x2d0 net/socket.c:1060 sock_ioctl+0x477/0x6a0 net/socket.c:1177 vfs_ioctl fs/ioctl.c:48 [inline] __do_sys_ioctl fs/ioctl.c:753 [inline] __se_sys_ioctl fs/ioctl.c:739 [inline] __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:739 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x44/0xa9 Fixes: c8907043c6ac9 ("ethtool: Get link mode in use instead of speed and duplex parameters") Signed-off-by: Danielle Ratson <danieller@nvidia.com> Reported-by: Eric Dumazet <eric.dumazet@gmail.com> Reviewed-by: Ido Schimmel <idosch@nvidia.com> --- drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c | 9 ++++----- include/linux/ethtool.h | 1 + net/ethtool/ioctl.c | 6 ++++-- 3 files changed, 9 insertions(+), 7 deletions(-)