[PATCH net v3] qede: Fix NULL pointer dereference in TPA fragment processing

[PATCH net v3] qede: Fix NULL pointer dereference in TPA fragment processing

[Vaibhav Nagare]

Under memory pressure, the qede driver encounters NULL pointer
dereferences when processing TPA continuation fragments because:
1. qede_fill_frag_skb() does not validate the page pointer before use
2. qede_tpa_end() checks error state AFTER calling qede_fill_frag_skb()

The crash occurs when:
1. System experiences memory pressure (GFP_ATOMIC allocations fail)
2. qede_alloc_rx_buffer() returns -ENOMEM, leaving sw_rx_data->data NULL
3. qede_tpa_start() sets QEDE_AGG_STATE_ERROR on SKB allocation failure
4. Hardware delivers TPA_CONT and TPA_END events for this aggregation
5. qede_tpa_end() calls qede_fill_frag_skb() before checking error state
6. qede_fill_frag_skb() accesses NULL pointer in skb_fill_page_desc()
7. Kernel panics with NULL pointer dereference

Example crash from production system:
 BUG: unable to handle kernel NULL pointer dereference at 0x8
 RIP: qede_fill_frag_skb+0x96/0x430 [qede]
 Call Trace:
   qede_rx_int+0xb06/0x1de0
   qede_poll+0x2f4/0x6c0
   __napi_poll+0x2d/0x130

Observed on HPE Synergy 480 Gen11 running RHEL 8.10
(4.18.0-553.134.1.el8_10.x86_64), but the vulnerable code path
exists in mainline.

Fix by:
1. Adding NULL page validation in qede_fill_frag_skb()
   before dereferencing.
2. Checking error state EARLY in qede_tpa_end() and qede_tpa_cont() before
   processing fragments.
3. Ensuring NULL buffer descriptors are consumed rather than recycled to
   prevent NULL pointers from re-entering the active Rx ring.
4. Correcting buffer capacity tracking (rxq->filled_buffers) when dropping
   empty descriptors to avoid Rx ring starvation.
5. Validating uninitialized buffers before reusing them in error paths to
   prevent DMA and memory leaks.

Fixes: 55482edc25f0 ("qede: Add slowpath/fastpath support and enable hardware GRO")
Cc: stable@vger.kernel.org

Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>
---
 v3: Addressed AI review feedback:
   - Fixed out-of-bounds array read by correcting logical AND condition 
     order in qede_tpa_end() loop.
   - Decremented rxq->filled_buffers when dropping NULL descriptors to 
     prevent Rx ring starvation.
   - Checked rx_bd->data validity before recycling BDs in TPA error loops 
     to prevent re-injecting NULL pointers into the active ring.
   - Fixed qede_tpa_end() error jump to prevent qede_reuse_page() from 
     recycling uninitialized buffers and leaking DMA mappings.
   - Moved version history below the '---' marker per subsystem guidelines.

 v2: Addressed AI review feedback from Simon Horman:
   - Added net_ratelimit() to prevent printk storm in NAPI fast path
   - Fixed NULL buffer recycling: check if page is valid before recycling,
     otherwise just consume the BD to prevent NULL from re-entering the ring
   - Added proper cleanup in qede_tpa_end() before early exit to prevent
     memory leaks and ring desynchronization (DMA unmap + BD recycling)

 v1: https://lore.kernel.org/netdev/20260709044704.141507-1-vnagare@redhat.com/

 drivers/net/ethernet/qlogic/qede/qede_fp.c | 59 ++++++++++++++++++++--
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
index 33e18bb69774..dd6032200f52 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
@@ -670,13 +670,23 @@ static int qede_fill_frag_skb(struct qede_dev *edev,
 							 NUM_RX_BDS_MAX];
 	struct qede_agg_info *tpa_info = &rxq->tpa_info[tpa_agg_index];
 	struct sk_buff *skb = tpa_info->skb;
+	struct page *page = current_bd->data;
 
 	if (unlikely(tpa_info->state != QEDE_AGG_STATE_START))
 		goto out;
 
+	/* Avoid NULL pointer dereference when under severe memory pressure */
+	if (unlikely(!page)) {
+		if (net_ratelimit())
+			DP_NOTICE(edev,
+				  "Failed to allocate RX buffer for TPA agg %u\n",
+				  tpa_agg_index);
+		goto out;
+	}
+
 	/* Add one frag and update the appropriate fields in the skb */
 	skb_fill_page_desc(skb, tpa_info->frag_id++,
-			   current_bd->data,
+			   page,
 			   current_bd->page_offset + rxq->rx_headroom,
 			   len_on_bd);
 
@@ -684,7 +694,7 @@ static int qede_fill_frag_skb(struct qede_dev *edev,
 		/* Incr page ref count to reuse on allocation failure
 		 * so that it doesn't get freed while freeing SKB.
 		 */
-		page_ref_inc(current_bd->data);
+		page_ref_inc(page);
 		goto out;
 	}
 
