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

[PATCH net v4] 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.

As identified by Jakub Kicinski, commit 8a8633978b84
("qede: Add build_skb() support.") accidentally dropped
the assignment of tpa_info->buffer.data in qede_tpa_start().

When memory pressure causes an SKB allocation failure in qede_tpa_start(),
the driver sets tpa_start_fail = true and attempts to recycle the physical
page later in qede_tpa_end(). However, because buffer.data was left
uninitialized (NULL), qede_reuse_page() pushes a "ghost"
page (valid mapping but NULL data pointer) back into the
active Rx ring. The next time the hardware uses this descriptor,
it passes a NULL page to qede_fill_frag_skb(), causing a kernel
panic.

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 the root cause by:
1. Restoring the tpa_info->buffer.data assignment in qede_tpa_start().
2. Reverting the error recovery block in qede_tpa_end() to rely on
   tpa_start_fail, which safely recycles the page without causing
   double DMA unmaps.

Additionally, harden the surrounding TPA flow:
3. Add NULL page validation in qede_fill_frag_skb() before dereferencing.
4. Correct bounds checking logic in TPA error loops (evaluating bounds
   before reading the array elements to prevent out-of-bounds reads).
5. Check error state early in qede_tpa_end() and qede_tpa_cont() before
   processing fragments.
6. Ensure NULL buffer descriptors are consumed rather than recycled, and
   correct buffer capacity tracking (rxq->filled_buffers--) to avoid
   Rx ring starvation.

Fixes: 8a8633978b84 ("qede: Add build_skb() support.")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>
---
  v4: Fix err: label handling as identified by Jakub Kicinski:
    - Restored tpa_info->buffer.data assignment in qede_tpa_start(),
      which was dropped by commit 8a8633978b84
    - Reverted err: label in qede_tpa_end() to use tpa_start_fail flag
      instead of buffer.data check (ownership semantics)
    - Updated Fixes: tag to 8a8633978b84
    - Added Suggested-by: Jakub Kicinski
  v3: Addressed additional AI review feedback:
    - Fixed NULL pointer recycling in qede_tpa_cont() and qede_tpa_end()
    - Fixed array bounds check order in TPA error loops
    - Moved version notes after --- per Markus Elfring feedback
    - Resent as independent thread per netdev-bot feedback
  v2: Addressed AI review feedback from Simon Horman:
    - Added net_ratelimit() to prevent printk storm in NAPI fast path
    - Fixed NULL buffer recycling in qede_fill_frag_skb()
    - Added proper cleanup in qede_tpa_end() early exit path
  v1: https://lore.kernel.org/netdev/20260709044704.141507-1-vnagare@redhat.com/

 drivers/net/ethernet/qlogic/qede/qede_fp.c | 56 ++++++++++++++++++++--
 1 file changed, 51 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..bf0448b035b4 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;
 }
 
@@ -845,7 +859,7 @@ static void qede_tpa_start(struct qede_dev *edev,
 					      pad, false);
 	tpa_info->buffer.page_offset = sw_rx_data_cons->page_offset;
 	tpa_info->buffer.mapping = sw_rx_data_cons->mapping;
-
+	tpa_info->buffer.data = sw_rx_data_cons->data;
 	if (unlikely(!tpa_info->skb)) {
 		DP_NOTICE(edev, "Failed to allocate SKB for gro\n");
 
@@ -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]));
@@ -986,6 +1016,22 @@ static int qede_tpa_end(struct qede_dev *edev,
 		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
 			       PAGE_SIZE, rxq->data_direction);
 
