[PATCH net-next 0/7] ethtool: Module EEPROM API improvements

[PATCH net-next 0/7] ethtool: Module EEPROM API improvements

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

This patchset contains various improvements to recently introduced
module EEPROM netlink API. Noticed these while adding module EEPROM
write support.

Ido Schimmel (7):
  ethtool: Use correct command name in title
  ethtool: Document correct attribute type
  ethtool: Decrease size of module EEPROM get policy array
  ethtool: Document behavior when module EEPROM bank attribute is
    omitted
  ethtool: Use kernel data types for internal EEPROM struct
  ethtool: Validate module EEPROM length as part of policy
  ethtool: Validate module EEPROM offset as part of policy

 Documentation/networking/ethtool-netlink.rst |  8 +++++---
 include/linux/ethtool.h                      | 12 ++++++------
 include/uapi/linux/ethtool_netlink.h         |  2 +-
 net/ethtool/eeprom.c                         | 13 ++++---------
 net/ethtool/netlink.h                        |  2 +-
 5 files changed, 17 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH net-next 1/7] ethtool: Use correct command name in title

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The command is called 'ETHTOOL_MSG_MODULE_EEPROM_GET', not
'ETHTOOL_MSG_MODULE_EEPROM'.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/ethtool-netlink.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 25131df3c2bd..c3600f9c8988 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1363,8 +1363,8 @@ in an implementation specific way.
 ``ETHTOOL_A_FEC_AUTO`` requests the driver to choose FEC mode based on SFP
 module parameters. This does not mean autonegotiation.
 
-MODULE_EEPROM
-=============
+MODULE_EEPROM_GET
+=================
 
 Fetch module EEPROM data dump.
 This interface is designed to allow dumps of at most 1/2 page at once. This
-- 
2.31.1

[PATCH net-next 2/7] ethtool: Document correct attribute type

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

'ETHTOOL_A_MODULE_EEPROM_DATA' is a binary attribute, not a nested one.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/ethtool-netlink.rst | 2 +-
 include/uapi/linux/ethtool_netlink.h         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index c3600f9c8988..8ae644f800f0 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1388,7 +1388,7 @@ Kernel response contents:
  +---------------------------------------------+--------+---------------------+
  | ``ETHTOOL_A_MODULE_EEPROM_HEADER``          | nested | reply header        |
  +---------------------------------------------+--------+---------------------+
- | ``ETHTOOL_A_MODULE_EEPROM_DATA``            | nested | array of bytes from |
+ | ``ETHTOOL_A_MODULE_EEPROM_DATA``            | binary | array of bytes from |
  |                                             |        | module EEPROM       |
  +---------------------------------------------+--------+---------------------+
 
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 825cfda1c5d5..c7135c9c37a5 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -675,7 +675,7 @@ enum {
 	ETHTOOL_A_MODULE_EEPROM_PAGE,			/* u8 */
 	ETHTOOL_A_MODULE_EEPROM_BANK,			/* u8 */
 	ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS,		/* u8 */
-	ETHTOOL_A_MODULE_EEPROM_DATA,			/* nested */
+	ETHTOOL_A_MODULE_EEPROM_DATA,			/* binary */
 
 	__ETHTOOL_A_MODULE_EEPROM_CNT,
 	ETHTOOL_A_MODULE_EEPROM_MAX = (__ETHTOOL_A_MODULE_EEPROM_CNT - 1)
-- 
2.31.1

[PATCH net-next 3/7] ethtool: Decrease size of module EEPROM get policy array

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The 'ETHTOOL_A_MODULE_EEPROM_DATA' attribute is not part of the get
request.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ethtool/netlink.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 90b10966b16b..3e25a47fd482 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -380,7 +380,7 @@ extern const struct nla_policy ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_T
 extern const struct nla_policy ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1];
 extern const struct nla_policy ethnl_fec_get_policy[ETHTOOL_A_FEC_HEADER + 1];
 extern const struct nla_policy ethnl_fec_set_policy[ETHTOOL_A_FEC_AUTO + 1];
-extern const struct nla_policy ethnl_module_eeprom_get_policy[ETHTOOL_A_MODULE_EEPROM_DATA + 1];
+extern const struct nla_policy ethnl_module_eeprom_get_policy[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS + 1];
 extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1];
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
-- 
2.31.1

