From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: veb(4): Fix double m_freem() and refcnt leak
To: Jan Klemkow <jan@openbsd.org>
Cc: tech@openbsd.org
Date: Sat, 2 Aug 2025 06:19:20 +0300

On Fri, Aug 01, 2025 at 09:43:02PM +0200, Jan Klemkow wrote:
> Hi,
> 
> bluhm pointed out a double m_freem() and refcnt leak in veb(4).  The
> following diff fixes the issues.
> 
> ok?
> 
> bye,
> Jan
> 
> Index: net/if_veb.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_veb.c,v
> diff -u -p -r1.41 if_veb.c
> --- net/if_veb.c	7 Jul 2025 02:28:50 -0000	1.41
> +++ net/if_veb.c	1 Aug 2025 19:33:33 -0000
> @@ -1027,8 +1027,10 @@ veb_broadcast(struct veb_softc *sc, stru
>  		if (veb_rule_filter(tp, VEB_RULE_LIST_OUT, m0, src, dst))
>  			continue;
>  
> -		if ((m0 = veb_offload(ifp, ifp0, m0)) == NULL)
> -			goto done;
> +		if ((m0 = veb_offload(ifp, ifp0, m0)) == NULL) {
> +			refcnt_rele_wake(&pm->m_refs);
> +			return;
> +		}
>  
>  		m = m_dup_pkt(m0, max_linkhdr + ETHER_ALIGN, M_NOWAIT);
>  		if (m == NULL) {

The first hunk is right except it does not fix doble m_freem() because
m_freem() accepts NULL. However, `pm' leaks. I would prefer "goto rele"
instead of "rele_wake and return", but feel free to commit as is.

               if ((m0 = veb_offload(ifp, ifp0, m0)) == NULL)
                        goto rele;
               /* ... */
        }
rele:
        refcnt_rele_wake(&pm->m_refs);
done:
        m_freem(m0);

> @@ -1083,7 +1085,7 @@ veb_transmit(struct veb_softc *sc, struc
>  	    m->m_pkthdr.len);
>  
>  	if ((m = veb_offload(ifp, ifp0, m)) == NULL)
> -		goto drop;
> +		return (NULL);
>  
>  	(*tp->p_enqueue)(ifp0, m); /* XXX count error */
>  
>

m_freem() accepts NULL, I see no problems here:

        if ((m = veb_offload(ifp, ifp0, m)) == NULL)
                goto drop;

        (*tp->p_enqueue)(ifp0, m); /* XXX count error */

        return (NULL);
drop:
        m_freem(m);
        return (NULL);
}