[PATCH net v3 0/2] net: pse-pd: Fix possible issues with a PSE supporting both c33 and PoDL

[PATCH net v3 0/2] net: pse-pd: Fix possible issues with a PSE supporting both c33 and PoDL

[Kory Maincent]

Although PSE controllers supporting both c33 and PoDL are not on the
market yet, we want to prevent potential issues from arising in the
future. Two possible issues could occur with a PSE supporting both c33
and PoDL:

- Setting the config for one type of PSE leaves the other type's config
  null. In this case, the PSE core would return EOPNOTSUPP, which is not
  the correct behavior.
- Null dereference of Netlink attributes as only one of the Netlink
  attributes would be specified at a time.

This patch series contains two patches to fix these issues.

Changes in v3:
- Add a cover letter and the net prefix

Changes in v2:
- Add a patch to deal with null config

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Kory Maincent (2):
      net: pse-pd: Do not return EOPNOSUPP if config is null
      net: ethtool: pse-pd: Fix possible null-deref

 drivers/net/pse-pd/pse_core.c | 4 ++--
 net/ethtool/pse-pd.c          | 8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)
---
base-commit: d7c199e77ef2fe259ad5b1beca5ddd6c951fcba2
change-id: 20240711-fix_pse_pd_deref-6a7d0de068a4

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

[PATCH net v3 1/2] net: pse-pd: Do not return EOPNOSUPP if config is null

[Kory Maincent]

For a PSE supporting both c33 and PoDL, setting config for one type of PoE
leaves the other type's config null. Currently, this case returns
EOPNOTSUPP, which is incorrect. Instead, we should do nothing if the
configuration is empty.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")
---

Changes in v2:
- New patch to fix dealing with a null config.
---
 drivers/net/pse-pd/pse_core.c | 4 ++--
 net/ethtool/pse-pd.c          | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 795ab264eaf2..513cd7f85933 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -719,13 +719,13 @@ int pse_ethtool_set_config(struct pse_control *psec,
 {
 	int err = 0;
 
-	if (pse_has_c33(psec)) {
+	if (pse_has_c33(psec) && config->c33_admin_control) {
 		err = pse_ethtool_c33_set_config(psec, config);
 		if (err)
 			return err;
 	}
 
-	if (pse_has_podl(psec))
+	if (pse_has_podl(psec) && config->podl_admin_control)
 		err = pse_ethtool_podl_set_config(psec, config);
 
 	return err;
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 2c981d443f27..982995ff1628 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -183,7 +183,9 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 	if (pse_has_c33(phydev->psec))
 		config.c33_admin_control = nla_get_u32(tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]);
 
-	/* Return errno directly - PSE has no notification */
+	/* Return errno directly - PSE has no notification
+	 * pse_ethtool_set_config() will do nothing if the config is null
+	 */
 	return pse_ethtool_set_config(phydev->psec, info->extack, &config);
 }
 

-- 
2.34.1

[PATCH net v3 2/2] net: ethtool: pse-pd: Fix possible null-deref

[Kory Maincent]

Fix a possible null dereference when a PSE supports both c33 and PoDL, but
only one of the netlink attributes is specified. The c33 or PoDL PSE
capabilities are already validated in the ethnl_set_pse_validate() call.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reported-by: Jakub Kicinski <kuba@kernel.org>
Closes: https://lore.kernel.org/netdev/20240705184116.13d8235a@kernel.org/
Fixes: 4d18e3ddf427 ("net: ethtool: pse-pd: Expand pse commands with the PSE PoE interface")
---
 net/ethtool/pse-pd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 982995ff1628..776ac96cdadc 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -178,9 +178,9 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	phydev = dev->phydev;
 	/* These values are already validated by the ethnl_pse_set_policy */
-	if (pse_has_podl(phydev->psec))
+	if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL])
 		config.podl_admin_control = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]);
-	if (pse_has_c33(phydev->psec))
+	if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL])
 		config.c33_admin_control = nla_get_u32(tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]);
 
 	/* Return errno directly - PSE has no notification

-- 
2.34.1

Re: [PATCH net v3 0/2] net: pse-pd: Fix possible issues with a PSE supporting both c33 and PoDL

[patchwork-bot+netdevbpf]

Hello:

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

On Thu, 11 Jul 2024 15:55:17 +0200 you wrote:
> Although PSE controllers supporting both c33 and PoDL are not on the
> market yet, we want to prevent potential issues from arising in the
> future. Two possible issues could occur with a PSE supporting both c33
> and PoDL:
> 
> - Setting the config for one type of PSE leaves the other type's config
>   null. In this case, the PSE core would return EOPNOTSUPP, which is not
>   the correct behavior.
> - Null dereference of Netlink attributes as only one of the Netlink
>   attributes would be specified at a time.
> 
> [...]

Here is the summary with links:
  - [net,v3,1/2] net: pse-pd: Do not return EOPNOSUPP if config is null
    https://git.kernel.org/netdev/net/c/93c3a96c301f
  - [net,v3,2/2] net: ethtool: pse-pd: Fix possible null-deref
    https://git.kernel.org/netdev/net/c/4cddb0f15ea9

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