[v2,1/2] net/af_xdp: fix use after free in af_xdp_tx_zc()

Message ID 20250116225151.188214-2-ariel.otilibili@6wind.com (mailing list archive)
State Changes Requested
Delegated to: Stephen Hemminger
Headers
Series Fix use after free, and refactor af_xdp_tx_zc() |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Ariel Otilibili Jan. 16, 2025, 10:51 p.m. UTC
tx_bytes is computed after both legs are tested. This might
produce a use after memory free.

The computation is now moved into each leg.

Bugzilla ID: 1440
Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

Comments

Maryam Tahhan Jan. 20, 2025, 2:54 p.m. UTC | #1
On 16/01/2025 17:51, Ariel Otilibili wrote:
> tx_bytes is computed after both legs are tested. This might
> produce a use after memory free.
>
> The computation is now moved into each leg.
>
> Bugzilla ID: 1440
> Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
> Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
> ---
>   drivers/net/af_xdp/rte_eth_af_xdp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 814398ba4b44..4326a29f7042 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -574,6 +574,7 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   					umem->mb_pool->header_size;
>   			offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
>   			desc->addr = addr | offset;
> +			tx_bytes += mbuf->pkt_len;
>   			count++;
>   		} else {
>   			struct rte_mbuf *local_mbuf =
> @@ -601,11 +602,10 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   			desc->addr = addr | offset;
>   			rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),
>   					desc->len);
> +			tx_bytes += mbuf->pkt_len;
>   			rte_pktmbuf_free(mbuf);
>   			count++;
>   		}
> -
> -		tx_bytes += mbuf->pkt_len;
>   	}
>   
>   out:

I think that you could've just set tx_bytes to the desc->len as this is 
being set in all scenarios...

tx_bytes += desc->len;
  
Ariel Otilibili Jan. 21, 2025, 8:54 a.m. UTC | #2
Hi Maryam,

On Mon, Jan 20, 2025 at 3:54 PM Maryam Tahhan <mtahhan@redhat.com> wrote:

> I think that you could've just set tx_bytes to the desc->len as this is
> being set in all scenarios...
>
> tx_bytes += desc->len;
>
 Thanks for your feedback. I'll change that.
  

Patch

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 814398ba4b44..4326a29f7042 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -574,6 +574,7 @@  af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					umem->mb_pool->header_size;
 			offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
 			desc->addr = addr | offset;
+			tx_bytes += mbuf->pkt_len;
 			count++;
 		} else {
 			struct rte_mbuf *local_mbuf =
@@ -601,11 +602,10 @@  af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			desc->addr = addr | offset;
 			rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),
 					desc->len);
+			tx_bytes += mbuf->pkt_len;
 			rte_pktmbuf_free(mbuf);
 			count++;
 		}
-
-		tx_bytes += mbuf->pkt_len;
 	}
 
 out: