[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.

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

[Jakub Kicinski]

On Wed, 29 Jul 2026 14:57:54 -0700 Jacob Keller wrote:
> 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.

I fed this back into AI and it still disagrees (slop at the end).
Could y'all triple check? I'd also look at bnxt for inspiration
Naming of things are suspiciously similar between these drivers.


  current_bd and tpa_info->buffer are distinct objects:

  - current_bd resides in dynamically allocated rxq->sw_rx_ring.
  - tpa_info->buffer is embedded in rxq->tpa_info[] (drivers/net/ethernet/
    qlogic/qede/qede.h:305).

  qede_alloc_rx_buffer() assigns only a ring entry (drivers/net/ethernet/
  qlogic/qede/qede_fp.c:58). Neither it nor qede_realloc_rx_buffer() assigns
  tpa_info->buffer.data.

  At TPA start, only page_offset and mapping are copied (drivers/net/ethernet/
  qlogic/qede/qede_fp.c:865). Because the RX queue is zero-allocated,
  tpa_info->buffer.data remains NULL.

  Consequently, after a TPA-start failure:

  1. The valid ring descriptor is consumed.
  2. buffer.data remains NULL.
  3. The new check at line 1077 skips qede_reuse_page().
  4. The page/mapping are lost and filled_buffers remains overstated,
     preventing normal refill.

  The history confirms this: before commit 8a8633978b84 added build_skb()
  support, the driver used:

  tpa_info->buffer = *sw_rx_data_cons;

  That commit replaced it with copies of only mapping and page_offset,
  apparently dropping data accidentally.

  The proper correction is to preserve data for the failed-start recovery and
  retain tpa_start_fail as the ownership condition. Merely switching to
  buffer.data is also unsafe: after a successful full-page start, a non-NULL
  pointer does not mean the page may be recycled; it belongs to the skb. The
  reviewer’s concern is legitimate and likely identifies the actual root cause
  of the NULL descriptor.