* Re: [PATCH net] net: dsa: bcm_sf2_cfp: fix NULL deref when port_num is out of bounds
2026-09-05 1:34 [PATCH net] net: dsa: bcm_sf2_cfp: fix NULL deref when port_num is out of bounds Danesh Petigara
@ 2026-09-06 10:06 ` Jonas Gorski
0 siblings, 0 replies; 2+ messages in thread
From: Jonas Gorski @ 2026-09-06 10:06 UTC (permalink / raw)
To: Danesh Petigara
Cc: florian.fainelli, andrew, olteanv, davem, edumazet, kuba, pabeni,
vivien.didelot, netdev, linux-kernel, Justin Chen
Hi,
On Sat, Sep 5, 2026 at 3:34 AM Danesh Petigara
<danesh.petigara@broadcom.com> wrote:
>
> From: Justin Chen <justin.chen@broadcom.com>
>
> bcm_sf2_cfp_rule_insert() validates port_num against
> priv->hw_params.num_ports, but the bounds check was placed after the
> calls to dsa_is_user_port() and dsa_is_cpu_port():
>
> if (ring_cookie == RX_CLS_FLOW_DISC ||
> !(dsa_is_user_port(ds, port_num) ||
> dsa_is_cpu_port(ds, port_num)) ||
> port_num >= priv->hw_params.num_ports)
>
> Both helpers call dsa_to_port(), which iterates the port list and
> returns NULL if no entry matches. With a user-supplied ring_cookie
> large enough to produce an out-of-bounds port_num, dsa_to_port()
> returns NULL and the immediate ->type dereference faults.
>
> Move the bounds check first so dsa_is_user_port()/dsa_is_cpu_port()
> are never reached with an invalid port_num.
While in isolation this makes sense, AFAICT the only possible call
trace here is:
dsa_user_set_rxnfc(...)
{
struct dsa_port *dp = dsa_user_to_port(dev);
struct dsa_switch *ds = dp->ds;
...
return ds->ops->set_rxnfc(ds, dp->index, nfc);
}
int bcm_sf2_set_rxnfc(.. ,int port, ...)
{
...
ret = bcm_sf2_cfp_rule_set(ds, port, &nfc->fs);
...
}
static int bcm_sf2_cfp_rule_set(...)
{
...
ret = bcm_sf2_cfp_rule_cmp(priv, port, fs);
}
Since the port is dp->index from the dsa_port from
dsa_user_to_port(dev), AFAICT it is guaranteed that dsa_to_port(ds,
port) will be non-NULL (else the user port's dsa_port::index would not
the user port's index, which would mean the data got corrupted
somehow).
Also seeing how ds_user_set_rxnfc() is
dsa_user_ethtool_ops::set_rxnfc(), I don't see how port could ever be
anything other than a user port, but maybe I am overlooking some code
path where it can be called for a CPU port.
The check for hw_params.num_ports is sort of valid though, AFAICT sf2
doesn't ensure that the device tree registered ports are within
hw_params::num_ports, though I would also assume it would break at
other places in that scenario. Maybe we should just ensure at
registration time this is the case so we can just drop the check here?
Best regards,
Jonas
> Fixes: 4a5b85ffe2a0 ("net: dsa: use dsa_is_user_port everywhere")
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> Assisted-by: Claude:claude-sonnet-4-6 vscode
> Signed-off-by: Danesh Petigara <danesh.petigara@broadcom.com>
> ---
> drivers/net/dsa/bcm_sf2_cfp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
> index 84a086c3e99b..6ac5cf154f31 100644
> --- a/drivers/net/dsa/bcm_sf2_cfp.c
> +++ b/drivers/net/dsa/bcm_sf2_cfp.c
> @@ -867,9 +867,9 @@ static int bcm_sf2_cfp_rule_insert(struct dsa_switch *ds, int port,
> port_num = ring_cookie / SF2_NUM_EGRESS_QUEUES;
>
> if (ring_cookie == RX_CLS_FLOW_DISC ||
> + port_num >= priv->hw_params.num_ports ||
> !(dsa_is_user_port(ds, port_num) ||
> - dsa_is_cpu_port(ds, port_num)) ||
> - port_num >= priv->hw_params.num_ports)
> + dsa_is_cpu_port(ds, port_num)))
> return -EINVAL;
>
> /* If the rule is matching a particular VLAN, make sure that we honor
> --
> 2.54.0
>
^ permalink raw reply [flat|nested] 2+ messages in thread