[PATCH net-next 4/7] ethtool: Document behavior when module EEPROM bank attribute is omitted

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The kernel assumes bank 0 when 'ETHTOOL_MSG_MODULE_EEPROM_GET' is sent
without 'ETHTOOL_A_MODULE_EEPROM_BANK'.

Document it as part of the interface documentation.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/ethtool-netlink.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 8ae644f800f0..6ea91e41593f 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1383,6 +1383,8 @@ Request contents:
   ``ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS``  u8      page I2C address
   =======================================  ======  ==========================
 
+If ``ETHTOOL_A_MODULE_EEPROM_BANK`` is not specified, bank 0 is assumed.
+
 Kernel response contents:
 
  +---------------------------------------------+--------+---------------------+
-- 
2.31.1

[PATCH net-next 5/7] ethtool: Use kernel data types for internal EEPROM struct

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The struct is not visible to user space and therefore should not use the
user visible data types.

Instead, use internal data types like other structures in the file.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 include/linux/ethtool.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index e030f7510cd3..29dbb603bc91 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -401,12 +401,12 @@ struct ethtool_rmon_stats {
  * required information to the driver.
  */
 struct ethtool_module_eeprom {
-	__u32	offset;
-	__u32	length;
-	__u8	page;
-	__u8	bank;
-	__u8	i2c_address;
-	__u8	*data;
+	u32	offset;
+	u32	length;
+	u8	page;
+	u8	bank;
+	u8	i2c_address;
+	u8	*data;
 };
 
 /**
-- 
2.31.1

[PATCH net-next 6/7] ethtool: Validate module EEPROM length as part of policy

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Validate the number of bytes to read from the module EEPROM as part of
the netlink policy and remove the corresponding check from the code.

This also makes it possible to query the length range from user space:

 $ genl ctrl policy name ethtool
 ...
 ID: 0x14  policy[32]:attr[3]: type=U32 range:[1,128]
 ...

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ethtool/eeprom.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index 5d38e90895ac..1e75d9c1b154 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -159,9 +159,6 @@ static int eeprom_parse_request(struct ethnl_req_info *req_info, struct nlattr *
 	request->offset = nla_get_u32(tb[ETHTOOL_A_MODULE_EEPROM_OFFSET]);
 	request->length = nla_get_u32(tb[ETHTOOL_A_MODULE_EEPROM_LENGTH]);
 
-	if (!request->length)
-		return -EINVAL;
-
 	/* The following set of conditions limit the API to only dump 1/2
 	 * EEPROM page without crossing low page boundary located at offset 128.
 	 * This means user may only request dumps of length limited to 128 from
@@ -237,7 +234,8 @@ const struct ethnl_request_ops ethnl_module_eeprom_request_ops = {
 const struct nla_policy ethnl_module_eeprom_get_policy[] = {
 	[ETHTOOL_A_MODULE_EEPROM_HEADER]	= NLA_POLICY_NESTED(ethnl_header_policy),
 	[ETHTOOL_A_MODULE_EEPROM_OFFSET]	= { .type = NLA_U32 },
-	[ETHTOOL_A_MODULE_EEPROM_LENGTH]	= { .type = NLA_U32 },
+	[ETHTOOL_A_MODULE_EEPROM_LENGTH]	=
+		NLA_POLICY_RANGE(NLA_U32, 1, ETH_MODULE_EEPROM_PAGE_LEN),
 	[ETHTOOL_A_MODULE_EEPROM_PAGE]		= { .type = NLA_U8 },
 	[ETHTOOL_A_MODULE_EEPROM_BANK]		= { .type = NLA_U8 },
 	[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS]	=
-- 
2.31.1

[PATCH net-next 7/7] ethtool: Validate module EEPROM offset as part of policy

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Validate the offset to read from module EEPROM as part of the netlink
policy and remove the corresponding check from the code.

This also makes it possible to query the offset range from user space:

 $ genl ctrl policy name ethtool
 ...
 ID: 0x14  policy[32]:attr[2]: type=U32 range:[0,255]
 ...

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ethtool/eeprom.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index 1e75d9c1b154..7e6b37a54add 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -177,10 +177,6 @@ static int eeprom_parse_request(struct ethnl_req_info *req_info, struct nlattr *
 		NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_MODULE_EEPROM_LENGTH],
 				    "reading cross half page boundary is illegal");
 		return -EINVAL;
-	} else if (request->offset >= ETH_MODULE_EEPROM_PAGE_LEN * 2) {
-		NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_MODULE_EEPROM_OFFSET],
-				    "offset is out of bounds");
-		return -EINVAL;
 	} else if (request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN * 2) {
 		NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_MODULE_EEPROM_LENGTH],
 				    "reading cross page boundary is illegal");
@@ -233,7 +229,8 @@ const struct ethnl_request_ops ethnl_module_eeprom_request_ops = {
 
 const struct nla_policy ethnl_module_eeprom_get_policy[] = {
 	[ETHTOOL_A_MODULE_EEPROM_HEADER]	= NLA_POLICY_NESTED(ethnl_header_policy),
-	[ETHTOOL_A_MODULE_EEPROM_OFFSET]	= { .type = NLA_U32 },
+	[ETHTOOL_A_MODULE_EEPROM_OFFSET]	=
+		NLA_POLICY_MAX(NLA_U32, ETH_MODULE_EEPROM_PAGE_LEN * 2 - 1),
 	[ETHTOOL_A_MODULE_EEPROM_LENGTH]	=
 		NLA_POLICY_RANGE(NLA_U32, 1, ETH_MODULE_EEPROM_PAGE_LEN),
 	[ETHTOOL_A_MODULE_EEPROM_PAGE]		= { .type = NLA_U8 },
-- 
2.31.1

Re: [PATCH net-next 0/7] ethtool: Module EEPROM API improvements

[Jakub Kicinski]

On Tue, 22 Jun 2021 09:50:45 +0300 Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> This patchset contains various improvements to recently introduced
> module EEPROM netlink API. Noticed these while adding module EEPROM
> write support.

Scary that 3/7 was not generating a warning.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next 0/7] ethtool: Module EEPROM API improvements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Tue, 22 Jun 2021 09:50:45 +0300 you wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> This patchset contains various improvements to recently introduced
> module EEPROM netlink API. Noticed these while adding module EEPROM
> write support.
> 
> Ido Schimmel (7):
>   ethtool: Use correct command name in title
>   ethtool: Document correct attribute type
>   ethtool: Decrease size of module EEPROM get policy array
>   ethtool: Document behavior when module EEPROM bank attribute is
>     omitted
>   ethtool: Use kernel data types for internal EEPROM struct
>   ethtool: Validate module EEPROM length as part of policy
>   ethtool: Validate module EEPROM offset as part of policy
> 
> [...]

Here is the summary with links:
  - [net-next,1/7] ethtool: Use correct command name in title
    https://git.kernel.org/netdev/net-next/c/78c57f22e3c8
  - [net-next,2/7] ethtool: Document correct attribute type
    https://git.kernel.org/netdev/net-next/c/913d026fbfaf
  - [net-next,3/7] ethtool: Decrease size of module EEPROM get policy array
    https://git.kernel.org/netdev/net-next/c/f5fe211d13af
  - [net-next,4/7] ethtool: Document behavior when module EEPROM bank attribute is omitted
    https://git.kernel.org/netdev/net-next/c/37a025e83902
  - [net-next,5/7] ethtool: Use kernel data types for internal EEPROM struct
    https://git.kernel.org/netdev/net-next/c/b8c48be23c2d
  - [net-next,6/7] ethtool: Validate module EEPROM length as part of policy
    https://git.kernel.org/netdev/net-next/c/0dc7dd02ba7a
  - [net-next,7/7] ethtool: Validate module EEPROM offset as part of policy
    https://git.kernel.org/netdev/net-next/c/88f9a87afeee

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 0/7] ethtool: Module EEPROM API improvements

[Michal Kubecek]

On Tue, Jun 22, 2021 at 09:30:05AM -0700, Jakub Kicinski wrote:
> On Tue, 22 Jun 2021 09:50:45 +0300 Ido Schimmel wrote:
> > From: Ido Schimmel <idosch@nvidia.com>
> > 
> > This patchset contains various improvements to recently introduced
> > module EEPROM netlink API. Noticed these while adding module EEPROM
> > write support.
> 
> Scary that 3/7 was not generating a warning.

Actually, it's not as scary as it seems. If an array variable is
declared with fixed size and then defined and initialized with just
"[]", the number of elements is still taken from the declaration, not
from the maximum initializer index. So there was nothing to worry about,
we just had a partially initialized array with extra elements zero
initialized.

Michal