[dpdk-dev] [PATCH] ip_frag: fix double free of chained mbufs

[dpdk-dev] [PATCH] ip_frag: fix double free of chained mbufs

[Allain Legacy]

The first mbuf and the last mbuf to be visited in the preceeding loop
are not set to NULL in the fragmentation table.  This creates the
possibility of a double free when the fragmentation table is later freed
with rte_ip_frag_table_destroy().

Fixes: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_ip_frag/rte_ipv4_reassembly.c | 4 +++-
 lib/librte_ip_frag/rte_ipv6_reassembly.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
index 82e831ca3..42974fb8b 100644
--- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
@@ -25,7 +25,7 @@ ipv4_frag_reassemble(struct ip_frag_pkt *fp)
 	/*start from the last fragment. */
 	m = fp->frags[IP_LAST_FRAG_IDX].mb;
 	ofs = fp->frags[IP_LAST_FRAG_IDX].ofs;
-	curr_idx = IP_LAST_FRAG_IDX;
+	curr_idx = 	IP_LAST_FRAG_IDX;
 
 	while (ofs != first_len) {
 
@@ -59,7 +59,9 @@ ipv4_frag_reassemble(struct ip_frag_pkt *fp)
 	/* chain with the first fragment. */
 	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
 	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	fp->frags[curr_idx].mb = NULL;
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
+	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
 
 	/* update mbuf fields for reassembled packet. */
 	m->ol_flags |= PKT_TX_IP_CKSUM;
diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
index 3479fabb8..db249fe60 100644
--- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
@@ -82,7 +82,9 @@ ipv6_frag_reassemble(struct ip_frag_pkt *fp)
 	/* chain with the first fragment. */
 	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
 	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	fp->frags[curr_idx].mb = NULL;
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
+	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
 
 	/* update mbuf fields for reassembled packet. */
 	m->ol_flags |= PKT_TX_IP_CKSUM;
-- 
2.12.1

[dpdk-dev] [PATCH v2] ip_frag: fix double free of chained mbufs

[Allain Legacy]

The first mbuf and the last mbuf to be visited in the preceding loop
are not set to NULL in the fragmentation table.  This creates the
possibility of a double free when the fragmentation table is later freed
with rte_ip_frag_table_destroy().

Fixes: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_ip_frag/rte_ipv4_reassembly.c | 2 ++
 lib/librte_ip_frag/rte_ipv6_reassembly.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
index 82e831ca3..4956b99ea 100644
--- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
@@ -59,7 +59,9 @@ ipv4_frag_reassemble(struct ip_frag_pkt *fp)
 	/* chain with the first fragment. */
 	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
 	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	fp->frags[curr_idx].mb = NULL;
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
+	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
 
 	/* update mbuf fields for reassembled packet. */
 	m->ol_flags |= PKT_TX_IP_CKSUM;
diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
index 3479fabb8..db249fe60 100644
--- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
@@ -82,7 +82,9 @@ ipv6_frag_reassemble(struct ip_frag_pkt *fp)
 	/* chain with the first fragment. */
 	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
 	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	fp->frags[curr_idx].mb = NULL;
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
+	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
 
 	/* update mbuf fields for reassembled packet. */
 	m->ol_flags |= PKT_TX_IP_CKSUM;
-- 
2.12.1

Re: [dpdk-dev] [PATCH v2] ip_frag: fix double free of chained mbufs

[Thomas Monjalon]

Please, any review?

19/03/2018 15:25, Allain Legacy:
> The first mbuf and the last mbuf to be visited in the preceding loop
> are not set to NULL in the fragmentation table.  This creates the
> possibility of a double free when the fragmentation table is later freed
> with rte_ip_frag_table_destroy().
> 
> Fixes: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  lib/librte_ip_frag/rte_ipv4_reassembly.c | 2 ++
>  lib/librte_ip_frag/rte_ipv6_reassembly.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> index 82e831ca3..4956b99ea 100644
> --- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> @@ -59,7 +59,9 @@ ipv4_frag_reassemble(struct ip_frag_pkt *fp)
>  	/* chain with the first fragment. */
>  	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
>  	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
> +	fp->frags[curr_idx].mb = NULL;
>  	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
> +	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
>  
>  	/* update mbuf fields for reassembled packet. */
>  	m->ol_flags |= PKT_TX_IP_CKSUM;
> diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> index 3479fabb8..db249fe60 100644
> --- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> @@ -82,7 +82,9 @@ ipv6_frag_reassemble(struct ip_frag_pkt *fp)
>  	/* chain with the first fragment. */
>  	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
>  	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
> +	fp->frags[curr_idx].mb = NULL;
>  	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
> +	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
>  
>  	/* update mbuf fields for reassembled packet. */
>  	m->ol_flags |= PKT_TX_IP_CKSUM;
> 

Re: [dpdk-dev] [PATCH v2] ip_frag: fix double free of chained mbufs

[Ananyev, Konstantin]

Hi Allain,

> 
> The first mbuf and the last mbuf to be visited in the preceding loop
> are not set to NULL in the fragmentation table.  This creates the
> possibility of a double free when the fragmentation table is later freed
> with rte_ip_frag_table_destroy().
> 
> Fixes: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  lib/librte_ip_frag/rte_ipv4_reassembly.c | 2 ++
>  lib/librte_ip_frag/rte_ipv6_reassembly.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> index 82e831ca3..4956b99ea 100644
> --- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> @@ -59,7 +59,9 @@ ipv4_frag_reassemble(struct ip_frag_pkt *fp)
>  	/* chain with the first fragment. */
>  	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
>  	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
> +	fp->frags[curr_idx].mb = NULL;
>  	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
> +	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;


I wonder why we have to NULL only first and cur entry?
We probably have to NULL each one in that case, right?
If so, then it probably better to do in the same place we do ip_frag_key_invalidate().
As alternative, and probably better approach - can we modify rte_ip_frag_table_destroy(),
so it will free mbufs only for entires with valid keys?
Konstantin 

> 
>  	/* update mbuf fields for reassembled packet. */
>  	m->ol_flags |= PKT_TX_IP_CKSUM;
> diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> index 3479fabb8..db249fe60 100644
> --- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> @@ -82,7 +82,9 @@ ipv6_frag_reassemble(struct ip_frag_pkt *fp)
>  	/* chain with the first fragment. */
>  	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
>  	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
> +	fp->frags[curr_idx].mb = NULL;
>  	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
> +	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
> 
>  	/* update mbuf fields for reassembled packet. */
>  	m->ol_flags |= PKT_TX_IP_CKSUM;
> --
> 2.12.1

Re: [dpdk-dev] [PATCH v2] ip_frag: fix double free of chained mbufs

[Legacy, Allain]

> -----Original Message-----
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Wednesday, April 11, 2018 7:02 AM
<..>
> 
> 
> I wonder why we have to NULL only first and cur entry?
> We probably have to NULL each one in that case, right?

We have to do first and current entries at those locations because 
the code does not clear them properly.  All other entries are cleared by 
the following piece of code but it does not handle the two cases that I am 
addressing with my change.

	/* this mbuf should not be accessed directly */
	fp->frags[curr_idx].mb = NULL;
	curr_idx = i;


> If so, then it probably better to do in the same place we do
> ip_frag_key_invalidate().

I don't feel that ip_frag_key_invalidate is the appropriate place to take care of this.  In the interest of code readability and maintainability it should stick to what its name implies and only invalidate the key and nothing else.    Since the ipv*_frag_reassemble() functions were already setup to set the pointers to NULL it should continue to be done there, but of course since it is does not handle all cases correctly it should be fixed.


> As alternative, and probably better approach - can we modify
> rte_ip_frag_table_destroy(), so it will free mbufs only for entires with valid
> keys?

If you prefer this approach I can start over. 

Allain

Re: [dpdk-dev] [PATCH v2] ip_frag: fix double free of chained mbufs

[Ananyev, Konstantin]

> -----Original Message-----
> From: Legacy, Allain [mailto:Allain.Legacy@windriver.com]
> Sent: Wednesday, April 11, 2018 12:28 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Peters, Matt (Wind River) <matt.peters@windriver.com>; stable@dpdk.org
> Subject: RE: [PATCH v2] ip_frag: fix double free of chained mbufs
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> > Sent: Wednesday, April 11, 2018 7:02 AM
> <..>
> >
> >
> > I wonder why we have to NULL only first and cur entry?
> > We probably have to NULL each one in that case, right?
> 
> We have to do first and current entries at those locations because
> the code does not clear them properly.  All other entries are cleared by
> the following piece of code but it does not handle the two cases that I am
> addressing with my change.
> 
> 	/* this mbuf should not be accessed directly */
> 	fp->frags[curr_idx].mb = NULL;
> 	curr_idx = i;

Ah ok, makes sense.

> 
> 
> > If so, then it probably better to do in the same place we do
> > ip_frag_key_invalidate().
> 
> I don't feel that ip_frag_key_invalidate is the appropriate place to take care of this.  In the interest of code readability and maintainability it
> should stick to what its name implies and only invalidate the key and nothing else.    Since the ipv*_frag_reassemble() functions were
> already setup to set the pointers to NULL it should continue to be done there, but of course since it is does not handle all cases correctly it
> should be fixed.
> 
> 
> > As alternative, and probably better approach - can we modify
> > rte_ip_frag_table_destroy(), so it will free mbufs only for entires with valid
> > keys?
> 
> If you prefer this approach I can start over.

If we already doing that as you pointed above, then probably no need for new solution.
Konstantin

Re: [dpdk-dev] [PATCH v2] ip_frag: fix double free of chained mbufs

[Ananyev, Konstantin]

> -----Original Message-----
> From: Allain Legacy [mailto:allain.legacy@windriver.com]
> Sent: Monday, March 19, 2018 2:25 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Peters, Matt (Wind River) <matt.peters@windriver.com>; stable@dpdk.org
> Subject: [PATCH v2] ip_frag: fix double free of chained mbufs
> 
> The first mbuf and the last mbuf to be visited in the preceding loop
> are not set to NULL in the fragmentation table.  This creates the
> possibility of a double free when the fragmentation table is later freed
> with rte_ip_frag_table_destroy().
> 
> Fixes: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  lib/librte_ip_frag/rte_ipv4_reassembly.c | 2 ++
>  lib/librte_ip_frag/rte_ipv6_reassembly.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> index 82e831ca3..4956b99ea 100644
> --- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> @@ -59,7 +59,9 @@ ipv4_frag_reassemble(struct ip_frag_pkt *fp)
>  	/* chain with the first fragment. */
>  	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
>  	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
> +	fp->frags[curr_idx].mb = NULL;
>  	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
> +	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
> 
>  	/* update mbuf fields for reassembled packet. */
>  	m->ol_flags |= PKT_TX_IP_CKSUM;
> diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> index 3479fabb8..db249fe60 100644
> --- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> @@ -82,7 +82,9 @@ ipv6_frag_reassemble(struct ip_frag_pkt *fp)
>  	/* chain with the first fragment. */
>  	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
>  	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
> +	fp->frags[curr_idx].mb = NULL;
>  	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
> +	fp->frags[IP_FIRST_FRAG_IDX].mb = NULL;
> 
>  	/* update mbuf fields for reassembled packet. */
>  	m->ol_flags |= PKT_TX_IP_CKSUM;
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.12.1

Re: [dpdk-dev] [PATCH v2] ip_frag: fix double free of chained mbufs

[Thomas Monjalon]

> > The first mbuf and the last mbuf to be visited in the preceding loop
> > are not set to NULL in the fragmentation table.  This creates the
> > possibility of a double free when the fragmentation table is later freed
> > with rte_ip_frag_table_destroy().
> > 
> > Fixes: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")

Cc: stable@dpdk.org
Reminder: this Cc must be saved in the commit message for later backport.

> > Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks