On 15 December 2017 at 14:24, Robert Elz wrote:
>
> Date:Fri, 15 Dec 2017 05:01:17 +
> From:"Kengo NAKAHARA"
> Message-ID: <20171215050117.241baf...@cvs.netbsd.org>
>
> | Fix pullup'ed mbuf leaks.
> | The match function just requires enough mbuf length.
>
> This is still not correct. There are no mbuf leaks, I think you did
> not understand maxv's point (if I remember it correctly).
>
> The problem isn't lost mbufs, it is that if code does a m_pullup()
> (or m_ensure_contig() which is essentially the same thing) the
> mbuf chain can be altered.
>
> What that means is that after a call of in_l2tp_match(m, ...) in the
> calling function, the value of m has been lost - using the old one is
> incorrect and anything might happen (up to and including a panic).
>
> This is why so many mbuf functions pass a struct mbuf ** instead of a
> struct mbuf * so when the parameter is changed, it gets passed back to
> the caller. This function probably needs something like that (the other
> way is returning the updated m value, but I don't think that fits here.)
>
> Unfortunately, because of the way it gets called, this might not be an
> easy change to make.
>
> The problem occurs way back in encap4_input() which does
>
> match = encap4_lookup(m,...);
>
> and then later
>
> encap_fillarg(m, match);
> or
> m_freem(m);
> or
> rip_input(m, off, proto);
>
> all of which use the 'm' that was passed to encap4_lookup().
>
> encap4_lookup does ..
>
> prio = (*ep->func)(m, off, proto, ep->arg);
>
> where *ep->func is in_l2tp_match which does the m_pullup() or now
> m_ensure_contig() instead which potentially changes what 'm' should
> be (the old one may have been freed and replaced by a different one.)
Just wondering, is there scope for a (rather heavy) debug option in
which m_ensure_contig() always returns a new mbuf for valid input?
David