@@ -698,8 +708,12 @@ static int qede_fill_frag_skb(struct qede_dev *edev,
 
 out:
 	tpa_info->state = QEDE_AGG_STATE_ERROR;
-	qede_recycle_rx_bd_ring(rxq, 1);
-
+	if (current_bd->data) {
+		qede_recycle_rx_bd_ring(rxq, 1);
+	} else {
+		qede_rx_bd_ring_consume(rxq);
+		rxq->filled_buffers--;
+	}
 	return -ENOMEM;
 }
 
@@ -959,8 +973,24 @@ static inline void qede_tpa_cont(struct qede_dev *edev,
 				 struct qede_rx_queue *rxq,
 				 struct eth_fast_path_rx_tpa_cont_cqe *cqe)
 {
+	struct qede_agg_info *tpa_info = &rxq->tpa_info[cqe->tpa_agg_index];
 	int i;
 
+	/* Don't process fragments if TPA start failed */
+	if (unlikely(tpa_info->state != QEDE_AGG_STATE_START)) {
+		for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++) {
+			struct sw_rx_data *rx_bd = &rxq->sw_rx_ring[rxq->sw_rx_cons &
+								NUM_RX_BDS_MAX];
+				if (likely(rx_bd->data)) {
+					qede_recycle_rx_bd_ring(rxq, 1);
+				} else {
+					qede_rx_bd_ring_consume(rxq);
+					rxq->filled_buffers--;
+				}
+		}
+		return;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++)
 		qede_fill_frag_skb(edev, rxq, cqe->tpa_agg_index,
 				   le16_to_cpu(cqe->len_list[i]));
@@ -982,6 +1012,22 @@ static int qede_tpa_end(struct qede_dev *edev,
 	tpa_info = &rxq->tpa_info[cqe->tpa_agg_index];
 	skb = tpa_info->skb;
 
+	/* Drop the packet if TPA start failed */
+	if (unlikely(tpa_info->state != QEDE_AGG_STATE_START || !skb)) {
+		/* Recycle BDs from cqe->len_list to keep ring synchronized */
+		for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++) {
+			struct sw_rx_data *rx_bd = &rxq->sw_rx_ring[rxq->sw_rx_cons &
+								NUM_RX_BDS_MAX];
+			if (likely(rx_bd->data)) {
+				qede_recycle_rx_bd_ring(rxq, 1);
+			} else {
+				qede_rx_bd_ring_consume(rxq);
+				rxq->filled_buffers--;
+			}
+		}
+		goto err;
+	}
+
 	if (tpa_info->buffer.page_offset == PAGE_SIZE)
 		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
 			       PAGE_SIZE, rxq->data_direction);
