Re: [PATCHv3 1/3] net: fec: do not use readl()/writel() for ColdFire

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Heap buffer overflow in TX bounce buffer due to unvalidated packet and fragment lengths.
- [High] The hardware PPS interrupt handler accesses and modifies shared PTP state without acquiring the required lock, racing with configuration paths on other CPUs.
- [High] Unbounded hardware polling loops can cause a hard lockup if the hardware enters an error state.
- [High] Integer underflow in RX packet length calculation leads to out-of-bounds memory access.
- [High] Out-of-bounds read and write in `fec_enet_clear_csum` due to missing `pskb_may_pull`.
- [High] Missing dma_rmb() after reading descriptor status in RX paths.
--

commit b19b9a07ca8eff68179fb339b69d5c4690fdf442
Author: Greg Ungerer <gerg@linux-m68k.org>
net: fec: do not use readl()/writel() for ColdFire

This patch modifies the FEC driver to use local fec_readl() and fec_writel() 
methods instead of direct hardware register accesses. This avoids incorrect
byte swapping on big-endian architectures like ColdFire.

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
[ ... ]
> @@ -1923,7 +1924,7 @@ static int fec_enet_rx_queue(struct fec_enet_private *fep,
>  			break;
>  		pkt_received++;
>  
> -		writel(FEC_ENET_RXF_GET(queue), fep->hwp + FEC_IEVENT);
> +		fec_writel(FEC_ENET_RXF_GET(queue), fep->hwp + FEC_IEVENT);

[Severity: High]
This is a pre-existing issue, but is a memory barrier missing in the RX
loop?

The loop checks the status to confirm ownership in the while condition, but
without a dma_rmb() immediately after, couldn't speculative execution on
architectures with weak memory ordering cause the subsequent read of
cbd_datlen to happen before the DMA transfer is completely visible?

>  
>  		/* Check for errors. */
>  		status ^= BD_ENET_RX_LAST;

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
[ ... ]
> @@ -142,17 +142,17 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
>  	if (enable) {
>  		/* clear capture or output compare interrupt status if have.
>  		 */
> -		writel(FEC_T_TF_MASK, fep->hwp + FEC_TCSR(fep->pps_channel));
> +		fec_writel(FEC_T_TF_MASK, fep->hwp + FEC_TCSR(fep->pps_channel));
>  
>  		/* It is recommended to double check the TMODE field in the
>  		 * TCSR register to be cleared before the first compare counter
>  		 * is written into TCCR register. Just add a double check.
>  		 */
> -		val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
> +		val = fec_readl(fep->hwp + FEC_TCSR(fep->pps_channel));
>  		do {
>  			val &= ~(FEC_T_TMODE_MASK);
> -			writel(val, fep->hwp + FEC_TCSR(fep->pps_channel));
> -			val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
> +			fec_writel(val, fep->hwp + FEC_TCSR(fep->pps_channel));
> +			val = fec_readl(fep->hwp + FEC_TCSR(fep->pps_channel));
>  		} while (val & FEC_T_TMODE_MASK);

[Severity: High]
This is a pre-existing issue, but is it safe to have an unbounded polling
loop here while IRQs are disabled?

If the hardware transitions into a faulty state and perpetually returns a
value with FEC_T_TMODE_MASK set, could this loop cause a hard lockup or
kernel panic?

>  
>  		/* Dummy read counter to update the counter */
[ ... ]
> @@ -194,31 +194,31 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
>  		 * is bigger than fep->cc.mask would be a error.
>  		 */
>  		val &= fep->cc.mask;
> -		writel(val, fep->hwp + FEC_TCCR(fep->pps_channel));
> +		fec_writel(val, fep->hwp + FEC_TCCR(fep->pps_channel));
>  
>  		/* Calculate the second the compare event timestamp */
>  		fep->next_counter = (val + fep->reload_period) & fep->cc.mask;

[Severity: High]
This is a pre-existing issue, but I noticed a potential data race between
this configuration path and the interrupt handler.

Here, fep->next_counter is updated while holding fep->tmreg_lock. However,
the interrupt handler fec_pps_interrupt() modifies fep->next_counter
locklessly. Could this cause the PTP timer to miss events or corrupt the
hardware state if this is triggered concurrently from userspace?

>  
>  		/* * Enable compare event when overflow */
> -		val = readl(fep->hwp + FEC_ATIME_CTRL);
[ ... ]
> @@ -701,15 +701,15 @@ static irqreturn_t fec_pps_interrupt(int irq, void *dev_id)
>  	u8 channel = fep->pps_channel;
>  	struct ptp_clock_event event;
>  
> -	val = readl(fep->hwp + FEC_TCSR(channel));
> +	val = fec_readl(fep->hwp + FEC_TCSR(channel));
>  	if (val & FEC_T_TF_MASK) {
>  		/* Write the next next compare(not the next according the spec)
>  		 * value to the register
>  		 */
> -		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
> +		fec_writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>  		do {
> -			writel(val, fep->hwp + FEC_TCSR(channel));
> -		} while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
> +			fec_writel(val, fep->hwp + FEC_TCSR(channel));
> +		} while (fec_readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);

[Severity: High]
This is a pre-existing issue, but similar to the loop in
fec_ptp_enable_pps(), does this unbounded loop risk a hard lockup?

Since this runs in hardirq context, a faulty hardware state that fails to
clear FEC_T_TF_MASK would keep the CPU spinning indefinitely.

[ ... ]

I also noticed a few other pre-existing issues in this driver that fall
outside the immediate diff context of this patch:

[Severity: Critical]
This is a pre-existing issue, but does fec_enet_txq_submit_frag_skb()
overflow the TX bounce buffer?

index = fec_enet_get_bd_index(bdp, &txq->bd);
if (((unsigned long) bufaddr) & fep->tx_align ||
    fep->quirks & FEC_QUIRK_SWAP_FRAME) {
    memcpy(txq->tx_bounce[index], bufaddr, frag_len);
    bufaddr = txq->tx_bounce[index];

Since fragments can be up to PAGE_SIZE (4KB or 64KB), but the tx_bounce
buffers are statically sized to 2048 bytes, couldn't a large unaligned
packet or jumbo frame cause an out-of-bounds heap write?

[Severity: High]
This is a pre-existing issue, but I have a question about the packet length
calculation in fec_enet_rx_queue_xdp() and fec_enet_rx_queue():

pkt_len = fec16_to_cpu(bdp->cbd_datlen);
...
pkt_len -= sub_len;
xdp_prepare_buff(&xdp, page_address(page), data_start, pkt_len, false);

If a corrupted or maliciously crafted DMA descriptor reports a pkt_len less
than sub_len, pkt_len will underflow. Won't this pass a very large length
to xdp_prepare_buff() (or skb_put() in fec_build_skb()), potentially
allowing a BPF program to read/write out of bounds or causing an
skb_over_panic()?

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds
access on non-linear SKBs in fec_enet_clear_csum():

static bool is_ipv4_pkt(struct sk_buff *skb)
{
    return skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->version == 4;
}

If an AF_PACKET socket transmits a packet where the IP header is in a
non-linear fragment, wouldn't ip_hdr(skb)->version and the subsequent
ip_hdr(skb)->check = 0 assignment read and write past the linear buffer
into unallocated memory, since pskb_may_pull() hasn't been called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907134037.1855408-2-gerg@linux-m68k.org?part=1