Re: [net-next v24 4/7] net: mtip: Add net_device_ops functions to the L2 switch driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Unchecked memcpy into a fixed-size bounce buffer causes a heap buffer overflow if the SKB exceeds the buffer size.
- [High] Accessing an SKB after handing it to the hardware transmission ring causes a use-after-free.
- [High] The watchdog timeout work logic evaluates to true when the device is closed, leading to hardware access crashes.
- [High] Missing hardware lock during `mtip_switch_restart()` causes a data race with concurrent transmissions.
- [High] Mapping a fixed maximum size instead of the actual SKB length causes DMA out-of-bounds reads.
- [High] Incorrect teardown ordering in `mtip_ndev_cleanup` causes a Use-After-Free race with the TX timeout work.
- [Medium] The TX path maps buffers for DMA but does not unmap them during a timeout restart, causing a DMA mapping leak.
--

commit 472dd23af31f2a6b37da050f58cdf8ed66693ebc
Author: Lukasz Majewski <lukasz.majewski@mailbox.org>

net: mtip: Add net_device_ops functions to the L2 switch driver

This patch provides callbacks for struct net_device_ops for MTIP
L2 switch.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> index 5b5229ee411df..7dfeda230e3fb 100644
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
[ ... ]
> @@ -463,6 +472,120 @@ static void mtip_config_switch(struct switch_enet_private *fep)
[ ... ]
> +	/* On some FEC implementations data must be aligned on
> +	 * 4-byte boundaries. Use bounce buffers to copy data
> +	 * and get it aligned.
> +	 */
> +	if ((unsigned long)bufaddr & MTIP_ALIGNMENT ||
> +	    fep->quirks & FEC_QUIRK_SWAP_FRAME) {
> +		memcpy(fep->tx_bounce[index], skb->data, skb->len);

[Severity: Critical]
Does this code overflow the bounce buffer? The bounce buffer has a fixed size,
but there appears to be no bounds check to ensure the skb length actually
fits. A maliciously constructed oversized packet could cause a heap buffer
overflow.

> +		bufaddr = fep->tx_bounce[index];
> +
> +		if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
> +			swap_buffer(bufaddr, skb->len);
> +	}
> +
> +	/* Push the data cache so the CPM does not get stale memory
> +	 * data.
> +	 */
> +	bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev, bufaddr,
> +					  MTIP_SWITCH_TX_FRSIZE,
> +					  DMA_TO_DEVICE);

[Severity: High]
Is it safe to map a fixed maximum size instead of the actual packet length?
Since the underlying memory allocated for the packet may be smaller than
MTIP_SWITCH_TX_FRSIZE, this can force the DMA API to flush memory beyond the
bounds of the allocation, potentially triggering an out-of-bounds read or
IOMMU faults.

> +	if (unlikely(dma_mapping_error(&fep->pdev->dev, bdp->cbd_bufaddr))) {
[ ... ]
> +	/* Send it on its way.  Tell FEC it's ready, interrupt when done,
> +	 * it's the last BD of the frame, and to put the CRC on the end.
> +	 */
> +
> +	status |= (BD_ENET_TX_READY | BD_ENET_TX_INTR | BD_ENET_TX_LAST |
> +		   BD_ENET_TX_TC);
> +
> +	/* Synchronize all descriptor writes */
> +	wmb();
> +	bdp->cbd_sc = status;
> +
> +	skb_tx_timestamp(skb);
> +
> +	dev->stats.tx_bytes += skb->len;

[Severity: High]
Can this sequence cause a use-after-free? Once the descriptor status is
updated and the memory barrier is issued, ownership of the buffer is
transferred to the hardware. The transmission completion interrupt could free
the packet before the timestamp and length are accessed here.

[ ... ]
> +static void mtip_timeout_work(struct work_struct *work)
> +{
> +	struct mtip_ndev_priv *priv =
> +		container_of(work, struct mtip_ndev_priv, tx_timeout_work);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct net_device *dev = priv->dev;
> +
> +	rtnl_lock();
> +	if (netif_device_present(dev) || netif_running(dev)) {

[Severity: High]
Is this logical condition correct? The netif_device_present check evaluates to
true as long as the device is registered, even if it is administratively down.
Writing to the hardware registers via mtip_switch_restart while the device is
closed and the IPG clock is disabled could trigger an imprecise external abort
and crash the system.

> +		napi_disable(&fep->napi);
> +		netif_tx_lock_bh(dev);
> +		mtip_switch_restart(dev, fep->full_duplex[0],
> +				    fep->full_duplex[1]);

[Severity: High]
Does this restart logic cause a data race with concurrent transmissions on the
other port? The driver manages two network ports sharing a single hardware
ring. mtip_switch_restart resets the shared transmission ring state, but only
the tx lock for the specific port that timed out is acquired here, rather than
the shared hardware lock.

[Severity: Medium]
Does mtip_switch_restart leak DMA mappings during a timeout recovery? The
transmission path maps buffers for DMA, but the restart routine appears to
simply free the transmit socket buffers without calling the corresponding unmap
function, potentially leaking IOVA space or bounce buffers on every timeout.

> +		netif_tx_wake_all_queues(dev);
> +		netif_tx_unlock_bh(dev);
> +		napi_enable(&fep->napi);
> +	}
> +	rtnl_unlock();
> +}
[ ... ]
> @@ -1158,10 +1448,14 @@ static int mtip_switch_dma_init(struct switch_enet_private *fep)
>  
>  static void mtip_ndev_cleanup(struct switch_enet_private *fep)
>  {
> +	struct mtip_ndev_priv *priv;
>  	int i;
>  
>  	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
>  		if (fep->ndev[i]) {
> +			priv = netdev_priv(fep->ndev[i]);
> +			cancel_work_sync(&priv->tx_timeout_work);
> +
>  			unregister_netdev(fep->ndev[i]);

[Severity: High]
Can this result in a use-after-free race with the timeout work? Calling
unregister_netdev after cancel_work_sync means the networking watchdog is
still active and could fire immediately after cancel_work_sync returns. If the
watchdog queues the work item again, the work could execute after the device
structures have been freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601112437.2216043-1-lukasz.majewski@mailbox.org?part=4