[PATCH net v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device

[PATCH net v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device

[Maxime Chevallier]

ethnl_req_get_phydev() is used to lookup a phy_device, in the case an
ethtool netlink command targets a specific phydev within a netdev's
topology.

It takes as a parameter a const struct nlattr *header that's used for
error handling :

       if (!phydev) {
               NL_SET_ERR_MSG_ATTR(extack, header,
                                   "no phy matching phyindex");
               return ERR_PTR(-ENODEV);
       }

In the notify path after a ->set operation however, there's no request
attributes available.

The typical callsite for the above function looks like:

	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_XXX_HEADER],
				      info->extack);

So, when tb is NULL (such as in the ethnl notify path), we have a nice
crash.

It turns out that there's only the PLCA command that is in that case, as
the other phydev-specific commands don't have a notification.

This commit fixes the crash by passing the cmd index and the nlattr
array separately, allowing NULL-checking it directly inside the helper.

Fixes: c15e065b46dc ("net: ethtool: Allow passing a phy index for some commands")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---

V2: Send this patch as a standalone fix instead of including it in a
series [1] containing patches that don't belong to net. No modifications
were made besides a rebase on -net.

[1]: https://lore.kernel.org/netdev/20250227182454.1998236-1-maxime.chevallier@bootlin.com/

 net/ethtool/cabletest.c | 8 ++++----
 net/ethtool/linkstate.c | 2 +-
 net/ethtool/netlink.c   | 6 +++---
 net/ethtool/netlink.h   | 5 +++--
 net/ethtool/phy.c       | 2 +-
 net/ethtool/plca.c      | 6 +++---
 net/ethtool/pse-pd.c    | 4 ++--
 net/ethtool/stats.c     | 2 +-
 net/ethtool/strset.c    | 2 +-
 9 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index f22051f33868..84096f6b0236 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -72,8 +72,8 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	dev = req_info.dev;
 
 	rtnl_lock();
