| 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: <d0dcd510-520b-92ec-67fd-da50970d10a8@nvidia.com>
Date: Mon, 8 Mar 2021 10:45:25 +0200
From: Moshe Shemesh <moshe@...dia.com>
To: Don Bollinger <don@...bollingers.org>,
"'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>
Subject: Re: [RFC PATCH V2 net-next 1/5] ethtool: Allow network drivers to
dump arbitrary EEPROM data
On 3/5/2021 2:50 AM, Don Bollinger wrote:
>
> On Thu, Mar 04, 2021 at 10:57AM-0800, 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>
>> ---
>> include/linux/ethtool.h | 7 +-
>> include/uapi/linux/ethtool.h | 26 +++++
>> include/uapi/linux/ethtool_netlink.h | 19 ++++
>> net/ethtool/Makefile | 2 +-
>> net/ethtool/eeprom.c | 157 +++++++++++++++++++++++++++
>> net/ethtool/netlink.c | 10 ++
>> net/ethtool/netlink.h | 2 +
>> 7 files changed, 221 insertions(+), 2 deletions(-) create mode 100644
>> net/ethtool/eeprom.c
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index
>> ec4cd3921c67..2f65aae5f492 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -81,6 +81,7 @@ enum {
>> #define ETH_RSS_HASH_NO_CHANGE 0
>>
>> struct net_device;
>> +struct netlink_ext_ack;
>>
>> /* Some generic methods drivers may use in their ethtool_ops */
>> u32 ethtool_op_get_link(struct net_device *dev); @@ -410,6 +411,8 @@
>> struct ethtool_pause_stats {
>> * @get_ethtool_phy_stats: Return extended statistics about the PHY
>> device.
>> * This is only useful if the device maintains PHY statistics and
>> * cannot use the standard PHY library helpers.
>> + * @get_module_eeprom_data_by_page: Get a region of plug-in module
>> EEPROM data
>> + * from specified page. Returns a negative error code or zero.
>> *
>> * All operations are optional (i.e. the function pointer may be set
>> * to %NULL) and callers must take this into account. Callers must @@
> -515,6
>> +518,9 @@ struct ethtool_ops {
>> const struct ethtool_tunable *, void *);
>> int (*set_phy_tunable)(struct net_device *,
>> const struct ethtool_tunable *, const
> void
>> *);
>> + int (*get_module_eeprom_data_by_page)(struct net_device
>> *dev,
>> + const struct
>> ethtool_eeprom_data *page,
>> + struct netlink_ext_ack
>> *extack);
>> };
>>
>> int ethtool_check_ops(const struct ethtool_ops *ops); @@ -538,7 +544,6
>> @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>> const struct ethtool_link_ksettings
> *cmd,
>> u32 *dev_speed, u8 *dev_duplex);
>>
>> -struct netlink_ext_ack;
>> struct phy_device;
>> struct phy_tdr_config;
>>
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index
>> cde753bb2093..2459571fc1d1 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.
> Note here that bytes at offset 0-127 are the same for every page of the
> module, only bytes at offset 128 and higher are actually paged.
Right, but still having the offset relative to 128 will be confusing.
>> + * @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.
> The standards are all very clear, the only legal values are 0x50 and 0x51.
> It isn't 'expected', it is required. I suggest that 0xA0 and 0xA2 also be
> silently accepted, and translated to 0x50 and 0x51 respectively. Some
> of the specs use A0/A2 instead of 0x50/0x51. They actually mean the
> same thing.
>
>> + * @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;
>> + __u32 page;
>> + __u32 bank;
>> + __u32 i2c_address;
>> + __u8 *data;
>> +};
>> +
>> /**
>> * struct ethtool_eee - Energy Efficient Ethernet information
>> * @cmd: ETHTOOL_{G,S}EEE
>> @@ -1865,6 +1887,10 @@ static inline int ethtool_validate_duplex(__u8
>> duplex)
>> #define ETH_MODULE_SFF_8636_MAX_LEN 640
>> #define ETH_MODULE_SFF_8436_MAX_LEN 640
>>
>> +#define ETH_MODULE_EEPROM_MAX_LEN 640
> Please don't add this MAX_LEN constant. Even better, remove
> the two above it as well.
These constants are relevant to the ioctl function and used by the
drivers. For netlink new KAPI I will remove the new MAX_LEN per your
explanation, thanks.
> The proper value for all 3 of these MAX_LEN items is the
> architectural limit imposed by the 8 bit page register plus the constant
> lower page (hence 257*128 bytes). The 8436 and 8636
> specs do not actually limit these devices to 640 bytes (3 pages).
>
> There is no MAX_LEN listed for SFF_8472. If there is one, it should
> actually be 259 * 128 bytes (to account for 256 more bytes on the
> unpaged 0x50 i2c address).
>
> Nor is there one for CMIS. The maximum
> architected length for CMIS is (257*128) + (127 * 16 * 128). That's
> the QSFP max length plus 127 more banks of 16 pages.
>
>> +#define ETH_MODULE_EEPROM_PAGE_LEN 256
>> +#define ETH_MODULE_MAX_I2C_ADDRESS 0x7f
> Actually there are only two legal values for the i2c address (0x50, 0x51).
> Rather than defining a MAX address, consider defining the legal values,
> or... is it used at all? Leave it out?
As Andrew commented there might be usage of other i2c adresses, I will
keep it.
>> +
>> /* Reset flags */
>> /* The reset() operation must clear the flags for the components which
>> * were actually reset. On successful return, the flags indicate the
> diff --git
>> a/include/uapi/linux/ethtool_netlink.h
>> b/include/uapi/linux/ethtool_netlink.h
>> index a286635ac9b8..60dd848d0b54 100644
>> --- a/include/uapi/linux/ethtool_netlink.h
>> +++ b/include/uapi/linux/ethtool_netlink.h
>> @@ -42,6 +42,7 @@ enum {
>> ETHTOOL_MSG_CABLE_TEST_ACT,
>> ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
>> ETHTOOL_MSG_TUNNEL_INFO_GET,
>> + ETHTOOL_MSG_EEPROM_DATA_GET,
>>
>> /* add new constants above here */
>> __ETHTOOL_MSG_USER_CNT,
>> @@ -80,6 +81,7 @@ enum {
>> ETHTOOL_MSG_CABLE_TEST_NTF,
>> ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
>> ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
>> + ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
>>
>> /* add new constants above here */
>> __ETHTOOL_MSG_KERNEL_CNT,
>> @@ -629,6 +631,23 @@ enum {
>> ETHTOOL_A_TUNNEL_INFO_MAX =
>> (__ETHTOOL_A_TUNNEL_INFO_CNT - 1) };
>>
>> +/* MODULE EEPROM DATA */
>> +
>> +enum {
>> + ETHTOOL_A_EEPROM_DATA_UNSPEC,
>> + ETHTOOL_A_EEPROM_DATA_HEADER,
>> +
>> + ETHTOOL_A_EEPROM_DATA_OFFSET,
>> + ETHTOOL_A_EEPROM_DATA_LENGTH,
>> + ETHTOOL_A_EEPROM_DATA_PAGE,
>> + ETHTOOL_A_EEPROM_DATA_BANK,
>> + ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
>> + ETHTOOL_A_EEPROM_DATA,
>> +
>> + __ETHTOOL_A_EEPROM_DATA_CNT,
>> + ETHTOOL_A_EEPROM_DATA_MAX =
>> (__ETHTOOL_A_EEPROM_DATA_CNT - 1) };
>> +
>> /* generic netlink info */
>> #define ETHTOOL_GENL_NAME "ethtool"
>> #define ETHTOOL_GENL_VERSION 1
>> diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile index
>> 7a849ff22dad..d604346bc074 100644
>> --- a/net/ethtool/Makefile
>> +++ b/net/ethtool/Makefile
>> @@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
>> ethtool_nl-y := netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
>> linkstate.o debug.o wol.o features.o privflags.o rings.o
> \
>> channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o
> \
>> - tunnels.o
>> + tunnels.o eeprom.o
>> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c new file mode
>> 100644 index 000000000000..2618a55b9a40
>> --- /dev/null
>> +++ b/net/ethtool/eeprom.c
>> @@ -0,0 +1,157 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/ethtool.h>
>> +#include "netlink.h"
>> +#include "common.h"
>> +
>> +struct eeprom_data_req_info {
>> + struct ethnl_req_info base;
>> + u32 offset;
>> + u32 length;
>> + u32 page;
>> + u32 bank;
>> + u32 i2c_address;
>> +};
>> +
>> +struct eeprom_data_reply_data {
>> + struct ethnl_reply_data base;
>> + u32 length;
>> + u32 i2c_address;
>> + u8 *data;
>> +};
>> +
>> +#define EEPROM_DATA_REQINFO(__req_base) \
>> + container_of(__req_base, struct eeprom_data_req_info, base)
>> +
>> +#define EEPROM_DATA_REPDATA(__reply_base) \
>> + container_of(__reply_base, struct eeprom_data_reply_data, base)
>> +
>> +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 err;
>> +
>> + 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;
>> + err = ethnl_ops_begin(dev);
>> + if (err)
>> + goto err_free;
>> +
>> + err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev,
>> &page_data,
>> +
> info->extack);
>> + if (err)
>> + goto err_ops;
>> +
>> + reply->length = page_data.length;
>> + reply->i2c_address = page_data.i2c_address;
>> + reply->data = page_data.data;
>> +
>> + ethnl_ops_complete(dev);
> The two error paths below kfree(page_data.data). Does someone else
> free this memory when there is no error?
eeprom_data_cleanup_data()
>> + return 0;
>> +
>> +err_ops:
>> + ethnl_ops_complete(dev);
>> +err_free:
>> + kfree(page_data.data);
>> + return err;
>> +}
>> +
>> +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);
>> +
>> + 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_u32(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
>> + if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
>> + return -EINVAL;
> I would be much more restrictive, with one flexibility...
>
> if (request->i2c_address == 0xA0) request->i2c_address = 0x50;
> if (request->i2c_address == 0xA2) request->i2c_address = 0x51;
> if (request->i2c_address < 0x50) || (request->i2c_address > 0x51)
> return -EINVAL;
>
>> +
>> + request->offset =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
>> + request->length =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
>> + if (request->length > ETH_MODULE_EEPROM_MAX_LEN)
>> + return -EINVAL;
> This is really problematic as there are MANY different max values, within
> the specs, for the various EEPROMs being generically supported here.
> I would leave it to the drivers to handle out-of-range requests. If you
> really want to check, you need to know which spec the module supports,
> whether it supports pages, and whether it supports banks. I have not
> found a register that actually reports the number of supported pages
> that an eeprom supports. The specs should have included that :-(.
Will remove this check per your explanation, thanks.
>> + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
>> + request->offset + request->length >
>> ETH_MODULE_EEPROM_PAGE_LEN)
>> + return -EINVAL;
> Why does this stanza depend on DATA_PAGE? In this new data
> structure, no requests can cross the 256 byte page boundary.
>
> I suggest, rather then -EINVAL, you should reduce the length to reach
> the end of the page:
>
> if (request->offset + request->length) > ETH_MODULE_EEPROM_PAGE_LEN)
> request->length = ETH_MODULE_EEPROM_PAGE_LEN -
> request->offset.
Backward compatibility, users use the command without page and bank as
it was till now expect to get data that can cross page as it worked till
now, I can't break it.
> Note that this matches the choice you made to truncate rather than
> error out in fallback_set_parms().
We truncate in fallback_set_params() to eeprom_len.
length = modinfo->eeprom_len - offset;
>> +
>> + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
>> + request->page =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
>> + if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
>> + request->bank =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
> Other checks:
>
> Page and bank have to be between 0 and 255 (inclusive), they
> go into an 8 bit register in the eeprom.
Will add, thanks.
> Offset and length can't be negative.
Already are u32.
>> +
>> + 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(u32)) + /* _EEPROM_DATA_I2C_ADDRESS
>> */
>> + nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA
>> +*/ }
>> +
>> +static int eeprom_data_fill_reply(struct sk_buff *skb,
>> + const struct ethnl_req_info *req_base,
>> + const struct ethnl_reply_data *reply_base)
> {
>> + struct eeprom_data_reply_data *reply =
>> +EEPROM_DATA_REPDATA(reply_base);
>> +
>> + if (nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_LENGTH, reply-
>>> length) ||
>> + nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
>> reply->i2c_address) ||
>> + nla_put(skb, ETHTOOL_A_EEPROM_DATA, reply->length, reply-
>>> data))
>> + return -EMSGSIZE;
>> +
>> + return 0;
>> +}
>> +
>> +static void eeprom_data_cleanup_data(struct ethnl_reply_data
>> +*reply_base) {
>> + struct eeprom_data_reply_data *reply =
>> +EEPROM_DATA_REPDATA(reply_base);
>> +
>> + kfree(reply->data);
>> +}
>> +
>> +const struct ethnl_request_ops ethnl_eeprom_data_request_ops = {
>> + .request_cmd = ETHTOOL_MSG_EEPROM_DATA_GET,
>> + .reply_cmd =
>> ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
>> + .hdr_attr = ETHTOOL_A_EEPROM_DATA_HEADER,
>> + .req_info_size = sizeof(struct eeprom_data_req_info),
>> + .reply_data_size = sizeof(struct eeprom_data_reply_data),
>> +
>> + .parse_request = eeprom_data_parse_request,
>> + .prepare_data = eeprom_data_prepare_data,
>> + .reply_size = eeprom_data_reply_size,
>> + .fill_reply = eeprom_data_fill_reply,
>> + .cleanup_data = eeprom_data_cleanup_data,
>> +};
>> +
>> +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_U32 },
>> + [ETHTOOL_A_EEPROM_DATA_BANK] = { .type = NLA_U32 },
>> + [ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS] = { .type = NLA_U32 },
>> + [ETHTOOL_A_EEPROM_DATA] = { .type =
>> NLA_BINARY },
>> +};
>> +
>> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index
>> 50d3c8896f91..ff2528bee192 100644
>> --- a/net/ethtool/netlink.c
>> +++ b/net/ethtool/netlink.c
>> @@ -245,6 +245,7 @@
>> ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
>> [ETHTOOL_MSG_PAUSE_GET] =
>> ðnl_pause_request_ops,
>> [ETHTOOL_MSG_EEE_GET] = ðnl_eee_request_ops,
>> [ETHTOOL_MSG_TSINFO_GET] = ðnl_tsinfo_request_ops,
>> + [ETHTOOL_MSG_EEPROM_DATA_GET] =
>> ðnl_eeprom_data_request_ops,
>> };
>>
>> static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback
>> *cb) @@ -912,6 +913,15 @@ static const struct genl_ops ethtool_genl_ops[]
>> = {
>> .policy = ethnl_tunnel_info_get_policy,
>> .maxattr = ARRAY_SIZE(ethnl_tunnel_info_get_policy) - 1,
>> },
>> + {
>> + .cmd = ETHTOOL_MSG_EEPROM_DATA_GET,
>> + .doit = ethnl_default_doit,
>> + .start = ethnl_default_start,
>> + .dumpit = ethnl_default_dumpit,
>> + .done = ethnl_default_done,
>> + .policy = ethnl_eeprom_data_get_policy,
>> + .maxattr = ARRAY_SIZE(ethnl_eeprom_data_get_policy) - 1,
>> + },
>> };
>>
>> static const struct genl_multicast_group ethtool_nl_mcgrps[] = { diff
> --git
>> a/net/ethtool/netlink.h b/net/ethtool/netlink.h index
>> 6eabd58d81bf..60954c7b4dfe 100644
>> --- a/net/ethtool/netlink.h
>> +++ b/net/ethtool/netlink.h
>> @@ -344,6 +344,7 @@ extern const struct ethnl_request_ops
>> ethnl_coalesce_request_ops; extern const struct ethnl_request_ops
>> ethnl_pause_request_ops; extern const struct ethnl_request_ops
>> ethnl_eee_request_ops; extern const struct ethnl_request_ops
>> ethnl_tsinfo_request_ops;
>> +extern const struct ethnl_request_ops ethnl_eeprom_data_request_ops;
>>
>> extern const struct nla_policy
>> ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1]; extern const struct
>> nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1]; @@
>> -375,6 +376,7 @@ extern const struct nla_policy
>> ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_HEADER + extern const struct
>> nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_HEADER +
>> 1]; extern const struct nla_policy
>> ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG + 1];
>> extern const struct nla_policy
>> ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1];
>> +extern const struct nla_policy
>> +ethnl_eeprom_data_get_policy[ETHTOOL_A_EEPROM_DATA + 1];
>>
>> int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info); int
>> ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
>> --
>> 2.18.2
Powered by blists - more mailing lists