Re: [PATCH net] net: ethernet: ti: am65-cpsw-nuss: Fix port_id extraction from SRC TAG

On 10/08/26 22:26, Simon Horman wrote:
> 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://netdev-ai.bots.linux.dev/sashiko/#/patchset/[email protected]?part=1
> ---
> --- Patch [1]: [PATCH net] net: ethernet: ti: am65-cpsw-nuss: Fix port_id extraction from SRC TAG ---
> commit b0ed959d350c90d8b49f6f81eba7461fbc86dcf4
> Author: Siddharth Vadapalli <[email protected]>
> 
> net: ethernet: ti: am65-cpsw-nuss: Fix port_id extraction from SRC TAG
> 
> The RX path extracts the ingress MAC port ID from the CPPI5 descriptor's
> 16-bit Source Tag. Only the lower 8 bits carry the port ID, so the patch
> masks the value with 0xFF before it is used as an index into
> common->ports[].
> 
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 434a31080855..6330540979d7 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -1310,6 +1310,8 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_rx_flow *flow,
>>   	k3_udma_glue_rx_cppi5_to_dma_addr(rx_chn->rx_chn, &buf_dma);
>>   	pkt_len = cppi5_hdesc_get_pktlen(desc_rx);
>>   	cppi5_desc_get_tags_ids(&desc_rx->hdr, &port_id, NULL);
>> +	/* Port ID is contained in the lower 8-bits of the 16-bit Source Tag */
>> +	port_id &= 0xFF;
>>   	dev_dbg(dev, "%s rx port_id:%d\n", __func__, port_id);
>>   	port = am65_common_get_port(common, port_id);
>>   	ndev = port->ndev;
> 
> [Severity: High]
> [Finding: main-1-0]
> [Sources: opus-5]
> 
> The missing range check on port_id is pre-existing and not introduced by
> this patch, but since the change is specifically about avoiding the
> out-of-bounds access, would it be worth validating the value instead of
> relying only on the mask?

Hardware guarantees a valid Port ID in the lower 8-bits. A Software check 
is an unnecessary overhead and will impact performance since this is on the 
data-path and is not a one-time check during initialization.

> 
> After the mask, port_id is still an unvalidated hardware-supplied value in
> the range 0..255, and am65_common_get_port() does raw pointer arithmetic
> with no bounds check:
> 
> drivers/net/ethernet/ti/am65-cpsw-nuss.h:
>      #define am65_common_get_port(common, id) (&(common)->ports[(id) - 1])
> 
> common->ports is a devm_kcalloc() array of exactly common->port_num
> entries (1..AM65_CPSW_MAX_PORTS, checked in am65_cpsw_nuss_probe()), which
> is typically 1 or 2 on shipping parts. So port_id == 0 gives ports[-1] and

port_id will NOT be zero since port ID is a Hardware provided identifier of 
the Physical MAC Ports starting from '1'.

> any port_id above common->port_num reads past the end of the allocation.
> The resulting pointer is dereferenced right away in
> am65_cpsw_nuss_rx_packets():
> 
>      port = am65_common_get_port(common, port_id);
>      ndev = port->ndev;
>      ...
>      if (port->xdp_prog) {
> 
> and ndev then flows into am65_cpsw_build_skb(), netdev_priv(),
> netif_dormant() and ndev->stats.*. Can a stray low byte still produce the
> same class of crash the commit message describes?

Since Hardware guarantees that the lower 8-bits will always be the Port ID, 
the way Software expects it to be, additional checks aren't required.

> 
> The driver already enforces the required index domain for this accessor on
> the device tree path in am65_cpsw_nuss_init_slave_ports():
> 
>      if (!port_id || port_id > common->port_num) {
>          dev_err(dev, "%pOF has invalid port_id %u %s\n",
>              port_np, port_id, port_np->name);
>          ret = -EINVAL;
>          goto of_node_put;
>      }
> 
>      port = am65_common_get_port(common, port_id);
> 
> Would the same !port_id || port_id > common->port_num test in the RX path,
> dropping the packet and requeueing the page (bumping rx_errors, perhaps
> with a WARN_ON_ONCE), be a more robust fix than the mask alone?

Unlike am65_cpsw_init_slave_ports() function which is executed only once 
during initialization, am65_cpsw_nuss_rx_packets is executed for every 
packet that is received from the wire. More importantly, the sanity check 
for port_id in am65_cpsw_nuss_init_slave_ports() is performed on the 
device-tree provided MAC Port ID rather than a Hardware provided MAC Port 
ID. The comparison therefore isn't apt.

> 
> There is a second case even when the index is in range: ports disabled in
> the device tree never get an ndev allocated, and every other iteration site
> guards for that, for example in am65_cpsw_nuss_set_offload_fwd_mark():
> 
>      if (!port->ndev)
>          continue;
> 
> The RX path uses port->ndev unconditionally, so an in-range port_id
> belonging to a disabled port would hand a NULL net_device to
> am65_cpsw_build_skb() and netdev_priv(). Should that case be rejected too?

All MAC Ports that are disabled in the device-tree are kept disabled by the 
driver, so it is not possible to end up in a situation where the MAC Port 
ID in the RX DMA Descriptor contains a disabled MAC Port.

> 
> As a small nit, would a named mask defined alongside the other CPPI5/AM65
> definitions read better here than the bare 0xFF literal?

I don't see a real need for a macro given that a comment has been added 
just above, and there aren't other places where this is used at the moment. 
However, if reviewers believe that it should be replaced by a macro, I am 
open to doing so.

Regards,
Siddharth.