From: "Theo de Raadt" <deraadt@openbsd.org>
Subject: Re: mbuf pulldown replace memmove with memcpy
To: Alexander Bluhm <bluhm@openbsd.org>, tech@openbsd.org
Date: Thu, 29 Aug 2024 09:00:03 -0600

Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:

> On Thu, Aug 29, 2024 at 04:21:01PM +0200, Alexander Bluhm wrote:
> > Hi,
> > 
> > m_pulldown() copies memory between different mbufs.  So they cannot
> > overlap and memcpy() should be enough.
> > 
> > ok?
> 
> Indeed. The first memcpy is save because n is not a read-only buffer and
> so n and n->m_next have do be different mbufs or clusters.
> The 2nd memcpy goes to freshly allocated buffer o. So that is safe too.
> 
> OK claudio@

I agree.

I don't recall the situation backwards copy.  Does the kernel not check
for that, is that only in userland?  I did that so long ago..


> > Index: kern/uipc_mbuf2.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf2.c,v
> > diff -u -p -r1.46 uipc_mbuf2.c
> > --- kern/uipc_mbuf2.c	29 Aug 2024 10:44:40 -0000	1.46
> > +++ kern/uipc_mbuf2.c	29 Aug 2024 14:04:56 -0000
> > @@ -171,7 +171,7 @@ m_pulldown(struct mbuf *m, int off, int 
> >  		n->m_next->m_data -= hlen;
> >  		n->m_next->m_len += hlen;
> >  		counters_inc(mbstat, MBSTAT_PULLDOWN_COPY);
> > -		memmove(mtod(n->m_next, caddr_t), mtod(n, caddr_t) + off, hlen);
> > +		memcpy(mtod(n->m_next, caddr_t), mtod(n, caddr_t) + off, hlen);
> >  		n->m_len -= hlen;
> >  		n = n->m_next;
> >  		off = 0;
> > @@ -201,7 +201,7 @@ m_pulldown(struct mbuf *m, int off, int 
> >  	}
> >  	/* get hlen from <n, off> into <o, 0> */
> >  	o->m_len = hlen;
> > -	memmove(mtod(o, caddr_t), mtod(n, caddr_t) + off, hlen);
> > +	memcpy(mtod(o, caddr_t), mtod(n, caddr_t) + off, hlen);
> >  	n->m_len -= hlen;
> >  	/* get tlen from <n->m_next, 0> into <o, hlen> */
> >  	m_copydata(n->m_next, 0, tlen, mtod(o, caddr_t) + o->m_len);
> > 
> 
> -- 
> :wq Claudio
>