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  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:   Sun, 24 May 2020 22:34:12 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Chris Healy <cphealy@...il.com>
Subject: Re: [PATCH v2 net-next 5/6] net: ethtool: Allow PHY cable test TDR
 data to configured

On Sun, May 24, 2020 at 05:27:45PM +0200, Andrew Lunn wrote:
> Allow the user to configure where on the cable the TDR data should be
> retrieved, in terms of first and last sample, and the step between
> samples. Also add the ability to ask for TDR data for just one pair.
> 
> If this configuration is not provided, it defaults to 1-150m at 1m
> intervals for all pairs.
> 
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
[...]
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 739faa7070c6..0b68ab7d08ea 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -485,6 +485,10 @@ enum {
>  enum {
>  	ETHTOOL_A_CABLE_TEST_TDR_UNSPEC,
>  	ETHTOOL_A_CABLE_TEST_TDR_HEADER,	/* nest - _A_HEADER_* */
> +	ETHTOOL_A_CABLE_TEST_TDR_FIRST,		/* u32 */
> +	ETHTOOL_A_CABLE_TEST_TDR_LAST,		/* u32 */
> +	ETHTOOL_A_CABLE_TEST_TDR_STEP,		/* u32 */
> +	ETHTOOL_A_CABLE_TEST_TDR_PAIR,		/* u8 */
>  
>  	/* add new constants above here */
>  	__ETHTOOL_A_CABLE_TEST_TDR_CNT,

Please add description of these attributes to ethtool-netlink.rst.

Also, it might make sense to gather all "test parameters" in one nest
that could used in both request and the final notification (pulse
amplitude would not be allowed in request, unless there is a device
which allows to configure it as well). The main results nest would be
left only for measured results.

This nest could also directly correspond to a structure so that you
could pass one pointer instead of four arguments in some of the helpers
above.

> diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
> index c02575e26336..ddf678eae44f 100644
> --- a/net/ethtool/cabletest.c
> +++ b/net/ethtool/cabletest.c
> @@ -5,7 +5,11 @@
>  #include "netlink.h"
>  #include "common.h"
>  
> -/* CABLE_TEST_ACT */
> +/* 802.3 standard allows 100 meters for BaseT cables. However longer
> + * cables might work, depending on the quality of the cables and the
> + * PHY. So allow testing for up to 150 meters.
> + */
> +#define MAX_CABLE_LENGTH_CM (150 * 100)
>  
>  static const struct nla_policy
>  cable_test_act_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
> @@ -203,18 +207,30 @@ int ethnl_cable_test_fault_length(struct phy_device *phydev, u8 pair, u32 cm)
>  }
>  EXPORT_SYMBOL_GPL(ethnl_cable_test_fault_length);
>  
> +struct cable_test_tdr_req_info {
> +	struct ethnl_req_info		base;
> +};
> +
>  static const struct nla_policy
>  cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1] = {
>  	[ETHTOOL_A_CABLE_TEST_TDR_UNSPEC]	= { .type = NLA_REJECT },
>  	[ETHTOOL_A_CABLE_TEST_TDR_HEADER]	= { .type = NLA_NESTED },
> +	[ETHTOOL_A_CABLE_TEST_TDR_FIRST]	= { .type = NLA_U32 },
> +	[ETHTOOL_A_CABLE_TEST_TDR_LAST]		= { .type = NLA_U32 },
> +	[ETHTOOL_A_CABLE_TEST_TDR_STEP]		= { .type = NLA_U32 },
> +	[ETHTOOL_A_CABLE_TEST_TDR_PAIR]		= { .type = NLA_U8 },
>  };
>  
> +/* CABLE_TEST_TDR_ACT */
> +
>  int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1];
>  	struct ethnl_req_info req_info = {};
>  	struct net_device *dev;
> +	u32 first, last, step;
>  	int ret;
> +	u8 pair;
>  
>  	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
>  			  ETHTOOL_A_CABLE_TEST_TDR_MAX,
> @@ -235,12 +251,59 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>  		goto out_dev_put;
>  	}
>  
> +	if (tb[ETHTOOL_A_CABLE_TEST_TDR_FIRST])
> +		first = nla_get_u32(tb[ETHTOOL_A_CABLE_TEST_TDR_FIRST]);
> +	else
> +		first = 100;
> +	if (tb[ETHTOOL_A_CABLE_TEST_TDR_LAST])
> +		last = nla_get_u32(tb[ETHTOOL_A_CABLE_TEST_TDR_LAST]);
> +	else
> +		last = MAX_CABLE_LENGTH_CM;
> +
> +	if (tb[ETHTOOL_A_CABLE_TEST_TDR_STEP])
> +		step = nla_get_u32(tb[ETHTOOL_A_CABLE_TEST_TDR_STEP]);
> +	else
> +		step = 100;
> +
> +	if (tb[ETHTOOL_A_CABLE_TEST_TDR_PAIR]) {
> +		pair = nla_get_u8(tb[ETHTOOL_A_CABLE_TEST_TDR_PAIR]);
> +		if (pair < ETHTOOL_A_CABLE_PAIR_A ||
> +		    pair > ETHTOOL_A_CABLE_PAIR_D) {
> +			NL_SET_ERR_MSG(info->extack,
> +				       "invalid pair parameter");

Please use NL_SET_ERR_MSG_ATTR() here and in the checks below (probably
except the "first > last" test).

> +			return -EINVAL;
> +		}
> +	} else {
> +		pair = PHY_PAIR_ALL;
> +	}
> +
> +	if (first > MAX_CABLE_LENGTH_CM) {
> +		NL_SET_ERR_MSG(info->extack, "invalid first parameter");
> +		return -EINVAL;
> +	}
> +
> +	if (last > MAX_CABLE_LENGTH_CM) {
> +		NL_SET_ERR_MSG(info->extack, "invalid last parameter");
> +		return -EINVAL;
> +	}
> +
> +	if (first > last) {
> +		NL_SET_ERR_MSG(info->extack, "invalid first/last parameter");
> +		return -EINVAL;
> +	}
> +
> +	if (!step) {
> +		NL_SET_ERR_MSG(info->extack, "invalid step parameter");
> +		return -EINVAL;
> +	}

Without an upper bound for step, there might be some nasty tricks with
first + step overflowing the u32 value range. I don't see an example
from the top of my head but I suspect syzkaller would invent something.

Michal


> +
>  	rtnl_lock();
>  	ret = ethnl_ops_begin(dev);
>  	if (ret < 0)
>  		goto out_rtnl;
>  
> -	ret = phy_start_cable_test_tdr(dev->phydev, info->extack);
> +	ret = phy_start_cable_test_tdr(dev->phydev, info->extack,
> +				       first, last, step, pair);
>  
>  	ethnl_ops_complete(dev);
>  
> -- 
> 2.27.0.rc0
> 

Powered by blists - more mailing lists