Re: [net-next v24 6/7] net: mtip: Extend the L2 switch driver with management operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:

New issues:
- [High] Global steering register race with shared DMA transmission ring causes packets to be sent to the wrong port.
- [High] Bitwise OR operations on hardware registers without clearing field masks cause invalid modes.
- [High] Disabling multicast reception on the CPU management port breaks IPv6 and essential protocols.
- [High] Off-by-one error in VLAN domain verification check allows out-of-bounds register writes.
- [High] VLAN input processing helper unconditionally overwrites the `ESW_VRES` register, clobbering other ports' memberships.
- [High] Unsynchronized Read-Modify-Write (RMW) on shared hardware registers in management functions.
- [Medium] Per-port learning configuration incorrectly toggles a global interrupt mask and conflicts with NAPI polling.

Pre-existing issues:
- [High] Use-After-Free of SKB after transferring ownership to DMA hardware.
- [High] Double free and UAF of transmit SKBs due to lockless freeing in `mtip_switch_restart`.
- [High] Concurrent PHY adjustments trigger unsynchronized global switch resets.
- [High] Out-of-bounds DMA Mapping of `skb->data`.
--

commit 18b1fc23ec60b0ca391ad794f056d1c911c2b51b
Author: Lukasz Majewski <lukasz.majewski@mailbox.org>

net: mtip: Extend the L2 switch driver with management operations
    
This patch provides function necessary for managing the L2 switch.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> @@ -505,8 +505,35 @@ static void mtip_config_switch(struct switch_enet_private *fep)
>  
>  	writel(0, fep->hwp + ESW_BKLR);

[ ... ]

> +	mtip_port_broadcast_config(fep, 2, 1);
> +
> +	/* Disable multicast receive on port 0 (MGNT) */
> +	mtip_port_multicast_config(fep, 0, 0);

[Severity: High]
Does disabling multicast receive on port 0 break IPv6 communication?

The Linux network stack strictly requires multicast reception to function,
particularly for IPv6 (Neighbor Discovery, Router Advertisements, MLD) and
IPv4 protocols like mDNS. Dropping all multicast traffic in hardware for the
CPU management port might disrupt these protocols.

> +	mtip_port_multicast_config(fep, 1, 1);
> +	mtip_port_multicast_config(fep, 2, 1);
> +
> +	/* Setup VLANs to provide port separation */
> +	if (!fep->br_offload)
> +		mtip_switch_en_port_separation(fep);
>  }

[ ... ]

