Re: [PATCH net-next 2/2] net: econet: add EcoNet EN751221 ethernet driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 22 potential issue(s) to consider:
- [High] Use-after-free when `ndo_start_xmit` returns `NETDEV_TX_BUSY` after having already freed the SKB.
- [High] Potential spinlock deadlock due to missing `_bh` protection on hardware stats synchronization.
- [High] Silent SKB memory leak when DMA mapping fails due to incorrect error code propagation.
- [High] Copy-paste error assigns port 0 hardware registers to port 1, breaking the WAN port.
- [High] Dereferencing a potentially NULL port pointer in the RX path.
- [High] Hardware interrupts for TX chain 1 are permanently masked after firing once.
- [High] RX SKBs are built with zero headroom, causing panics during header pushes.
- [High] Sleeping function called while holding a spinlock in en75_dev_open.
- [High] Unchecked return value from of_reserved_mem_lookup causes NULL pointer dereference.
- [High] Hardware Forwarding Engine (HWF) is never stopped during teardown.
- [High] Unmasked hardware interrupts cause UAF against destroyed NAPI structs.
- [High] Calling netif_napi_del on uninitialized structs causes a panic.
- [High] virt_to_head_page(NULL) causes a panic during RX teardown.
- [High] napi_enable is called after hardware DMA is enabled, risking permanent queue stalls.
- [High] ERR_PTR dereference in debugfs teardown.
- [High] Configured MTU exceeds the size of the buffer provided to the RX DMA, causing packet corruption or drops.
- [High] Using physical memory addresses directly as DMA addresses breaks on systems with IOMMUs or DMA offsets.
- [Medium] Hardware statistics are cleared after reading, destroying concurrent increments (TOCTOU race).
- [Medium] Shared IRQ handler unconditionally returns IRQ_HANDLED.
- [Medium] Software TX drop statistics are silently overwritten and lost.
- [Medium] Driver ignores -EPROBE_DEFER for reset controls.
- [Medium] A `WARN_ON_ONCE` triggers spuriously during ring teardown if a previous out-of-memory condition occurred.
--

commit 98f885a3a05d744a2bc4c54348f8262c61f73dbd
Author: Caleb James DeLisle <cjd@cjdns.fr>
Date:   Wed Sep 9 21:21:46 2026 +0000

    net: econet: add EcoNet EN751221 ethernet driver
    
    The EcoNet ethernet driver is somewhat similar to both the Airoha
    and the MediaTek drivers, but still unique enough that attempts at
    code reuse did not prove fruitful.

