| Message ID | 20240709131201.166421-1-kory.maincent@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ethtool: pse-pd: Fix possible null-deref | expand |
On Tue, 9 Jul 2024 15:12:01 +0200 Kory Maincent wrote: > 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 2c981d443f27..9dc70eb50039 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 */ At a glance this doesn't follow usual ethtool flow. If user doesn't specify a value the previous configuration should be kept. We init config to 0. Is 0 a special value for both those params which tells drivers "don't change" ? Normal ethtool flow is to first fill in the data with a ->get() then modify what user wants to change. Either we need: - an explanation in the commit message how this keeps old config; or - a ->get() to keep the previous values; or - just reject setting one value but not the other in ethnl_set_pse_validate() (assuming it never worked, anyway).
On Tue, 9 Jul 2024 07:18:46 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > > - 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 */ > > At a glance this doesn't follow usual ethtool flow. > If user doesn't specify a value the previous configuration should be > kept. We init config to 0. Is 0 a special value for both those params > which tells drivers "don't change" ? Mmh in case of a PSE controller supporting PoE or PoDL on its ports, a 0 config value will return a ENOTSUPP error from the PSE core. We might have an issue in that case which doesn't exist for now as there is no such controller. As a PSE port can't be PoE and PoDL maybe the PSE type should be related to the PSE port and not the full PSE driver. > Normal ethtool flow is to first fill in the data with a ->get() then > modify what user wants to change. > > Either we need: > - an explanation in the commit message how this keeps old config; or > - a ->get() to keep the previous values; or > - just reject setting one value but not the other in > ethnl_set_pse_validate() (assuming it never worked, anyway). In fact it is the contrary we can't set both value at the same time because a PSE port can't be a PoE and a PoDL power interface at the same time. Regards,
On Tue, 9 Jul 2024 16:43:05 +0200 Kory Maincent wrote: > > Normal ethtool flow is to first fill in the data with a ->get() then > > modify what user wants to change. > > > > Either we need: > > - an explanation in the commit message how this keeps old config; or > > - a ->get() to keep the previous values; or > > - just reject setting one value but not the other in > > ethnl_set_pse_validate() (assuming it never worked, anyway). > > In fact it is the contrary we can't set both value at the same time because a > PSE port can't be a PoE and a PoDL power interface at the same time. In that case maybe we should have an inverse condition in validate, too? Something like: if ((pse_has_podl(phydev->psec) && GENL_REQ_ATTR_CHECK(info, ETHTOOL_A_PODL_PSE_ADMIN_CONTROL)) || (pse_has_c33(phydev->psec) && GENL_REQ_ATTR_CHECK(info, ETHTOOL_A_C33_PSE_ADMIN_CONTROL))) return -EINVAL; GENL_REQ_ATTR_CHECK will set the extack for us.
On Tue, 9 Jul 2024 08:52:05 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 9 Jul 2024 16:43:05 +0200 Kory Maincent wrote: > > > Normal ethtool flow is to first fill in the data with a ->get() then > > > modify what user wants to change. > > > > > > Either we need: > > > - an explanation in the commit message how this keeps old config; or > > > - a ->get() to keep the previous values; or > > > - just reject setting one value but not the other in > > > ethnl_set_pse_validate() (assuming it never worked, anyway). > > > > In fact it is the contrary we can't set both value at the same time because > > a PSE port can't be a PoE and a PoDL power interface at the same time. > > In that case maybe we should have an inverse condition in validate, too? > Something like: > > if ((pse_has_podl(phydev->psec) && > GENL_REQ_ATTR_CHECK(info, ETHTOOL_A_PODL_PSE_ADMIN_CONTROL)) || > (pse_has_c33(phydev->psec) && > GENL_REQ_ATTR_CHECK(info, ETHTOOL_A_C33_PSE_ADMIN_CONTROL))) > return -EINVAL; > > GENL_REQ_ATTR_CHECK will set the extack for us. I don't think it will work. In a case a PSE controller have one PoDL port and one PoE port. Your code will always return EINVAL if we try to set the config of only one of those ports. I think the patch I sent plus a change in pse-pd core to do nothing in case of an empty config will do. Regards,
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c index 2c981d443f27..9dc70eb50039 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 */
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(-)