netdev - RE: [RFC PATCH V2 net-next 5/5] ethtool: Add fallback to get_module

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, 4 Mar 2021 16:50:25 -0800
From:   "Don Bollinger" <don@...bollingers.org>
To:     "'Moshe Shemesh'" <moshe@...dia.com>,
        "'David S. Miller'" <davem@...emloft.net>,
        "'Jakub Kicinski'" <kuba@...nel.org>,
        "'Andrew Lunn'" <andrew@...n.ch>,
        "'Adrian Pop'" <pop.adrian61@...il.com>,
        "'Michal Kubecek'" <mkubecek@...e.cz>, <netdev@...r.kernel.org>
Cc:     "'Vladyslav Tarasiuk'" <vladyslavt@...dia.com>,
        <don@...bollingers.org>
Subject: RE: [RFC PATCH V2 net-next 5/5] ethtool: Add fallback to get_module_eeprom from netlink command

On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote:
> From: Vladyslav Tarasiuk <vladyslavt@...dia.com>
> 
> In case netlink get_module_eeprom_data_by_page() callback is not
> implemented by the driver, try to call old get_module_info() and
> get_module_eeprom() pair. Recalculate parameters to
> get_module_eeprom() offset and len using page number and their sizes.
> Return error if this can't be done.
> 
> Signed-off-by: Vladyslav Tarasiuk <vladyslavt@...dia.com>
> ---
>  net/ethtool/eeprom.c | 84
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c index
> 2618a55b9a40..72c7714a9d37 100644
> --- a/net/ethtool/eeprom.c
> +++ b/net/ethtool/eeprom.c
> @@ -26,6 +26,88 @@ struct eeprom_data_reply_data {  #define
> EEPROM_DATA_REPDATA(__reply_base) \
>  	container_of(__reply_base, struct eeprom_data_reply_data, base)
> 
> +static int fallback_set_params(struct eeprom_data_req_info *request,
> +			       struct ethtool_modinfo *modinfo,
> +			       struct ethtool_eeprom *eeprom) {

This is translating the new data structure into the old.  Hence, I assume we
have i2c_addr, page, bank, offset, len to work with, and we should use
all of them.  We shouldn't be applying the legacy data structure's rules
to how we interpret the *request data.  Therefore...

> +	u32 offset = request->offset;
> +	u32 length = request->length;
> +
> +	if (request->page)
> +		offset = 128 + request->page * 128 + offset;

This is tricky to map to old behavior.  The new data structure should give
lower 
memory for offsets less than 128, and paged upper memory for offsets of 128
and higher.  There is no way to describe that request as {offset, length} in
the
old ethtool format with a fake linear memory.

        if (request->page) {
                if (offset < 128) && (offset + length > 128)
                       return -EINVAL;
                if (offset > 127) offset = request->page * 128 + offset;

> +
> +	if (!length)
> +		length = modinfo->eeprom_len;
> +
> +	if (offset >= modinfo->eeprom_len)
> +		return -EINVAL;
> +
> +	if (modinfo->eeprom_len < offset + length)
> +		length = modinfo->eeprom_len - offset;
> +
> +	eeprom->cmd = ETHTOOL_GMODULEEEPROM;
> +	eeprom->len = length;
> +	eeprom->offset = offset;
> +
> +	switch (modinfo->type) {
> +	case ETH_MODULE_SFF_8079:
> +		if (request->page > 1)
> +			return -EINVAL;
> +		break;
> +	case ETH_MODULE_SFF_8472:
> +		if (request->page > 3)

Not sure this is needed, there can be pages higher than 3.

> +			return -EINVAL;

I *think* the linear memory on SFP puts 0x50 in the first
256 bytes, 0x51 after that, including pages after that.  So,
the old fashioned linear memory offset needs to be adjusted
for accesses to 0x51.  Thus add:

        if (request->i2c_address == 0x51)
                offset += 256;

> +		break;
> +	case ETH_MODULE_SFF_8436:
> +	case ETH_MODULE_SFF_8636:

Not sure this is needed, there can be pages higher than 3.

> +		if (request->page > 3)
> +			return -EINVAL;
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int eeprom_data_fallback(struct eeprom_data_req_info *request,
> +				struct eeprom_data_reply_data *reply,
> +				struct genl_info *info)
> +{
> +	struct net_device *dev = reply->base.dev;
> +	struct ethtool_modinfo modinfo = {0};
> +	struct ethtool_eeprom eeprom = {0};
> +	u8 *data;
> +	int err;
> +
> +	if ((!dev->ethtool_ops->get_module_info &&
> +	     !dev->ethtool_ops->get_module_eeprom) ||
> +	    request->bank || request->i2c_address) {

We don't need to reject if there is an i2c_address.  Indeed, we need that
to determine the correct offset for the legacy linear memory offset.

Note my comment on an earlier patch in this series, I would have rejected
any request that didn't have either 0x50 or 0x51 here.

> +		return -EOPNOTSUPP;
> +	}
> +	modinfo.cmd = ETHTOOL_GMODULEINFO;
> +	err = dev->ethtool_ops->get_module_info(dev, &modinfo);
> +	if (err < 0)
> +		return err;
> +
> +	err = fallback_set_params(request, &modinfo, &eeprom);
> +	if (err < 0)
> +		return err;
> +
> +	data = kmalloc(eeprom.len, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	err = dev->ethtool_ops->get_module_eeprom(dev, &eeprom,
> data);
> +	if (err < 0)
> +		goto err_out;
> +
> +	reply->data = data;
> +	reply->length = eeprom.len;
> +
> +	return 0;
> +
> +err_out:
> +	kfree(data);
> +	return err;
> +}
> +
>  static int eeprom_data_prepare_data(const struct ethnl_req_info
> *req_base,
>  				    struct ethnl_reply_data *reply_base,
>  				    struct genl_info *info)
> @@ -37,7 +119,7 @@ static int eeprom_data_prepare_data(const struct
> ethnl_req_info *req_base,
>  	int err;
> 
>  	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
> -		return -EOPNOTSUPP;
> +		return eeprom_data_fallback(request, reply, info);
> 
>  	page_data.offset = request->offset;
>  	page_data.length = request->length;
> --
> 2.18.2