+	/* 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;
+	}
+
 	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]));
-- 
2.54.0

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says: "Looks like AI is questioning whether some of the extra
code is really needed. Please TAL, the incorrect indent definitely
needs fixing, however trivial that is."
---
qede: Fix NULL pointer dereference in TPA fragment processing

This restores the tpa_info->buffer.data assignment in qede_tpa_start() that
commit 8a8633978b84 ("qede: Add build_skb() support.") dropped, and adds a
NULL page check plus early error-state exits in the TPA fragment paths.  The
new early exits recycle or consume the BDs described by cqe->len_list.

A few questions on the commit message and on the added hardening below.

First, on the enumerated list of changes:

>     Fix the root cause by:
>     1. Restoring the tpa_info->buffer.data assignment in qede_tpa_start().
>     2. Reverting the error recovery block in qede_tpa_end() to rely on
>        tpa_start_fail, which safely recycles the page without causing
>        double DMA unmaps.

[Severity: Medium]
Does item 2 correspond to anything in this patch?  In the tree being
patched, the err: label in qede_tpa_end() already reads:

	if (tpa_info->tpa_start_fail) {
		qede_reuse_page(rxq, &tpa_info->buffer);
		tpa_info->tpa_start_fail = false;
	}

and that block is not touched by any hunk here, so there is nothing being
reverted relative to the base tree.

>     4. Correct bounds checking logic in TPA error loops (evaluating bounds
>        before reading the array elements to prevent out-of-bounds reads).

[Severity: Medium]
Similarly, which out-of-bounds read does item 4 fix?  The existing loops in
qede_tpa_cont() and qede_tpa_end() already evaluate the bound first:

	for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++)

and they appear only as unchanged context lines in the hunks below.  Both
items 2 and 4 look like deltas against the earlier v3 posting rather than
against the tree, and an advertised out-of-bounds-read fix in a Cc: stable
patch tends to get picked up by backport and CVE tooling.  Could the message
describe only what this patch changes, namely restoring buffer.data, the
NULL page guard, and the early error-state exits with the consume and
filled_buffers accounting?

> diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> index c11e0d8f98aa5..ed9db7399d2fc 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;
> +	}

[Severity: Low]
Do the comment and the message here describe the condition that is actually
detected?  current_bd is just a ring slot:

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

so no allocator runs between function entry and this test, and a NULL data
means the sw_rx_ring entry at the consumer index is the "ghost" BD (valid
mapping, NULL page) that the commit message describes qede_reuse_page() as
pushing back into the ring.  That is a ring bookkeeping inconsistency rather
than an allocation failure.

The text is also very close to the genuine allocation-failure notice already
present in qede_tpa_rx_build_skb():

	DP_NOTICE(edev,
		  "Failed to allocate RX buffer for tpa start\n");

Would wording that names the ghost descriptor make the two distinguishable
in field logs?

[ ... ]

> @@ -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;
>  }
>  
> @@ -850,7 +864,7 @@ static void qede_tpa_start(struct qede_dev *edev,
>  					      pad, false);
>  	tpa_info->buffer.page_offset = sw_rx_data_cons->page_offset;
>  	tpa_info->buffer.mapping = sw_rx_data_cons->mapping;
> -
> +	tpa_info->buffer.data = sw_rx_data_cons->data;

[Severity: Low]
This one assignment appears to be the whole crash fix.  Would it be worth
splitting it out as the Fixes: / Cc: stable patch and sending items 3 to 6
as a separate hardening patch?

Two things make the added hardening look inconsistent with itself.  If a
descriptor with a valid mapping but NULL data really can reach the driver,
the regular receive path still dereferences the same field unguarded:

qede_build_skb()
	buf = page_address(bd->data) + bd->page_offset;

qede_rx_build_skb()
	skb_put_data(skb, page_address(bd->data) + offset, len);

and qede_tpa_start() itself hands sw_rx_data_cons to
qede_tpa_rx_build_skb() -> qede_build_skb() just above this new assignment,
before any validation.  Should those consumers get the same treatment, or is
the ghost BD no longer reachable once buffer.data is restored?

[Severity: Low]
This isn't a bug introduced by this patch, but the comment on
struct qede_agg_info::buffer in qede.h is now stale in a way that touches
this hunk:

	/* rx_buf is a data buffer that can be placed / consumed from rx bd
	 * chain. It has two purposes: We will preallocate the data buffer
	 * for each aggregation when we open the interface and will place this
	 * buffer on the rx-bd-ring when we receive TPA_START.
	 ...

There is no such preallocation any more.  The only TPA initialisation,
qede_set_tpa_param() in qede_main.c, just does:

	tpa_info->state = QEDE_AGG_STATE_NONE;

and nothing allocates, unmaps or frees tpa_info->buffer.data; the real
implementation is the copy of the current Rx consumer descriptor being
completed here.  Since this patch makes that field live again, could the
comment be refreshed to match?

>  	if (unlikely(!tpa_info->skb)) {
>  		DP_NOTICE(edev, "Failed to allocate SKB for gro\n");
>  
> @@ -964,8 +978,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;
> +	}
> +

[Severity: Low]
The if (likely(rx_bd->data)) block carries two extra tabs relative to the
enclosing for loop; checkpatch should flag that.

Beyond the indentation, is this new loop doing anything the existing call
did not?  With state != QEDE_AGG_STATE_START, qede_fill_frag_skb() takes its
out: path immediately and, with this patch applied, runs exactly the same
rx_bd->data test and recycle-or-consume sequence once per len_list entry.

> @@ -991,6 +1021,22 @@ static int qede_tpa_end(struct qede_dev *edev,
>  		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
>  			       PAGE_SIZE, rxq->data_direction);
>  
> +	/* 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;
> +	}
> +

[Severity: Low]
Same question here: the existing fill loop below already falls through to
qede_fill_frag_skb()'s out: path when state != QEDE_AGG_STATE_START, and
then reaches the err: label via the following check.  Is this open-coded
copy of that behaviour needed, given it enlarges a patch marked for stable?

>  	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]));
-- 
pw-bot: cr