-	phydev = ethnl_req_get_phydev(&req_info,
-				      tb[ETHTOOL_A_CABLE_TEST_HEADER],
+	phydev = ethnl_req_get_phydev(&req_info, tb,
+				      ETHTOOL_A_CABLE_TEST_HEADER,
 				      info->extack);
 	if (IS_ERR_OR_NULL(phydev)) {
 		ret = -EOPNOTSUPP;
@@ -339,8 +339,8 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 		goto out_dev_put;
 
 	rtnl_lock();
-	phydev = ethnl_req_get_phydev(&req_info,
-				      tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
+	phydev = ethnl_req_get_phydev(&req_info, tb,
+				      ETHTOOL_A_CABLE_TEST_TDR_HEADER,
 				      info->extack);
 	if (IS_ERR_OR_NULL(phydev)) {
 		ret = -EOPNOTSUPP;
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index af19e1bed303..05a5f72c99fa 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -103,7 +103,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_LINKSTATE_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_LINKSTATE_HEADER,
 				      info->extack);
 	if (IS_ERR(phydev)) {
 		ret = PTR_ERR(phydev);
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index b4c45207fa32..734849a57369 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -211,7 +211,7 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 }
 
 struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
-					const struct nlattr *header,
+					struct nlattr **tb, unsigned int header,
 					struct netlink_ext_ack *extack)
 {
 	struct phy_device *phydev;
@@ -225,8 +225,8 @@ struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
 		return req_info->dev->phydev;
 
 	phydev = phy_link_topo_get_phy(req_info->dev, req_info->phy_index);
-	if (!phydev) {
-		NL_SET_ERR_MSG_ATTR(extack, header,
+	if (!phydev && tb) {
+		NL_SET_ERR_MSG_ATTR(extack, tb[header],
 				    "no phy matching phyindex");
 		return ERR_PTR(-ENODEV);
 	}
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index ff69ca0715de..ec6ab5443a6f 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -275,7 +275,8 @@ static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
  * ethnl_req_get_phydev() - Gets the phy_device targeted by this request,
  *			    if any. Must be called under rntl_lock().
  * @req_info:	The ethnl request to get the phy from.
- * @header:	The netlink header, used for error reporting.
+ * @tb:		The netlink attributes array, for error reporting.
+ * @header:	The netlink header index, used for error reporting.
  * @extack:	The netlink extended ACK, for error reporting.
  *
  * The caller must hold RTNL, until it's done interacting with the returned
@@ -289,7 +290,7 @@ static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
  *	   is returned.
  */
 struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
-					const struct nlattr *header,
+					struct nlattr **tb, unsigned int header,
 					struct netlink_ext_ack *extack);
 
 /**
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index ed8f690f6bac..e067cc234419 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -125,7 +125,7 @@ static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
 	struct phy_req_info *req_info = PHY_REQINFO(req_base);
 	struct phy_device *phydev;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PHY_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PHY_HEADER,
 				      extack);
 	if (!phydev)
 		return 0;
diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
index d95d92f173a6..e1f7820a6158 100644
--- a/net/ethtool/plca.c
+++ b/net/ethtool/plca.c
@@ -62,7 +62,7 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PLCA_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PLCA_HEADER,
 				      info->extack);
 	// check that the PHY device is available and connected
 	if (IS_ERR_OR_NULL(phydev)) {
@@ -152,7 +152,7 @@ ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 	bool mod = false;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PLCA_HEADER],
+	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PLCA_HEADER,
 				      info->extack);
 	// check that the PHY device is available and connected
 	if (IS_ERR_OR_NULL(phydev))
@@ -211,7 +211,7 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PLCA_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PLCA_HEADER,
 				      info->extack);
 	// check that the PHY device is available and connected
 	if (IS_ERR_OR_NULL(phydev)) {
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 2819e2ba6be2..4f6b99eab2a6 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -64,7 +64,7 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PSE_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PSE_HEADER,
 				      info->extack);
 	if (IS_ERR(phydev))
 		return -ENODEV;
@@ -261,7 +261,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
+	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PSE_HEADER,
 				      info->extack);
 	ret = ethnl_set_pse_validate(phydev, info);
 	if (ret)
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 038a2558f052..3ca8eb2a3b31 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -138,7 +138,7 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_STATS_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_STATS_HEADER,
 				      info->extack);
 	if (IS_ERR(phydev))
 		return PTR_ERR(phydev);
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 6b76c05caba4..f6a67109beda 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -309,7 +309,7 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
 		return 0;
 	}
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_HEADER_FLAGS],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_HEADER_FLAGS,
 				      info->extack);
 
 	/* phydev can be NULL, check for errors only */
-- 
2.48.1

Re: [PATCH net v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device

[Kory Maincent]

On Sat,  1 Mar 2025 15:11:13 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> ethnl_req_get_phydev() is used to lookup a phy_device, in the case an
> ethtool netlink command targets a specific phydev within a netdev's
> topology.
> 
> It takes as a parameter a const struct nlattr *header that's used for
> error handling :
> 
>        if (!phydev) {
>                NL_SET_ERR_MSG_ATTR(extack, header,
>                                    "no phy matching phyindex");
>                return ERR_PTR(-ENODEV);
>        }
> 
> In the notify path after a ->set operation however, there's no request
> attributes available.
> 
> The typical callsite for the above function looks like:
> 
> 	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_XXX_HEADER],
> 				      info->extack);
> 
> So, when tb is NULL (such as in the ethnl notify path), we have a nice
> crash.
> 
> It turns out that there's only the PLCA command that is in that case, as
> the other phydev-specific commands don't have a notification.
> 
> This commit fixes the crash by passing the cmd index and the nlattr
> array separately, allowing NULL-checking it directly inside the helper.
> 
> Fixes: c15e065b46dc ("net: ethtool: Allow passing a phy index for some
> commands") Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

Thank you!

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device

[Jakub Kicinski]

On Sat,  1 Mar 2025 15:11:13 +0100 Maxime Chevallier wrote:
> ethnl_req_get_phydev() is used to lookup a phy_device, in the case an
> ethtool netlink command targets a specific phydev within a netdev's
> topology.
> 
> It takes as a parameter a const struct nlattr *header that's used for
> error handling :
> 
>        if (!phydev) {
>                NL_SET_ERR_MSG_ATTR(extack, header,
>                                    "no phy matching phyindex");
>                return ERR_PTR(-ENODEV);
>        }
> 
> In the notify path after a ->set operation however, there's no request
> attributes available.
> 
> The typical callsite for the above function looks like:
> 
> 	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_XXX_HEADER],
> 				      info->extack);
> 
> So, when tb is NULL (such as in the ethnl notify path), we have a nice
> crash.
> 
> It turns out that there's only the PLCA command that is in that case, as
> the other phydev-specific commands don't have a notification.
> 
> This commit fixes the crash by passing the cmd index and the nlattr
> array separately, allowing NULL-checking it directly inside the helper.
> 
> Fixes: c15e065b46dc ("net: ethtool: Allow passing a phy index for some commands")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Well, this alone doesn't look too bad.. :) Hopefully we can address
adding more suitable handlers for phy ops in net-next.

Didn't someone report this, tho? I vaguely remember seeing an email,
unless they said they don't want to be credited maybe we should add
a Reported-by tag? You can post it in reply, no need to repost 
the patch.

Re: [PATCH net v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device

[Maxime Chevallier]

On Mon, 3 Mar 2025 16:10:24 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Sat,  1 Mar 2025 15:11:13 +0100 Maxime Chevallier wrote:
> > ethnl_req_get_phydev() is used to lookup a phy_device, in the case an
> > ethtool netlink command targets a specific phydev within a netdev's
> > topology.
> > 
> > It takes as a parameter a const struct nlattr *header that's used for
> > error handling :
> > 
> >        if (!phydev) {
> >                NL_SET_ERR_MSG_ATTR(extack, header,
> >                                    "no phy matching phyindex");
> >                return ERR_PTR(-ENODEV);
> >        }
> > 
> > In the notify path after a ->set operation however, there's no request
> > attributes available.
> > 
> > The typical callsite for the above function looks like:
> > 
> > 	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_XXX_HEADER],
> > 				      info->extack);
> > 
> > So, when tb is NULL (such as in the ethnl notify path), we have a nice
> > crash.
> > 
> > It turns out that there's only the PLCA command that is in that case, as
> > the other phydev-specific commands don't have a notification.
> > 
> > This commit fixes the crash by passing the cmd index and the nlattr
> > array separately, allowing NULL-checking it directly inside the helper.
> > 
> > Fixes: c15e065b46dc ("net: ethtool: Allow passing a phy index for some commands")
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>  
> 
> Well, this alone doesn't look too bad.. :) Hopefully we can address
> adding more suitable handlers for phy ops in net-next.

Yeah I'm cooking something to improve on that, and I have also dusted
off a netdevsim patch I had written back when working on the
phy_link_topology that adds very very basic PHY support so that we can
start covering all these commands with proper tests. I'll hopefully send
something in the coming week or so.

> Didn't someone report this, tho? I vaguely remember seeing an email,
> unless they said they don't want to be credited maybe we should add
> a Reported-by tag? You can post it in reply, no need to repost 
> the patch.

Parthiban reported this without CC: netdev, but I think it's fair to
add :

Reported-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>

I didn't include it in the first place because checkpatch complained
about a reported-by tag without a "Closes:", which we don't have
because of the private reporting :)