> diff --git a/drivers/net/ethernet/econet/econet_eth.c b/drivers/net/ethernet/econet/econet_eth.c
> --- a/drivers/net/ethernet/econet/econet_eth.c
> +++ b/drivers/net/ethernet/econet/econet_eth.c
[ ... ]
> @@ -109,8 +109,8 @@ int en75_rx_before_recv(struct en75_eth *eth, struct sk_buff *skb,
>  {
>  	struct en75_eth_pvt *ep = (struct en75_eth_pvt *)eth;
>  	struct net_device *port;
>  
>  	port = en75_get_sport_dev(ep, sport);
>  
>  	skb->dev = port;
>  	skb->protocol = eth_type_trans(skb, port);

[Severity: High]
Can port be NULL here? If en75_get_sport_dev() returns an uninitialized
port from the eth->ports[] array, would this result in a NULL pointer
dereference when port is passed to eth_type_trans()?

[ ... ]
> @@ -153,8 +153,8 @@ static int en75_init_port(struct en75_eth_pvt *eth, struct device_node *np)
>  	if (id == 1)
>  		dev = en75_alloc_gdm_port(&eth->pub, np,
>  					  &eth->regs->port0.regs,
>  					  eth->qdma[0],
>  					  ETX_FPORT_GDM1,
>  					  false);
>  	else if (id == 2)
>  		dev = en75_alloc_gdm_port(&eth->pub, np,
>  					  &eth->regs->port0.regs,
>  					  eth->qdma[1],
>  					  ETX_FPORT_GDM2,
>  					  true);

[Severity: High]
Is it intentional to pass &eth->regs->port0.regs for the second port
(id == 2) instead of &eth->regs->port1.regs? This appears to assign the same
hardware registers to both the LAN and WAN ports, which could break the
WAN port functionality.

[ ... ]
> @@ -218,5 +218,5 @@ static int en75_probe(struct platform_device *pdev)
>  	eth->reset = devm_reset_control_array_get_exclusive(&pdev->dev);
>  	if (IS_ERR(eth->reset)) {
>  		dev_warn(&pdev->dev, "failed to get resets %pe\n", eth->reset);
>  	} else {

[Severity: Medium]
If devm_reset_control_array_get_exclusive() returns -EPROBE_DEFER, does it
make sense to print a warning and continue the probe sequence? Should the
error be propagated so the driver can properly defer its probe?

[ ... ]
> @@ -269,3 +269,3 @@ static int en75_probe(struct platform_device *pdev)
>  	eth->debug = en75_debugfs_init(&debug_conf);
>  
>  	return 0;

[Severity: High]
en75_debugfs_init() can return ERR_PTR(-ENOMEM) on failure. Does
eth->debug need to be checked with IS_ERR() to prevent storing an error
pointer in the device structure?

> diff --git a/drivers/net/ethernet/econet/econet_eth_debug.c b/drivers/net/ethernet/econet/econet_eth_debug.c
> --- a/drivers/net/ethernet/econet/econet_eth_debug.c
> +++ b/drivers/net/ethernet/econet/econet_eth_debug.c
[ ... ]
> @@ -612,4 +612,4 @@ void en75_debugfs_exit(struct en75_debug *debug)
>  {
>  	if (!debug)
>  		return;
>  	debugfs_remove_recursive(debug->dir);
>  	kfree(debug);
>  }

[Severity: High]
If debug contains an ERR_PTR from the failed initialization mentioned
above, would calling debugfs_remove_recursive(debug->dir) cause a kernel
panic due to an invalid pointer dereference?

> diff --git a/drivers/net/ethernet/econet/econet_port.c b/drivers/net/ethernet/econet/econet_port.c
> --- a/drivers/net/ethernet/econet/econet_port.c
> +++ b/drivers/net/ethernet/econet/econet_port.c
[ ... ]
> @@ -139,4 +139,4 @@ static int en75_dev_open(struct net_device *dev)
>  	guard(spinlock)(&port->reg_lock);
>  	rlt = en75_rreg(&port->regs->rx_len_threshold);
>  	set_gdm_len_th_runt_len(&rlt, 60);
>  	set_gdm_len_th_oversize_len(&rlt, ETH_HLEN + dev->mtu + ETH_FCS_LEN);
>  	en75_wreg(rlt, &port->regs->rx_len_threshold);
>  
>  	return en75_qdma_use(port->qdma);
>  }

[Severity: High]
Does en75_dev_open() hold port->reg_lock (via guard(spinlock)) while
calling en75_qdma_use()? Since en75_qdma_use() acquires a mutex, could
this lead to a sleep-in-atomic deadlock?

[ ... ]
> @@ -178,4 +178,4 @@ static netdev_tx_t en75_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  	ret = en75_qdma_xmit(port->qdma, skb, &msg, 0);
>  
>  	if (ret < 0) {
>  		netdev_tx_completed_queue(txq, 1, len);
>  		goto error;
>  	}

[Severity: High]
If en75_qdma_xmit() propagates the positive return value (1) from
dma_mapping_error(), would this ret < 0 check bypass the error handling,
returning NETDEV_TX_OK and silently leaking the SKB?

[ ... ]
> @@ -189,4 +189,4 @@ error:
>  	dev_kfree_skb_any(skb);
>  	dev->stats.tx_dropped++;
>  
>  	if (ret == -EBUSY) {
>  		netif_tx_stop_queue(txq);
>  		return NETDEV_TX_BUSY;
>  	}

[Severity: High]
If ret is -EBUSY, does this error path free the SKB before returning
NETDEV_TX_BUSY? Would the networking stack then attempt to re-queue and
re-transmit the already freed SKB, leading to a use-after-free?

[ ... ]
> @@ -199,4 +199,4 @@ static void en75_update_hw_stats(struct en75_gdm_port *port)
>  	struct clear_counters cc = {0};
>  	u32 i = 0;
>  
>  	guard(spinlock)(&port->stats.lock);
>  	u64_stats_update_begin(&port->stats.syncp);

[Severity: High]
Does the use of guard(spinlock) in en75_update_hw_stats() lack softirq
protection? If ndo_get_stats64 executes in softirq context while this lock
is held by process context on the same CPU, could it cause a deadlock? Would
guard(spinlock_bh) be more appropriate?

[ ... ]
> @@ -269,4 +269,4 @@ static void en75_update_hw_stats(struct en75_gdm_port *port)
>  	set_gdm_cl_cnt_rx(&cc, true);
>  	set_gdm_cl_cnt_tx(&cc, true);
>  	en75_wreg(cc, &port->regs->clear_counters);
>  
>  	u64_stats_update_end(&port->stats.syncp);
>  }

[Severity: Medium]
By explicitly clearing the hardware counters with clear_counters, is there a
race condition? Could any packets processed by the hardware between the MMIO
reads and the clear command be permanently lost from the statistics?

[ ... ]
> @@ -285,4 +285,4 @@ static void en75_dev_get_stats64(struct net_device *dev,
>  		storage->rx_errors = port->stats.rx_errors;
>  		storage->rx_dropped = port->stats.rx_drops;
>  		storage->tx_dropped = port->stats.tx_drops;
>  		storage->rx_over_errors = port->stats.rx_over_errors;

[Severity: Medium]
Does overwriting storage->tx_dropped with the hardware drops unconditionally
discard any software-initiated drops (like DMA mapping failures) that were
recorded in dev->stats.tx_dropped?

[ ... ]
> @@ -401,3 +401,3 @@ struct net_device *en75_alloc_gdm_port(struct en75_eth *eth,
>  	ndev->ethtool_ops = &en75_ethtool_ops;
>  	ndev->max_mtu = EN75_MAX_PACKET_SIZE;

[Severity: High]
If max_mtu is set to EN75_MAX_PACKET_SIZE (2048), would an incoming
2048-byte payload exceed the available buffer space since
en75_fill_rx_queue() in econet_qdma.c allocates only
SKB_WITH_OVERHEAD(2048) for the DMA? Could this cause hardware truncation or
overflow?

> diff --git a/drivers/net/ethernet/econet/econet_qdma.c b/drivers/net/ethernet/econet/econet_qdma.c
> --- a/drivers/net/ethernet/econet/econet_qdma.c
> +++ b/drivers/net/ethernet/econet/econet_qdma.c
[ ... ]
> @@ -188,4 +188,4 @@ static void en75_qdma_rx_process_one(struct en75_q_rx *q, u32 cpu_i,
>  	skb = napi_build_skb(e->buf, q->buf_size);
>  	if (!skb)
>  		goto return_page;
>  
>  	__skb_put(skb, len);

[Severity: High]
Does napi_build_skb create an SKB with zero headroom? Since the code pushes
the payload using __skb_put without reserving any headroom via
skb_reserve(), would any later header insertion in the networking stack
cause an skb_under_panic?

[ ... ]
> @@ -353,4 +353,4 @@ static irqreturn_t en75_irq_handler(int irq_num, void *dev_instance)
>  		irq->irqmask[i] &= ~disable_int;
>  		en75_wreg(irq->irqmask[i], irq->mask_reg[i]);
>  	}
>  
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
Since this interrupt is registered with IRQF_SHARED, does unconditionally
returning IRQ_HANDLED (even if no events were processed) break the kernel's
spurious interrupt detection?

[ ... ]
> @@ -418,4 +418,4 @@ static int en75_poll_tx_complete(struct napi_struct *napi, int budget)
>  	if (done < budget && napi_complete(napi)) {
>  		union en75_irq_purpose purpose = IRQ_PURPOSE(DONE, TX, id);
>  		union irq_bit b = en751221_irq_bit(purpose);
>  
>  		en75_qdma_set_irqmask(qdma, b, true);
>  	}

[Severity: High]
Since id is derived from the base of the q_tx_done array, and
QDMA_NUM_TX_DONE is 1, id will always be 0. Doesn't this mean that
en75_poll_tx_complete will always re-enable the interrupt for TX chain 0?
Could this leave the interrupt for TX chain 1 permanently masked after it
fires once?

[ ... ]
> @@ -401,3 +401,3 @@ int en75_qdma_xmit(struct en75_qdma *qdma, struct sk_buff *skb,
>  	addr = dma_map_single(qdma->dev, skb->data, len, DMA_TO_DEVICE);
>  	ret = dma_mapping_error(qdma->dev, addr);
>  	if (unlikely(ret))
>  		return ret;

[Severity: High]
dma_mapping_error() returns a positive boolean value on failure. Since this
function propagates the return value directly, does this cause the caller
en75_dev_xmit() to miss the error check?

[ ... ]
> @@ -643,5 +643,5 @@ static int en75_init_hw_fwd(struct en75_qdma *qdma)
>  		rmem = of_reserved_mem_lookup(np);
>  		of_node_put(np);
>  		dma_addr = rmem->base;
>  		/* Compute the number of hw descriptors according to the
>  		 * reserved memory size and the payload buffer size
>  		 */

[Severity: High]
If of_reserved_mem_lookup(np) returns NULL, does rmem->base cause a NULL
pointer dereference panic?

[Severity: High]
rmem->base is a physical address (phys_addr_t), but it is being assigned
to dma_addr and written to the hardware. Would this break DMA operations on
systems where the physical address is not the same as the DMA address (e.g.,
due to an IOMMU or DMA offset)?

[ ... ]
> @@ -666,4 +666,4 @@ static int en75_init_hw_fwd(struct en75_qdma *qdma)
>  	set_qregs_hwf_cfg1_start(&cfg1, true);
>  	en75_wreg(cfg1, &qdma->regs->hwf_cfg1);
>  
>  	ret = read_poll_timeout(en75_rreg, cfg1,

[Severity: High]
The hardware forwarding engine is explicitly started here, but it doesn't seem
to be stopped during driver teardown in en75_qdma_destroy_locked(). Could
the hardware continue to write to freed DMA memory after the driver is
unloaded, causing memory corruption?

[ ... ]
> @@ -714,5 +714,5 @@ static void en75_qdma_destroy_rxq_locked(struct en75_q_rx *q)
>  	for (i = 0; i < q->ndesc; i++) {
>  		struct en75_q_rx_ent *e = &q->entry[i];
>  		struct page *page = virt_to_head_page(e->buf);
>  
>  		if (WARN_ON_ONCE(!e->dma_addr))
>  			continue;

[Severity: High]
If an initialization failure caused some descriptors to remain uninitialized
(where e->buf is NULL), would evaluating virt_to_head_page(e->buf)
before the !e->dma_addr safety check result in a kernel panic?

[Severity: Medium]
If an allocation failure does occur during ring initialization, leaving
remaining descriptors with e->dma_addr == 0, wouldn't this WARN_ON_ONCE
trigger spuriously during the subsequent cleanup, creating noisy logs for a
safely-handled out-of-memory condition?

[ ... ]
> @@ -732,4 +732,4 @@ static int en75_qdma_destroy_locked(struct en75_qdma *qdma)
>  	for (i = 0; i < ARRAY_SIZE(qdma->q_rx); i++) {
>  		struct en75_q_rx *q = &qdma->q_rx[i];
>  
>  		en75_qdma_destroy_rxq_locked(q);
>  		netif_napi_del(&q->napi);
>  	}

[Severity: High]
If en75_init_rx_queue fails before calling netif_napi_add, does calling
netif_napi_del(&q->napi) on an uninitialized NAPI struct cause a panic due
to operating on zeroed list pointers?

[Severity: High]
Hardware interrupts do not appear to be globally masked during this teardown
sequence. If a late interrupt fires while the NAPI structures are being
deleted, could the active IRQ handler attempt to schedule the destroyed NAPI
struct, leading to a use-after-free and softnet corruption?

[ ... ]
> @@ -769,4 +769,4 @@ int en75_qdma_use(struct en75_qdma *qdma)
>  	en75_wreg(qcfg, &qdma->regs->qdma_cfg);
>  
>  	for (i = 0; i < ARRAY_SIZE(qdma->q_rx); i++)
>  		napi_enable(&qdma->q_rx[i].napi);

[Severity: High]
Is there a risk of permanently stalling the queues by enabling the hardware
DMA bits before enabling the software NAPI pollers? If an interrupt fires
between the DMA enable and napi_enable, it seems the NAPI scheduling would
be ignored and the interrupt left masked indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909212146.3184077-1-cjd@cjdns.fr?part=2