Re: m_defrag(9) leak
On Thu, Sep 03, 2020 at 07:15:23AM +0200, Claudio Jeker wrote:
> On Thu, Sep 03, 2020 at 06:34:38AM +0200, Bjorn Ketelaars wrote:
> > The diff below should fix this. At least I'm able to build a kernel
> > (amd64 only), which doesn't explode when actually using it.
>
> This is probably not correct. m_defrag does not free the mbuf on failure
> because in the drivers the mbuf is most probably still sitting on the
> interace output queue and will be retried later on.
It might still be worth taking a careful look at each of these.
Even if you don't end up finding a bug, you'll end up understanding
driver code much better.
Generally if a driver leaks mbufs people will notice quickly.
But some drivers aren't used a lot and we've had some mbuf leaks being
noticed only after weeks. I fixed one mbuf leak in urtwn(4) in July,
where the bug was introduced in June (and was unrelated to m_defrag()).
In any case, such changes should be tested with relevant devices before commit.
As claudio says, when reviewing these you need to find out where the mbuf
came from and when it is supposed to be freed. In most cases drivers will
refer to mbufs by other data structures on lists or queues. These mbufs
must not be freed while they're still being referred to.
For example:
> > diff --git sys/arch/octeon/dev/if_cnmac.c sys/arch/octeon/dev/if_cnmac.c
> > index c45503c3d7f..c6a12d20a9a 100644
> > --- sys/arch/octeon/dev/if_cnmac.c
> > +++ sys/arch/octeon/dev/if_cnmac.c
> > @@ -755,8 +755,10 @@ cnmac_send_makecmd_gbuf(struct cnmac_softc *sc, struct
> > mbuf *m0,
> > return 0;
> >
> > defrag:
> > - if (m_defrag(m0, M_DONTWAIT) != 0)
> > + if (m_defrag(m0, M_DONTWAIT) != 0) {
> > + m_freem(m0);
> > return 1;
> > + }
> > gbuf[0] = cnmac_send_makecmd_w1(m0->m_len, KVTOPHYS(m0->m_data));
> > *rsegs = 1;
> > return 0;
In this instance, cnmac_start() will already free the mbuf on failure,
so your change introduces a double-free. On success, the mbuf ends up on
a list (via cnmac_send()) and will be freed later (via cnmac_free_task()).
Re: m_defrag(9) leak
On Thu, Sep 03, 2020 at 06:34:38AM +0200, Bjorn Ketelaars wrote:
> On Tue 25/08/2020 08:42, Martin Pieuchot wrote:
> > Maxime Villard mentioned a leak due to a missing m_freem() in wg(4):
> > https://marc.info/?l=netbsd-tech-net=159827988018641=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.
>
> Similar issue seems to exist in:
>
> sys/arch/octeon/dev/if_cnmac.c
> sys/arch/octeon/dev/if_ogx.c
> sys/dev/fdt/if_dwge.c
> sys/dev/fdt/if_dwxe.c
> sys/dev/ic/bcmgenet.c
> sys/dev/ic/gem.c
> sys/dev/ic/re.c
> sys/dev/ic/rt2860.c
> sys/dev/pci/if_bge.c
> sys/dev/pci/if_bnx.c
> sys/dev/pci/if_bnxt.c
> sys/dev/pci/if_bwfm_pci.c
> sys/dev/pci/if_cas.c
> sys/dev/pci/if_em.c
> sys/dev/pci/if_iavf.c
> sys/dev/pci/if_ix.c
> sys/dev/pci/if_ixl.c
> sys/dev/pci/if_mcx.c
> sys/dev/pci/if_msk.c
> sys/dev/pci/if_myx.c
> sys/dev/pci/if_rge.c
> sys/dev/pci/if_se.c
> sys/dev/pci/if_sis.c
> sys/dev/pci/if_sk.c
> sys/dev/pci/if_vge.c
> sys/dev/pci/if_vic.c
> sys/dev/pci/if_vmx.c
> sys/dev/pci/if_vr.c
> sys/dev/pv/if_hvn.c
> sys/dev/pv/if_vio.c
> sys/dev/pv/if_xnf.c
>
> The diff below should fix this. At least I'm able to build a kernel
> (amd64 only), which doesn't explode when actually using it.
This is probably not correct. m_defrag does not free the mbuf on failure
because in the drivers the mbuf is most probably still sitting on the
interace output queue and will be retried later on.
This is by design. wg(4) abused m_defrag for something else and its error
handling was just wrong.
> diff --git sys/arch/octeon/dev/if_cnmac.c sys/arch/octeon/dev/if_cnmac.c
> index c45503c3d7f..c6a12d20a9a 100644
> --- sys/arch/octeon/dev/if_cnmac.c
> +++ sys/arch/octeon/dev/if_cnmac.c
> @@ -755,8 +755,10 @@ cnmac_send_makecmd_gbuf(struct cnmac_softc *sc, struct
> mbuf *m0,
> return 0;
>
> defrag:
> - if (m_defrag(m0, M_DONTWAIT) != 0)
> + if (m_defrag(m0, M_DONTWAIT) != 0) {
> + m_freem(m0);
> return 1;
> + }
> gbuf[0] = cnmac_send_makecmd_w1(m0->m_len, KVTOPHYS(m0->m_data));
> *rsegs = 1;
> return 0;
> diff --git sys/arch/octeon/dev/if_ogx.c sys/arch/octeon/dev/if_ogx.c
> index 13a9cd32a7d..67695fabb23 100644
> --- sys/arch/octeon/dev/if_ogx.c
> +++ sys/arch/octeon/dev/if_ogx.c
> @@ -1243,8 +1243,10 @@ ogx_send_mbuf(struct ogx_softc *sc, struct mbuf *m0)
> }
>
> if (m != NULL) {
> - if (m_defrag(m0, M_DONTWAIT) != 0)
> + if (m_defrag(m0, M_DONTWAIT) != 0) {
> + m_freem(m0);
> return ENOMEM;
> + }
>
> /* Discard previously set fragments. */
> scroff -= sizeof(word) * nfrags;
> diff --git sys/dev/fdt/if_dwge.c sys/dev/fdt/if_dwge.c
> index 58114bf3e38..e9bef31fcdc 100644
> --- sys/dev/fdt/if_dwge.c
> +++ sys/dev/fdt/if_dwge.c
> @@ -1136,8 +1136,10 @@ dwge_encap(struct dwge_softc *sc, struct mbuf *m, int
> *idx)
> map = sc->sc_txbuf[cur].tb_map;
>
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return (EFBIG);
> + }
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
> return (EFBIG);
> }
> diff --git sys/dev/fdt/if_dwxe.c sys/dev/fdt/if_dwxe.c
> index e4895a9a044..a3c6122840a 100644
> --- sys/dev/fdt/if_dwxe.c
> +++ sys/dev/fdt/if_dwxe.c
> @@ -1198,8 +1198,10 @@ dwxe_encap(struct dwxe_softc *sc, struct mbuf *m, int
> *idx)
> map = sc->sc_txbuf[cur].tb_map;
>
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return (EFBIG);
> + }
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
> return (EFBIG);
> }
> diff --git sys/dev/ic/bcmgenet.c sys/dev/ic/bcmgenet.c
> index c9b21f7768b..2444b71a68a 100644
> --- sys/dev/ic/bcmgenet.c
> +++ sys/dev/ic/bcmgenet.c
> @@ -215,15 +215,19 @@ genet_setup_txbuf(struct genet_softc *sc, int index,
> struct mbuf *m)
>* than the minimum Ethernet packet size.
>*/
> if (m->m_len < ETHER_MIN_LEN - ETHER_CRC_LEN) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return 0;
> + }
> }
>
> error = bus_dmamap_load_mbuf(sc->sc_tx.buf_tag,
> sc->sc_tx.buf_map[index].map, m, BUS_DMA_WRITE | BUS_DMA_NOWAIT);
> if (error == EFBIG) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> +
Re: m_defrag(9) leak
On Tue 25/08/2020 08:42, Martin Pieuchot wrote:
> Maxime Villard mentioned a leak due to a missing m_freem() in wg(4):
> https://marc.info/?l=netbsd-tech-net=159827988018641=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.
Similar issue seems to exist in:
sys/arch/octeon/dev/if_cnmac.c
sys/arch/octeon/dev/if_ogx.c
sys/dev/fdt/if_dwge.c
sys/dev/fdt/if_dwxe.c
sys/dev/ic/bcmgenet.c
sys/dev/ic/gem.c
sys/dev/ic/re.c
sys/dev/ic/rt2860.c
sys/dev/pci/if_bge.c
sys/dev/pci/if_bnx.c
sys/dev/pci/if_bnxt.c
sys/dev/pci/if_bwfm_pci.c
sys/dev/pci/if_cas.c
sys/dev/pci/if_em.c
sys/dev/pci/if_iavf.c
sys/dev/pci/if_ix.c
sys/dev/pci/if_ixl.c
sys/dev/pci/if_mcx.c
sys/dev/pci/if_msk.c
sys/dev/pci/if_myx.c
sys/dev/pci/if_rge.c
sys/dev/pci/if_se.c
sys/dev/pci/if_sis.c
sys/dev/pci/if_sk.c
sys/dev/pci/if_vge.c
sys/dev/pci/if_vic.c
sys/dev/pci/if_vmx.c
sys/dev/pci/if_vr.c
sys/dev/pv/if_hvn.c
sys/dev/pv/if_vio.c
sys/dev/pv/if_xnf.c
The diff below should fix this. At least I'm able to build a kernel
(amd64 only), which doesn't explode when actually using it.
diff --git sys/arch/octeon/dev/if_cnmac.c sys/arch/octeon/dev/if_cnmac.c
index c45503c3d7f..c6a12d20a9a 100644
--- sys/arch/octeon/dev/if_cnmac.c
+++ sys/arch/octeon/dev/if_cnmac.c
@@ -755,8 +755,10 @@ cnmac_send_makecmd_gbuf(struct cnmac_softc *sc, struct
mbuf *m0,
return 0;
defrag:
- if (m_defrag(m0, M_DONTWAIT) != 0)
+ if (m_defrag(m0, M_DONTWAIT) != 0) {
+ m_freem(m0);
return 1;
+ }
gbuf[0] = cnmac_send_makecmd_w1(m0->m_len, KVTOPHYS(m0->m_data));
*rsegs = 1;
return 0;
diff --git sys/arch/octeon/dev/if_ogx.c sys/arch/octeon/dev/if_ogx.c
index 13a9cd32a7d..67695fabb23 100644
--- sys/arch/octeon/dev/if_ogx.c
+++ sys/arch/octeon/dev/if_ogx.c
@@ -1243,8 +1243,10 @@ ogx_send_mbuf(struct ogx_softc *sc, struct mbuf *m0)
}
if (m != NULL) {
- if (m_defrag(m0, M_DONTWAIT) != 0)
+ if (m_defrag(m0, M_DONTWAIT) != 0) {
+ m_freem(m0);
return ENOMEM;
+ }
/* Discard previously set fragments. */
scroff -= sizeof(word) * nfrags;
diff --git sys/dev/fdt/if_dwge.c sys/dev/fdt/if_dwge.c
index 58114bf3e38..e9bef31fcdc 100644
--- sys/dev/fdt/if_dwge.c
+++ sys/dev/fdt/if_dwge.c
@@ -1136,8 +1136,10 @@ dwge_encap(struct dwge_softc *sc, struct mbuf *m, int
*idx)
map = sc->sc_txbuf[cur].tb_map;
if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
- if (m_defrag(m, M_DONTWAIT))
+ if (m_defrag(m, M_DONTWAIT)) {
+ m_freem(m);
return (EFBIG);
+ }
if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
return (EFBIG);
}
diff --git sys/dev/fdt/if_dwxe.c sys/dev/fdt/if_dwxe.c
index e4895a9a044..a3c6122840a 100644
--- sys/dev/fdt/if_dwxe.c
+++ sys/dev/fdt/if_dwxe.c
@@ -1198,8 +1198,10 @@ dwxe_encap(struct dwxe_softc *sc, struct mbuf *m, int
*idx)
map = sc->sc_txbuf[cur].tb_map;
if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
- if (m_defrag(m, M_DONTWAIT))
+ if (m_defrag(m, M_DONTWAIT)) {
+ m_freem(m);
return (EFBIG);
+ }
if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
return (EFBIG);
}
diff --git sys/dev/ic/bcmgenet.c sys/dev/ic/bcmgenet.c
index c9b21f7768b..2444b71a68a 100644
--- sys/dev/ic/bcmgenet.c
+++ sys/dev/ic/bcmgenet.c
@@ -215,15 +215,19 @@ genet_setup_txbuf(struct genet_softc *sc, int index,
struct mbuf *m)
* than the minimum Ethernet packet size.
*/
if (m->m_len < ETHER_MIN_LEN - ETHER_CRC_LEN) {
- if (m_defrag(m, M_DONTWAIT))
+ if (m_defrag(m, M_DONTWAIT)) {
+ m_freem(m);
return 0;
+ }
}
error = bus_dmamap_load_mbuf(sc->sc_tx.buf_tag,
sc->sc_tx.buf_map[index].map, m, BUS_DMA_WRITE | BUS_DMA_NOWAIT);
if (error == EFBIG) {
- if (m_defrag(m, M_DONTWAIT))
+ if (m_defrag(m, M_DONTWAIT)) {
+ m_freem(m);
return 0;
+ }
error = bus_dmamap_load_mbuf(sc->sc_tx.buf_tag,
sc->sc_tx.buf_map[index].map, m,
BUS_DMA_WRITE | BUS_DMA_NOWAIT);
diff --git sys/dev/ic/gem.c sys/dev/ic/gem.c
index 0cd66dbfd63..b5ef3eca8da 100644
--- sys/dev/ic/gem.c
+++ sys/dev/ic/gem.c
@@ -1698,6 +1698,7 @@ gem_load_mbuf(struct gem_softc *sc, struct gem_sxd *sd,
struct mbuf *m)
break;
/* FALLTHROUGH */
Re: m_defrag(9) leak
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 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=159827988018641=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
Re: m_defrag(9) leak
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 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=159827988018641=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? > > -- > :wq Claudio >
Re: m_defrag(9) leak
On Tue, Aug 25, 2020 at 08:38:06PM +1000, Matt Dunwoodie wrote: > On Tue, 25 Aug 2020 08:54:10 +0200 > Claudio Jeker 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=159827988018641=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. -- :wq Claudio
Re: m_defrag(9) leak
On Tue, 25 Aug 2020 08:54:10 +0200 Claudio Jeker 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=159827988018641=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. 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. (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) &&
Re: m_defrag(9) leak
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=159827988018641=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.
> Index: net//if_wg.c
> ===
> RCS file: /cvs/src/sys/net/if_wg.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 if_wg.c
> --- net//if_wg.c 21 Aug 2020 22:59:27 - 1.12
> +++ net//if_wg.c 25 Aug 2020 06:34:32 -
> @@ -2022,8 +2022,10 @@ wg_input(void *_sc, struct mbuf *m, stru
> /* 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)
> + if (m_defrag(m, M_NOWAIT) != 0) {
> + m_freem(m);
> return NULL;
> + }
>
> if ((m->m_pkthdr.len == sizeof(struct wg_pkt_initiation) &&
> *mtod(m, uint32_t *) == WG_PKT_INITIATION) ||
>
--
:wq Claudio
m_defrag(9) leak
Maxime Villard mentioned a leak due to a missing m_freem() in wg(4):
https://marc.info/?l=netbsd-tech-net=159827988018641=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.
Index: net//if_wg.c
===
RCS file: /cvs/src/sys/net/if_wg.c,v
retrieving revision 1.12
diff -u -p -r1.12 if_wg.c
--- net//if_wg.c21 Aug 2020 22:59:27 - 1.12
+++ net//if_wg.c25 Aug 2020 06:34:32 -
@@ -2022,8 +2022,10 @@ wg_input(void *_sc, struct mbuf *m, stru
/* 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)
+ if (m_defrag(m, M_NOWAIT) != 0) {
+ m_freem(m);
return NULL;
+ }
if ((m->m_pkthdr.len == sizeof(struct wg_pkt_initiation) &&
*mtod(m, uint32_t *) == WG_PKT_INITIATION) ||
