| 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
| ||
|
Message-ID: <e41d2db8-8b75-7c1c-20d3-2368c4cf8276@nvidia.com>
Date: Tue, 16 Mar 2021 19:59:24 +0200
From: Moshe Shemesh <moshe@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "David S. Miller" <davem@...emloft.net>,
Andrew Lunn <andrew@...n.ch>,
Adrian Pop <pop.adrian61@...il.com>,
Michal Kubecek <mkubecek@...e.cz>,
"Don Bollinger" <don@...bollingers.org>, <netdev@...r.kernel.org>,
"Vladyslav Tarasiuk" <vladyslavt@...dia.com>
Subject: Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to
dump arbitrary EEPROM data
On 3/15/2021 11:01 PM, Jakub Kicinski wrote:
>
> On Mon, 15 Mar 2021 19:12:39 +0200 Moshe Shemesh wrote:
>> From: Vladyslav Tarasiuk <vladyslavt@...dia.com>
>>
>> Define get_module_eeprom_data_by_page() ethtool callback and implement
>> netlink infrastructure.
>>
>> get_module_eeprom_data_by_page() allows network drivers to dump a part
>> of module's EEPROM specified by page and bank numbers along with offset
>> and length. It is effectively a netlink replacement for
>> get_module_info() and get_module_eeprom() pair, which is needed due to
>> emergence of complex non-linear EEPROM layouts.
>>
>> Signed-off-by: Vladyslav Tarasiuk <vladyslavt@...dia.com>
> s/eeprom_data/eeprom/ everywhere?
>
> Otherwise some of your identifiers are over 30 characters long.
That can work part to one struct on the uapi file that can conflict with
previous old KAPI struct, but we can figure it too.
Will fix.
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index cde753bb2093..b3e92db3ad37 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -340,6 +340,28 @@ struct ethtool_eeprom {
>> __u8 data[0];
>> };
>>
>> +/**
>> + * struct ethtool_eeprom_data - EEPROM dump from specified page
>> + * @offset: Offset within the specified EEPROM page to begin read, in bytes.
>> + * @length: Number of bytes to read.
>> + * @page: Page number to read from.
>> + * @bank: Page bank number to read from, if applicable by EEPROM spec.
>> + * @i2c_address: I2C address of a page. Value less than 0x7f expected. Most
>> + * EEPROMs use 0x50 or 0x51.
>> + * @data: Pointer to buffer with EEPROM data of @length size.
>> + *
>> + * This can be used to manage pages during EEPROM dump in ethtool and pass
>> + * required information to the driver.
>> + */
>> +struct ethtool_eeprom_data {
>> + __u32 offset;
>> + __u32 length;
>> + __u8 page;
>> + __u8 bank;
>> + __u8 i2c_address;
>> + __u8 *data;
>> +};
>> +
> This structure does not have to be part of uAPI, right?
Right, will fix.
>> +#define ETH_MODULE_EEPROM_PAGE_LEN 256
>> +#define ETH_MODULE_MAX_I2C_ADDRESS 0x7f
> Not sure about these defines either.
Probably.
>> +static int eeprom_data_prepare_data(const struct ethnl_req_info *req_base,
>> + struct ethnl_reply_data *reply_base,
>> + struct genl_info *info)
>> +{
>> + struct eeprom_data_reply_data *reply = EEPROM_DATA_REPDATA(reply_base);
>> + struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
>> + struct ethtool_eeprom_data page_data = {0};
>> + struct net_device *dev = reply_base->dev;
>> + int ret;
>> +
>> + if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
>> + return -EOPNOTSUPP;
>> +
>> + page_data.offset = request->offset;
>> + page_data.length = request->length;
>> + page_data.i2c_address = request->i2c_address;
>> + page_data.page = request->page;
>> + page_data.bank = request->bank;
>> + page_data.data = kmalloc(page_data.length, GFP_KERNEL);
>> + if (!page_data.data)
>> + return -ENOMEM;
> nit: new line?
Ack.
>> + ret = ethnl_ops_begin(dev);
>> + if (ret)
>> + goto err_free;
>> +
>> + ret = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data,
>> + info->extack);
>> + if (ret < 0)
>> + goto err_ops;
>> +
>> + reply->length = ret;
>> + reply->data = page_data.data;
>> +
>> + ethnl_ops_complete(dev);
>> + return 0;
>> +
>> +err_ops:
>> + ethnl_ops_complete(dev);
>> +err_free:
>> + kfree(page_data.data);
>> + return ret;
>> +}
>> +
>> +static int eeprom_data_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_info);
>> + struct net_device *dev = req_info->dev;
>> +
>> + if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
>> + !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
>> + !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
>> + return -EINVAL;
>> +
>> + request->i2c_address = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
>> + if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
>> + return -EINVAL;
> Max value should be set in the netlink policy.
Ack
>> + request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
>> + request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
>> + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
>> + dev->ethtool_ops->get_module_eeprom_data_by_page &&
> Why check dev->ethtool_ops->get_module_eeprom_data_by_page here?
Right, actually not needed here, thanks.
>> + request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
>> + return -EINVAL;
>> +
>> + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
>> + request->page = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
>> + if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
>> + request->bank = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
>> +
>> + return 0;
>> +}
>> +
>> +static int eeprom_data_reply_size(const struct ethnl_req_info *req_base,
>> + const struct ethnl_reply_data *reply_base)
>> +{
>> + const struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
>> +
>> + return nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_LENGTH */
>> + nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA */
>> +}
> Why report length? Is the netlink length of the DATA attribute not
> sufficient?
Right, thanks.
> Should we echo back offset in case driver wants to report re-aligned
> data?
The driver knows the requested offset and data has to be from that offset.
>> +const struct nla_policy ethnl_eeprom_data_get_policy[] = {
>> + [ETHTOOL_A_EEPROM_DATA_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
>> + [ETHTOOL_A_EEPROM_DATA_OFFSET] = { .type = NLA_U32 },
>> + [ETHTOOL_A_EEPROM_DATA_LENGTH] = { .type = NLA_U32 },
>> + [ETHTOOL_A_EEPROM_DATA_PAGE] = { .type = NLA_U8 },
>> + [ETHTOOL_A_EEPROM_DATA_BANK] = { .type = NLA_U8 },
>> + [ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS] = { .type = NLA_U8 },
>> + [ETHTOOL_A_EEPROM_DATA] = { .type = NLA_BINARY },
> This is a policy for inputs, DATA should not be allowed on input, right?
Right, thanks.
Powered by blists - more mailing lists