@@ -1023,8 +1069,11 @@ static int qede_tpa_end(struct qede_dev *edev,
 err:
 	tpa_info->state = QEDE_AGG_STATE_NONE;
 
-	if (tpa_info->tpa_start_fail) {
+	if (likely(tpa_info->buffer.data)) {
 		qede_reuse_page(rxq, &tpa_info->buffer);
+	} else if (tpa_info->buffer.page_offset == PAGE_SIZE) {
+		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
+			       PAGE_SIZE, rxq->data_direction);
 		tpa_info->tpa_start_fail = false;
 	}
 
-- 
2.54.0

Re: [PATCH net v3] qede: Fix NULL pointer dereference in TPA fragment processing

[Jacob Keller]

On 7/27/2026 2:33 AM, Vaibhav Nagare wrote:
> Under memory pressure, the qede driver encounters NULL pointer
> dereferences when processing TPA continuation fragments because:
> 1. qede_fill_frag_skb() does not validate the page pointer before use
> 2. qede_tpa_end() checks error state AFTER calling qede_fill_frag_skb()
> 
> The crash occurs when:
> 1. System experiences memory pressure (GFP_ATOMIC allocations fail)
> 2. qede_alloc_rx_buffer() returns -ENOMEM, leaving sw_rx_data->data NULL
> 3. qede_tpa_start() sets QEDE_AGG_STATE_ERROR on SKB allocation failure
> 4. Hardware delivers TPA_CONT and TPA_END events for this aggregation
> 5. qede_tpa_end() calls qede_fill_frag_skb() before checking error state
> 6. qede_fill_frag_skb() accesses NULL pointer in skb_fill_page_desc()
> 7. Kernel panics with NULL pointer dereference
> 
> Example crash from production system:
>  BUG: unable to handle kernel NULL pointer dereference at 0x8
>  RIP: qede_fill_frag_skb+0x96/0x430 [qede]
>  Call Trace:
>    qede_rx_int+0xb06/0x1de0
>    qede_poll+0x2f4/0x6c0
>    __napi_poll+0x2d/0x130
> 
> Observed on HPE Synergy 480 Gen11 running RHEL 8.10
> (4.18.0-553.134.1.el8_10.x86_64), but the vulnerable code path
> exists in mainline.
> 
> Fix by:
> 1. Adding NULL page validation in qede_fill_frag_skb()
>    before dereferencing.
> 2. Checking error state EARLY in qede_tpa_end() and qede_tpa_cont() before
>    processing fragments.
> 3. Ensuring NULL buffer descriptors are consumed rather than recycled to
>    prevent NULL pointers from re-entering the active Rx ring.
> 4. Correcting buffer capacity tracking (rxq->filled_buffers) when dropping
>    empty descriptors to avoid Rx ring starvation.
> 5. Validating uninitialized buffers before reusing them in error paths to
>    prevent DMA and memory leaks.
> 
> Fixes: 55482edc25f0 ("qede: Add slowpath/fastpath support and enable hardware GRO")
> Cc: stable@vger.kernel.org
> 

v3 still has 2 comments from Sashiko, but I am not convinced they're
legitimate. I've outlined them below, but I believe the complaints are
not correct and fail to understand the flow, so:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>
> ---
>  v3: Addressed AI review feedback:
>    - Fixed out-of-bounds array read by correcting logical AND condition 
>      order in qede_tpa_end() loop.
>    - Decremented rxq->filled_buffers when dropping NULL descriptors to 
>      prevent Rx ring starvation.
>    - Checked rx_bd->data validity before recycling BDs in TPA error loops 
>      to prevent re-injecting NULL pointers into the active ring.
>    - Fixed qede_tpa_end() error jump to prevent qede_reuse_page() from 
>      recycling uninitialized buffers and leaking DMA mappings.
>    - Moved version history below the '---' marker per subsystem guidelines.
> 
>  v2: Addressed AI review feedback from Simon Horman:
>    - Added net_ratelimit() to prevent printk storm in NAPI fast path
>    - Fixed NULL buffer recycling: check if page is valid before recycling,
>      otherwise just consume the BD to prevent NULL from re-entering the ring
>    - Added proper cleanup in qede_tpa_end() before early exit to prevent
>      memory leaks and ring desynchronization (DMA unmap + BD recycling)
> 
>  v1: https://lore.kernel.org/netdev/20260709044704.141507-1-vnagare@redhat.com/
> 
>  drivers/net/ethernet/qlogic/qede/qede_fp.c | 59 ++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> index 33e18bb69774..dd6032200f52 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> @@ -670,13 +670,23 @@ static int qede_fill_frag_skb(struct qede_dev *edev,
>  							 NUM_RX_BDS_MAX];
>  	struct qede_agg_info *tpa_info = &rxq->tpa_info[tpa_agg_index];
>  	struct sk_buff *skb = tpa_info->skb;
> +	struct page *page = current_bd->data;
>  
>  	if (unlikely(tpa_info->state != QEDE_AGG_STATE_START))
>  		goto out;
>  
> +	/* Avoid NULL pointer dereference when under severe memory pressure */
> +	if (unlikely(!page)) {
> +		if (net_ratelimit())
> +			DP_NOTICE(edev,
> +				  "Failed to allocate RX buffer for TPA agg %u\n",
> +				  tpa_agg_index);
> +		goto out;
> +	}
> +
>  	/* Add one frag and update the appropriate fields in the skb */
>  	skb_fill_page_desc(skb, tpa_info->frag_id++,
> -			   current_bd->data,
> +			   page,
>  			   current_bd->page_offset + rxq->rx_headroom,
>  			   len_on_bd);
>  
> @@ -684,7 +694,7 @@ static int qede_fill_frag_skb(struct qede_dev *edev,
>  		/* Incr page ref count to reuse on allocation failure
>  		 * so that it doesn't get freed while freeing SKB.
>  		 */
> -		page_ref_inc(current_bd->data);
> +		page_ref_inc(page);
>  		goto out;
>  	}
>  
> @@ -698,8 +708,12 @@ static int qede_fill_frag_skb(struct qede_dev *edev,
>  
>  out:
>  	tpa_info->state = QEDE_AGG_STATE_ERROR;
> -	qede_recycle_rx_bd_ring(rxq, 1);
> -
> +	if (current_bd->data) {
> +		qede_recycle_rx_bd_ring(rxq, 1);
> +	} else {
> +		qede_rx_bd_ring_consume(rxq);
> +		rxq->filled_buffers--;
> +	}
>  	return -ENOMEM;
>  }
>  
> @@ -959,8 +973,24 @@ static inline void qede_tpa_cont(struct qede_dev *edev,
>  				 struct qede_rx_queue *rxq,
>  				 struct eth_fast_path_rx_tpa_cont_cqe *cqe)
>  {
> +	struct qede_agg_info *tpa_info = &rxq->tpa_info[cqe->tpa_agg_index];
>  	int i;
>  
> +	/* Don't process fragments if TPA start failed */
> +	if (unlikely(tpa_info->state != QEDE_AGG_STATE_START)) {
> +		for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++) {
> +			struct sw_rx_data *rx_bd = &rxq->sw_rx_ring[rxq->sw_rx_cons &
> +								NUM_RX_BDS_MAX];
> +				if (likely(rx_bd->data)) {
> +					qede_recycle_rx_bd_ring(rxq, 1);
> +				} else {
> +					qede_rx_bd_ring_consume(rxq);
> +					rxq->filled_buffers--;
> +				}
> +		}
> +		return;
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++)
>  		qede_fill_frag_skb(edev, rxq, cqe->tpa_agg_index,
>  				   le16_to_cpu(cqe->len_list[i]));
> @@ -982,6 +1012,22 @@ static int qede_tpa_end(struct qede_dev *edev,
>  	tpa_info = &rxq->tpa_info[cqe->tpa_agg_index];
>  	skb = tpa_info->skb;
>  
> +	/* Drop the packet if TPA start failed */
> +	if (unlikely(tpa_info->state != QEDE_AGG_STATE_START || !skb)) {
> +		/* Recycle BDs from cqe->len_list to keep ring synchronized */
> +		for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++) {
> +			struct sw_rx_data *rx_bd = &rxq->sw_rx_ring[rxq->sw_rx_cons &
> +								NUM_RX_BDS_MAX];
> +			if (likely(rx_bd->data)) {
> +				qede_recycle_rx_bd_ring(rxq, 1);
> +			} else {
> +				qede_rx_bd_ring_consume(rxq);
> +				rxq->filled_buffers--;
> +			}
> +		}
> +		goto err;
> +	}
> +
>  	if (tpa_info->buffer.page_offset == PAGE_SIZE)
>  		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
>  			       PAGE_SIZE, rxq->data_direction);
> @@ -1023,8 +1069,11 @@ static int qede_tpa_end(struct qede_dev *edev,
>  err:
>  	tpa_info->state = QEDE_AGG_STATE_NONE;
>  
> -	if (tpa_info->tpa_start_fail) {
> +	if (likely(tpa_info->buffer.data)) {
>  		qede_reuse_page(rxq, &tpa_info->buffer);


Sashiko complains here with the following:
> Will this check always evaluate to false? Looking at qede_tpa_start(),
> it appears tpa_info->buffer.data is never assigned:
> qede_tpa_start() {
>     ...
>     tpa_info->buffer.page_offset = sw_rx_data_cons->page_offset;
>     tpa_info->buffer.mapping = sw_rx_data_cons->mapping;
>     /* buffer.data is not set here */
> }
> If buffer.data is never initialized, will this bypass the error recovery
> logic for TPA start failures? This could result in a permanent memory leak
> for the page and DMA mapping, and permanently exhaust the physical ring slots
> since rxq->filled_buffers is never decremented here to account for the lost
> slot. Could the original NULL pointer dereference be fixed by adding the
> missing assignment in qede_tpa_start() instead?


It doesn't seem to realize that buffer.data is assigned by
qede_realloc_rx_buffer or qede_alloc_rx_buffer, so I do not think this
is legitimate.

In particular, I think the AI fails to track that tpa_info->buffer
points to the same memory:

struct sw_rx_data *current_bd = &rxq->sw_rx_ring[rxq->sw_rx_cons &
NUM_RX_BDS_MAX]

vs
struct qede_agg_info *tpa_info = &rxq->tpa_info[cqe->tpa_agg_index];

As a result, it thinks the buffer.data is never initialized and relies
on this conclusion for both this and its next complaint.


> +	} else if (tpa_info->buffer.page_offset == PAGE_SIZE) {
> +		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
> +			       PAGE_SIZE, rxq->data_direction);
>  		tpa_info->tpa_start_fail = false;
>  	}
>  

Here, sashiko says this:

> Does this introduce a double DMA unmap on fragment failure?
> In qede_tpa_end(), if a TPA start was successful and exhausted the page,
> the mapping is unmapped unconditionally at the top of the function:
> qede_tpa_end() {
>     ...
>     if (tpa_info->buffer.page_offset == PAGE_SIZE)
>         dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
>                        PAGE_SIZE, rxq->data_direction);
>     ...
> }
> If a subsequent fragment allocation fails inside qede_fill_frag_skb(),
> execution jumps to the err block. Since tpa_info->buffer.data is always NULL
> (as noted above), and page_offset == PAGE_SIZE, execution would fall through
> to this else if block and unmap the exact same mapping a second time.
> Could this lead to IOMMU faults or memory corruption?

Same as above, this relies on buffer.data not being initialized.