On Mon, Aug 31, 2020 at 08:50:09AM +0200, Theo Buehler wrote: > On Tue, Aug 25, 2020 at 03:28:03PM +0200, Claudio Jeker wrote: > > On Tue, Aug 25, 2020 at 08:38:06PM +1000, Matt Dunwoodie wrote: > > > On Tue, 25 Aug 2020 08:54:10 +0200 > > > Claudio Jeker <cje...@diehard.n-r-g.com> wrote: > > > > > > > On Tue, Aug 25, 2020 at 08:42:47AM +0200, Martin Pieuchot wrote: > > > > > Maxime Villard mentioned a leak due to a missing m_freem() in wg(4): > > > > > https://marc.info/?l=netbsd-tech-net&m=159827988018641&w=2 > > > > > > > > > > It seems to be that such leak is present in other uses of > > > > > m_defrag() in the tree. I won't take the time to go though all of > > > > > them, but an audit would be welcome. > > > > > > > > Why does wg(4) needs m_defrag? and why is it used in this way? It > > > > does not even check if m_defrag() is actually needed. > > > > > > > > m_defrag() should be used by HW drivers where DMA constraints require > > > > the mbuf to be flattened. Software dirvers should use m_pullup / > > > > m_pulldown etc instead. > > > > > > wg(4) uses m_defrag to bring all mbuf fragments into one, so it can > > > read handshake values, and decrypt data directly from the mbuf. In > > > both cases it needs the full length of the packet. > > > > For the handshake headers m_pullup() or m_pulldown() is normally the way > > to go. Doing a massive pullup to decrypt data is a poor design. You work > > against the network stack here and this comes at a reasonably high price > > (considering you need to allocate a new mbuf, mbuf cluster and copy all > > data over). > > > > > However you're right, it doesn't check if it needs defrag and yes > > > m_pullup is preferable. I have a new patch for m_pullup and includes a > > > comment on why it's done. Thoughts? > > > > > > There is one other case of "m_pullup(m, m->m_pkthdr.len)" in sppp_input. > > > > sppp(4) is not without its own flaws. That code was added because of > > alignment issues. It may actually no longer be needed but I have not the > > time right now to really think this through. > > > > > (To avoid any confusion with the previous patch, there is no need to > > > m_free(m) as m_pullup will do that for us.) > > > > > diff --git if_wg.c if_wg.c > > > index e5a1071ccf1..242bd105e72 100644 > > > --- if_wg.c > > > +++ if_wg.c > > > @@ -2022,7 +2022,13 @@ wg_input(void *_sc, struct mbuf *m, struct ip *ip, > > > struct ip6_hdr *ip6, > > > /* m has a IP/IPv6 header of hlen length, we don't need it anymore. */ > > > m_adj(m, hlen); > > > > > > - if (m_defrag(m, M_NOWAIT) != 0) > > > + /* > > > + * Ensure mbuf is contiguous over full length of packet. This is done > > > + * so we can directly read the handshake values in wg_handshake, and so > > > + * we can decrypt a transport packet by passing a single buffer to > > > + * noise_remote_decrypt in wg_decap. > > > + */ > > > + if ((m = m_pullup(m, m->m_pkthdr.len)) == NULL) > > > return NULL; > > > > > > if ((m->m_pkthdr.len == sizeof(struct wg_pkt_initiation) && > > > > I'm fine with this or the m_defrag fix to go in now to fix the mbuf leak. > > Could someone who understands this please commit either version of the > diff?
I committed this diff now. Thanks
