[PATCH net-next] net: dsa: felix: validate switch in .netdev_to_port() conversion

[PATCH net-next] net: dsa: felix: validate switch in .netdev_to_port() conversion

[David Yang]

felix_netdev_to_port() uses dsa_port_from_netdev() without checking
whether the resulting dsa_port actually belongs to the switch being
operated on. The port index is then consumed by flower rule parsing,
silently resolving to a wrong port on the local hardware.

Add a dp->ds mismatch check and adjust struct ocelot_ops accordingly.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/ocelot/felix.c            | 6 ++++--
 drivers/net/dsa/ocelot/felix.h            | 2 +-
 drivers/net/ethernet/mscc/ocelot.h        | 2 +-
 drivers/net/ethernet/mscc/ocelot_flower.c | 4 ++--
 drivers/net/ethernet/mscc/ocelot_net.c    | 2 +-
 include/soc/mscc/ocelot.h                 | 2 +-
 6 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 4272ea6e9ca8..84a95007825f 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -2446,12 +2446,14 @@ struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port)
 }
 EXPORT_SYMBOL_GPL(felix_port_to_netdev);
 
-int felix_netdev_to_port(struct net_device *dev)
+int felix_netdev_to_port(struct ocelot *ocelot, struct net_device *dev)
 {
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct dsa_switch *ds = felix->ds;
 	struct dsa_port *dp;
 
 	dp = dsa_port_from_netdev(dev);
-	if (IS_ERR(dp))
+	if (IS_ERR(dp) || dp->ds != ds)
 		return -EINVAL;
 
 	return dp->index;
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index a657b190c5d7..19addcfd62be 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -104,6 +104,6 @@ int felix_register_switch(struct device *dev, resource_size_t switch_base,
 			  enum dsa_tag_protocol init_tag_proto,
 			  const struct felix_info *info);
 struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port);
-int felix_netdev_to_port(struct net_device *dev);
+int felix_netdev_to_port(struct ocelot *ocelot, struct net_device *dev);
 
 #endif
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index e50be508c166..42d2c456f712 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -92,7 +92,7 @@ int ocelot_mact_learn(struct ocelot *ocelot, int port,
 int ocelot_mact_forget(struct ocelot *ocelot,
 		       const unsigned char mac[ETH_ALEN], unsigned int vid);
 struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port);
-int ocelot_netdev_to_port(struct net_device *dev);
+int ocelot_netdev_to_port(struct ocelot *ocelot, struct net_device *dev);
 
 int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target,
 		      struct device_node *portnp);
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 844292eb7422..0c3aa61c1413 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -234,7 +234,7 @@ ocelot_flower_parse_egress_port(struct ocelot *ocelot, struct flow_cls_offload *
 				struct netlink_ext_ack *extack)
 {
 	const char *act_string = mirror ? "mirror" : "redirect";
-	int egress_port = ocelot->ops->netdev_to_port(a->dev);
+	int egress_port = ocelot->ops->netdev_to_port(ocelot, a->dev);
 	enum flow_action_id offloadable_act_id;
 
 	offloadable_act_id = mirror ? FLOW_ACTION_MIRRED : FLOW_ACTION_REDIRECT;
@@ -580,7 +580,7 @@ static int ocelot_flower_parse_indev(struct ocelot *ocelot, int port,
 		return -ENOENT;
 	}
 
-	ingress_port = ocelot->ops->netdev_to_port(indev);
+	ingress_port = ocelot->ops->netdev_to_port(ocelot, indev);
 	if (ingress_port < 0) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "Can only offload an ocelot ingress port");
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 1b8269320464..d2e96d065dc4 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -984,7 +984,7 @@ static bool ocelot_netdevice_dev_check(const struct net_device *dev)
 	return dev->netdev_ops == &ocelot_port_netdev_ops;
 }
 
-int ocelot_netdev_to_port(struct net_device *dev)
+int ocelot_netdev_to_port(struct ocelot *ocelot, struct net_device *dev)
 {
 	struct ocelot_port_private *priv;
 
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 48d6deb3efd7..05a8191b148e 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -650,7 +650,7 @@ struct device_node;
 
 struct ocelot_ops {
 	struct net_device *(*port_to_netdev)(struct ocelot *ocelot, int port);
-	int (*netdev_to_port)(struct net_device *dev);
+	int (*netdev_to_port)(struct ocelot *ocelot, struct net_device *dev);
 	int (*reset)(struct ocelot *ocelot);
 	u16 (*wm_enc)(u16 value);
 	u16 (*wm_dec)(u16 value);
-- 
2.53.0

Re: [PATCH net-next] net: dsa: felix: validate switch in .netdev_to_port() conversion

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: dsa: felix: validate switch in .netdev_to_port() conversion

This commit changes the .netdev_to_port() callback to take a struct
ocelot pointer and adds a dp->ds mismatch check in felix_netdev_to_port()
so that a netdev belonging to a different switch is rejected before the
port index is consumed by flower rule parsing.

> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 1b8269320464..d2e96d065dc4 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -984,7 +984,7 @@ static bool ocelot_netdevice_dev_check(const struct net_device *dev)
>  	return dev->netdev_ops == &ocelot_port_netdev_ops;
>  }
>  
> -int ocelot_netdev_to_port(struct net_device *dev)
> +int ocelot_netdev_to_port(struct ocelot *ocelot, struct net_device *dev)
>  {
>  	struct ocelot_port_private *priv;

[Medium]
The new ocelot parameter is added here to match the updated
ocelot_ops::netdev_to_port signature, but is it actually used in the
body of ocelot_netdev_to_port()?

The function still does only:

    if (!dev || !ocelot_netdevice_dev_check(dev))
            return -EINVAL;

    priv = netdev_priv(dev);

    return priv->port.index;

ocelot_netdevice_dev_check() compares dev->netdev_ops against the
file-scope ocelot_port_netdev_ops vtable, which is shared across all
ocelot instances, so a netdev belonging to a sibling ocelot switch
passes this check and priv->port.index is returned as a port number
on the local hardware.

Isn't this the same silent wrong-port resolution the commit message
describes for the felix path?

The corresponding switch-ownership check the commit adds for felix:

    if (IS_ERR(dp) || dp->ds != ds)
            return -EINVAL;

has no analogue in the ocelot variant. struct ocelot_port carries a
back-pointer to its owning struct ocelot (used elsewhere in this
file), so a symmetric check such as:

    if (priv->port.ocelot != ocelot)
            return -EINVAL;

after the netdev_ops check would mirror the felix change and consume
the otherwise unused ocelot argument. Without it, the parameter is
dead and the validation promised by the commit message is only
half-applied.

The two call sites in ocelot_flower.c, ocelot_flower_parse_egress_port()
consuming a->dev and ocelot_flower_parse_indev() resolving a netdev
from a user-supplied ifindex via __dev_get_by_index(), both feed
externally-controlled netdevs into ocelot->ops->netdev_to_port(ocelot, dev),
so the standalone ocelot path used by ocelot_vsc7514 still accepts a
foreign switch's netdev when more than one ocelot instance is present.