From: Visa Hankala <visa@hankala.org>
Subject: Re: sys/cnmac: read next RX buffer pointer before release
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org
Date: Fri, 19 Jun 2026 13:32:36 +0000

On Thu, Jun 18, 2026 at 11:02:36AM +0200, Kirill A. Korinsky wrote:
> tech@, Visa,
> 
> My ER-4 which I use as home's gw crashed some time ago with stacktrace:
> 
> ddb{0}> trace
> cnmac_recv_mbuf+0x13c (77e060040f88a,1,980000000fd979f8,980000000fd979f4)  ra 0
> xffffffff810f13a0 sp 0x980000000fd97990, sz 32
> cnmac_recv+0x88 (77e060040f88a,1,980000000fd979f8,b14f8e2ae1694c1c)  ra 0xfffff
> fff810eddb0 sp 0x980000000fd979b0, sz 144
> cnmac_intr+0x100 (77e060040f88a,593aedd32134a2c6,980000000fd979f8,4)  ra 0xffff
> ffff8124a06c sp 0x980000000fd97a40, sz 112
> octciu_intr_bank+0x274 (77e060040f88a,593aedd32134a2c6,980000000fd979f8,47bf232
> 1b40c9360)  ra 0xffffffff8124976c sp 0x980000000fd97ab0, sz 160
> octciu_intr0+0x94 (77e060040f88a,593aedd32134a2c6,57aacbc77ca5909c,47bf2321b40c
> 9360)  ra 0xffffffff811a0f68 sp 0x980000000fd97b50, sz 64
> interrupt+0x170 (77e060040f88a,1da67593047e9497,57aacbc77ca5909c,47bf2321b40c93
> 60)  ra 0xffffffff813461f4 sp 0x980000000fd97b90, sz 64
> k_intr+0xb4 (980000000fd97bf8,1da67593047e9497,57aacbc77ca5909c,ffffffff811e948
> c)  ra 0x0 sp 0x980000000fd97bd0, sz 0
> (KERNEL INTERRUPT)
> cpu_idle_cycle_wait+0x4 (980000000fd97bf8,1da67593047e9497,57aacbc77ca5909c,fff
> fffff811e948c)  ra 0xffffffff810a7ccc sp 0x980000000fd97d50, sz 0
> sched_idle+0x314 (980000000fd97bf8,1da67593047e9497,57aacbc77ca5909c,ffffffff81
> 1e948c)  ra 0xffffffff811e95bc sp 0x980000000fd97d50, sz 96
> proc_trampoline+0x1c (980000000fd97bf8,1da67593047e9497,57aacbc77ca5909c,ffffff
> ff811e948c)  ra 0x0 sp 0x980000000fd97db0, sz 0
> User-level: pid 7056
> ddb{0}> 
> 
> I not completley sure that had happened, but here some thoughts that can fix
> that, as blind shot, or it can be absolutley wrong.
> 
> The idea that in both RX drop and receive paths, copy the next WQE word
> before the current packet buffer is returned to FPA or its hidden mbuf
> pointer is cleared.
> 
> Returning the buffer first allows hardware to reuse it before the driver
> has consumed the chained buffer pointer; that can corrupt word3 and
> later fault in cnmac_recv_mbuf() while recovering the mbuf from pktbuf.

cnmac_buf_free_work() has a use-after-free. It should happen only when
a jumbo frame gets dropped. (A single hardware packet buffer has
enough space for a non-jumbo-sized frame.)

The crash could in principle happen already in cnmac_buf_free_work()
if the hardware's bookkeeping operations were very quick to clobber
the next word3.

> Index: sys/arch/octeon/dev/if_cnmac.c
> ===================================================================
> RCS file: /home/cvs/src/sys/arch/octeon/dev/if_cnmac.c,v
> diff -u -p -r1.90 if_cnmac.c
> --- sys/arch/octeon/dev/if_cnmac.c	27 Apr 2026 16:54:49 -0000	1.90
> +++ sys/arch/octeon/dev/if_cnmac.c	18 Jun 2026 01:23:19 -0000
> @@ -722,12 +722,12 @@ cnmac_buf_free_work(struct cnmac_softc *
>  		    PIP_WQE_WORD3_BACK_SHIFT;
>  		pktbuf = (addr & ~(CACHELINESIZE - 1)) - back * CACHELINESIZE;
>  
> -		cn30xxfpa_store(pktbuf, OCTEON_POOL_NO_PKT,
> -		    OCTEON_POOL_SIZE_PKT / CACHELINESIZE);
> -
>  		if (nbufs > 0)
>  			memcpy(&word3, (void *)PHYS_TO_XKPHYS(addr -
>  			    sizeof(word3), CCA_CACHED), sizeof(word3));
> +
> +		cn30xxfpa_store(pktbuf, OCTEON_POOL_NO_PKT,
> +		    OCTEON_POOL_SIZE_PKT / CACHELINESIZE);
>  	}
>  
>  	cn30xxfpa_buf_put_paddr(cnmac_fb_wqe, XKPHYS_TO_PHYS(work));

OK visa@ for the above.

> @@ -1244,6 +1244,9 @@ cnmac_recv_mbuf(struct cnmac_softc *sc, 
>  		addr = word3 & PIP_WQE_WORD3_ADDR;
>  		back = (word3 & PIP_WQE_WORD3_BACK) >> PIP_WQE_WORD3_BACK_SHIFT;
>  		pktbuf = (addr & ~(CACHELINESIZE - 1)) - back * CACHELINESIZE;
> +		if (i + 1 < nbufs)
> +			memcpy(&word3, (void *)PHYS_TO_XKPHYS(addr -
> +			    sizeof(word3), CCA_CACHED), sizeof(word3));
>  		pm = (struct mbuf **)PHYS_TO_XKPHYS(pktbuf, CCA_CACHED) - 1;
>  		m = *pm;
>  		*pm = NULL;
> @@ -1276,10 +1279,6 @@ cnmac_recv_mbuf(struct cnmac_softc *sc, 
>  			mprev->m_next = m;
>  		}
>  		mprev = m;
> -
> -		if (i + 1 < nbufs)
> -			memcpy(&word3, (void *)PHYS_TO_XKPHYS(addr -
> -			    sizeof(word3), CCA_CACHED), sizeof(word3));
>  	}
>  
>  	m0->m_pkthdr.len = total;

I think it is clearer to update word3 only after cnmac_recv_mbuf() has
finished with the current segment. Please keep the function as is.

The locations of word3 and pm should never overlap. word3 is inside
the hardware buffer region, whereas pm has to be outside of it.
Otherwise the hardware could clobber pm.