Thanks Jakub,

Maxime

Re: [PATCH net v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device

[Parthiban.Veerasooran]

Hi All,

On 04/03/25 3:22 pm, Maxime Chevallier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, 3 Mar 2025 16:10:24 -0800
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
>> On Sat,  1 Mar 2025 15:11:13 +0100 Maxime Chevallier wrote:
>>> ethnl_req_get_phydev() is used to lookup a phy_device, in the case an
>>> ethtool netlink command targets a specific phydev within a netdev's
>>> topology.
>>>
>>> It takes as a parameter a const struct nlattr *header that's used for
>>> error handling :
>>>
>>>         if (!phydev) {
>>>                 NL_SET_ERR_MSG_ATTR(extack, header,
>>>                                     "no phy matching phyindex");
>>>                 return ERR_PTR(-ENODEV);
>>>         }
>>>
>>> In the notify path after a ->set operation however, there's no request
>>> attributes available.
>>>
>>> The typical callsite for the above function looks like:
>>>
>>>      phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_XXX_HEADER],
>>>                                    info->extack);
>>>
>>> So, when tb is NULL (such as in the ethnl notify path), we have a nice
>>> crash.
>>>
>>> It turns out that there's only the PLCA command that is in that case, as
>>> the other phydev-specific commands don't have a notification.
>>>
>>> This commit fixes the crash by passing the cmd index and the nlattr
>>> array separately, allowing NULL-checking it directly inside the helper.
>>>
>>> Fixes: c15e065b46dc ("net: ethtool: Allow passing a phy index for some commands")
>>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>>
>> Well, this alone doesn't look too bad.. :) Hopefully we can address
>> adding more suitable handlers for phy ops in net-next.
> 
> Yeah I'm cooking something to improve on that, and I have also dusted
> off a netdevsim patch I had written back when working on the
> phy_link_topology that adds very very basic PHY support so that we can
> start covering all these commands with proper tests. I'll hopefully send
> something in the coming week or so.
> 
>> Didn't someone report this, tho? I vaguely remember seeing an email,
>> unless they said they don't want to be credited maybe we should add
>> a Reported-by tag? You can post it in reply, no need to repost
>> the patch.
> 
> Parthiban reported this without CC: netdev, but I think it's fair to
> add :
> 
> Reported-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
Thank you for fixing the issue.

Best regards,
Parthiban V
> 
> I didn't include it in the first place because checkpatch complained
> about a reported-by tag without a "Closes:", which we don't have
> because of the private reporting :)
> 
> Thanks Jakub,
> 
> Maxime

Re: [PATCH net v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sat,  1 Mar 2025 15:11:13 +0100 you wrote:
> ethnl_req_get_phydev() is used to lookup a phy_device, in the case an
> ethtool netlink command targets a specific phydev within a netdev's
> topology.
> 
> It takes as a parameter a const struct nlattr *header that's used for
> error handling :
> 
> [...]

Here is the summary with links:
  - [net,v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device
    https://git.kernel.org/netdev/net/c/637399bf7e77

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