From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: mbuf pulldown replace memmove with memcpy
To: tech@openbsd.org
Date: Thu, 29 Aug 2024 17:33:54 +0200

On Thu, Aug 29, 2024 at 09:00:03AM -0600, Theo de Raadt wrote:
> 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..

lib/libkern/memmove.c checks direction, lib/libkern/memcpy.c does
not.  I think we removed the direction check from all memcpy a while
ago.  Argument was, that memcpy caller should do it correctly.

----------------------------
revision 1.3
date: 2013/06/12 16:44:22;  author: deraadt;  state: Exp;  lines: +4 -16;
From now on, the MI libkern memcpy should not do overlap handling.
----------------------------

bluhm

> > > 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
> >