netdev - [PATCH net-next v2 3/6] ethtool: fec: sanitize ethtool

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Mar 2021 19:07:24 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     davem@...emloft.net
Cc:     netdev@...r.kernel.org, ecree.xilinx@...il.com,
        michael.chan@...adcom.com, paul.greenwalt@...el.com,
        rajur@...lsio.com, jaroslawx.gawin@...el.com, vkochan@...vell.com,
        alobakin@...me, snelson@...sando.io, shayagr@...zon.com,
        ayal@...dia.com, shenjian15@...wei.com, saeedm@...dia.com,
        mkubecek@...e.cz, andrew@...n.ch, roopa@...dia.com,
        Jakub Kicinski <kuba@...nel.org>
Subject: [PATCH net-next v2 3/6] ethtool: fec: sanitize ethtool_fecparam->reserved

struct ethtool_fecparam::reserved is never looked at by the core.
Make sure it's actually 0. Unfortunately we can't return an error
because old ethtool doesn't zero-initialize the structure for SET.
On GET we can be more verbose, there are no in tree (ab)users.

Fix up the kdoc on the structure. Remove the mention of FEC
bypass. Seems like a niche thing to configure in the first
place.

v2: - also mention the zero-init-on-SET kerfuffle in kdoc

Signed-off-by: Jakub Kicinski <kuba@...nel.org>
---
 include/uapi/linux/ethtool.h | 6 +++++-
 net/ethtool/ioctl.c          | 5 +++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 36bf435d232c..39a7d285b32b 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1376,15 +1376,19 @@ struct ethtool_per_queue_op {
 };
 
 /**
  * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
  * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
  * @active_fec: FEC mode which is active on the port
  * @fec: Bitmask of supported/configured FEC modes
- * @rsvd: Reserved for future extensions. i.e FEC bypass feature.
+ * @reserved: Reserved for future extensions, ignore on GET, write 0 for SET.
+ *
+ * Note that @reserved was never validated on input and ethtool user space
+ * left it uninitialized when calling SET. Hence going forward it can only be
+ * used to return a value to userspace with GET.
  */
 struct ethtool_fecparam {
 	__u32   cmd;
 	/* bitmask of FEC modes */
 	__u32   active_fec;
 	__u32   fec;
 	__u32   reserved;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0788cc3b3114..be3549023d89 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2564,14 +2564,17 @@ static int ethtool_get_fecparam(struct net_device *dev, void __user *useraddr)
 	if (!dev->ethtool_ops->get_fecparam)
 		return -EOPNOTSUPP;
 
 	rc = dev->ethtool_ops->get_fecparam(dev, &fecparam);
 	if (rc)
 		return rc;
 
+	if (WARN_ON_ONCE(fecparam.reserved))
+		fecparam.reserved = 0;
+
 	if (copy_to_user(useraddr, &fecparam, sizeof(fecparam)))
 		return -EFAULT;
 	return 0;
 }
 
 static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 {
@@ -2579,14 +2582,16 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 
 	if (!dev->ethtool_ops->set_fecparam)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
 		return -EFAULT;
 
+	fecparam.reserved = 0;
+
 	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
 }
 
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
 {
-- 
2.30.2