> @@ -592,6 +619,10 @@ static netdev_tx_t mtip_start_xmit_port(struct sk_buff *skb,
>  
>  	skb_tx_timestamp(skb);

[Severity: High]
This is a pre-existing issue, but does accessing the skb fields here cause a
use-after-free?

Ownership of the SKB is transferred to the hardware by setting
bdp->cbd_sc = status just above this. The hardware can immediately transmit
the packet and issue a completion interrupt which frees the SKB before
skb_tx_timestamp() or skb->len are evaluated.

> +	/* For port separation - force sending via specified port */
> +	if (!fep->br_offload && port != 0)
> +		mtip_forced_forward(fep, port, 1);

[Severity: High]
Is there a race condition here with the shared DMA transmission ring?

The driver steers individual packets to a specific port by updating the global
hardware register ESW_P0FFEN via mtip_forced_forward() immediately before
triggering the DMA transmission. Since the hardware processes the DMA ring
asynchronously, modifying this global register dynamically might forcefully
redirect previously enqueued but not-yet-transmitted packets to the new port.

Also, since forced forwarding is never disabled (a call with enable=0 is
missing), will all subsequent bridge or broadcast traffic originating from the
CPU be misrouted to the last specified port?

>  	dev->stats.tx_bytes += skb->len;

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_mgnt.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_mgnt.c
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_mgnt.c
> @@ -0,0 +1,442 @@

[ ... ]

> +int mtip_vlan_input_process(struct switch_enet_private *fep,
> +			    int port, int mode, unsigned short port_vlanid,
> +			    int vlan_verify_en, int vlan_domain_num,
> +			    int vlan_domain_port)
> +{

[ ... ]

> +	if (vlan_verify_en == 1 &&
> +	    (vlan_domain_num < 0 || vlan_domain_num > 32)) {

[Severity: High]
Does this bounds check permit the index to be exactly 32?

Since the VLAN resolution table consists of 32 entries (indexed 0 to 31),
calculating the offset for index 32 might overwrite the adjacent ESW_DISCN
register and cause memory corruption.

> +		dev_err(&fep->pdev->dev, "%s: Domain out of range\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	writel(FIELD_PREP(MCF_ESW_PID_VLANID_MASK, port_vlanid),
> +	       fep->hwp + ESW_PID(port));
> +	if (port == 0) {
> +		if (vlan_verify_en == 1)
> +			writel(FIELD_PREP(MCF_ESW_VRES_VLANID_MASK,
> +					  port_vlanid) | MCF_ESW_VRES_P0,
> +			       fep->hwp + ESW_VRES(vlan_domain_num));

[Severity: High]
Does writing directly to ESW_VRES(vlan_domain_num) using an absolute writel()
clobber other ports' memberships?

Because ESW_VRES is a bitmap representing all ports in the domain, this might
destroy the membership of any other port previously assigned to that same
VLAN domain.

> +		writel(readl(fep->hwp + ESW_VIMEN) | MCF_ESW_VIMEN_EN0,
> +		       fep->hwp + ESW_VIMEN);
> +		writel(readl(fep->hwp + ESW_VIMSEL) |
> +		       FIELD_PREP(MCF_ESW_VIMSEL_IM0_MASK, mode),
> +		       fep->hwp + ESW_VIMSEL);

[Severity: High]
Does this read-modify-write pattern clear the previous bits in the target
mask before the new mode is applied?

If the register previously held a mode with conflicting bits, the logical OR
will combine them (e.g., updating mode 3 to mode 1 leaves the register at 3),
placing the hardware in an invalid configuration state.

[ ... ]

> +int mtip_vlan_output_process(struct switch_enet_private *fep, int port,
> +			     int mode)
> +{

[ ... ]

> +	if (port == 0) {
> +		writel(readl(fep->hwp + ESW_VOMSEL) |
> +		       FIELD_PREP(MCF_ESW_VOMSEL_OM0_MASK, mode),
> +		       fep->hwp + ESW_VOMSEL);

[Severity: High]
Does this read-modify-write pattern clear the previous bits in the target
mask before the new mode is applied?

If the register previously held a mode with conflicting bits, the logical OR
will combine them, placing the hardware in an invalid configuration state.

[ ... ]

> +int mtip_port_enable_config(struct switch_enet_private *fep, int port,
> +			    bool tx_en, bool rx_en)
> +{
> +	u32 reg = 0;

[ ... ]

> +	reg = readl(fep->hwp + ESW_PER);

[Severity: High]
Are these read-modify-write operations on shared global hardware registers
synchronized?

Since these functions do not acquire fep->hw_lock to serialize access,
concurrent calls from ethtool, sysfs, or NAPI might overlap, tearing the
state and corrupting the hardware registers.

[ ... ]

> +int mtip_port_learning_config(struct switch_enet_private *fep, int port,
> +			      bool disable, bool irq_adj)
> +{
> +	u32 reg = 0;

[ ... ]

> +	reg = readl(fep->hwp + ESW_BKLR);
> +	if (disable) {
> +		if (irq_adj)
> +			writel(readl(fep->hwp + ESW_IMR) & ~MCF_ESW_IMR_LRN,
> +			       fep->hwp + ESW_IMR);

[Severity: Medium]
Does disabling learning on a single port clear MCF_ESW_IMR_LRN in the global
ESW_IMR register?

This might suppress learning interrupts for all other ports. Also, the
pre-existing NAPI poll routine mtip_rx_napi() unconditionally overwrites
ESW_IMR with hardcoded values, meaning this configuration might be
immediately discarded upon the next packet reception.


Additional pre-existing issues found in the codebase:

[Severity: High]
This is a pre-existing issue, but does lockless freeing of SKBs cause a
double free and use-after-free here?

drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c:mtip_switch_restart() {
...
    for (i = 0; i <= TX_RING_MOD_MASK; i++) {
        if (fep->tx_skbuff[i]) {
            dev_kfree_skb_any(fep->tx_skbuff[i]);
            fep->tx_skbuff[i] = NULL;
        }
    }
...
}

The phylib state machine triggers mtip_adjust_link() from process context,
which unconditionally calls mtip_switch_restart() and locklessly frees all
tx_skbuff entries. If the NAPI softirq mtip_switch_tx() is concurrently
processing TX completions, both threads can simultaneously retrieve and free
the same SKB pointers.


[Severity: High]
This is a pre-existing issue, but do concurrent PHY adjustments trigger
unsynchronized global switch resets?

drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c:mtip_adjust_link() {
...
}

The phylib core triggers mtip_adjust_link() for each port's PHY
independently. Because mtip_adjust_link() invokes mtip_switch_restart()
without holding a global lock spanning all ports, simultaneous link state
changes on multiple ports will trigger concurrent MCF_ESW_MODE_SW_RST resets
and overlapping re-initialization of the shared hardware rings.


[Severity: High]
This is a pre-existing issue, but does this mapping read out-of-bounds?

drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c:mtip_start_xmit_port() {
...
    bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev, bufaddr,
                                      MTIP_SWITCH_TX_FRSIZE,
                                      DMA_TO_DEVICE);
...
}

When preparing a transmission descriptor without a bounce buffer,
dma_map_single() is called with a fixed length of MTIP_SWITCH_TX_FRSIZE
instead of the packet's actual length skb->len. Since networking packets are
generally much smaller than the maximum frame size, this causes the DMA
mapping to read well beyond the end of the allocated SKB buffer, potentially
triggering IOMMU faults.

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