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

[PATCH net] 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() before processing fragments
3. Checking error state in qede_tpa_cont() to skip fragment processing

This allows the system to survive memory pressure by dropping packets
instead of crashing.

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

Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>
---
 drivers/net/ethernet/qlogic/qede/qede_fp.c | 25 ++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
index 33e18bb69774..95b5cfcc43c2 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
@@ -670,13 +670,22 @@ 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)) {
+		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 +693,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;
 	}
 
@@ -959,8 +968,16 @@ 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++)
+			qede_recycle_rx_bd_ring(rxq, 1);
+		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 +999,10 @@ 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))
+		goto err;
+
 	if (tpa_info->buffer.page_offset == PAGE_SIZE)
 		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
 			       PAGE_SIZE, rxq->data_direction);
-- 
2.54.0

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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
--- Patch [1]: [PATCH net] qede: Fix NULL pointer dereference in TPA fragment processing ---
commit 437c90c9a6930fba111f83e319f598fb4b80193c
Author: Vaibhav Nagare <nagarevaibhav@gmail.com>

qede: Fix NULL pointer dereference in TPA fragment processing

This commit attempts to fix a NULL pointer dereference under memory pressure
by adding validation in qede_fill_frag_skb() and checking the aggregation
state early in qede_tpa_end() and qede_tpa_cont().

> diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> index c11e0d8f98aa5..18a3167b35c84 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> @@ -670,13 +670,22 @@ 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)) {
> +		DP_NOTICE(edev,
> +			  "Failed to allocate RX buffer for TPA agg %u\n",
> +			  tpa_agg_index);

[Severity: Medium]
Since this function executes per-fragment inside the fast-path NAPI poll
loop, could this unconditional DP_NOTICE cause a printk storm under memory
pressure?

If a TPA event has multiple fragments, this might flood the kernel buffer
and potentially cause soft lockups. Should this be a rate-limited print?

> +		goto out;

[Severity: Critical]
This is a pre-existing issue, but does jumping to the out label when page is
NULL actually fix the crash, or just delay it?

Looking at the error path:

qede_fill_frag_skb() {
...
out:
	tpa_info->state = QEDE_AGG_STATE_ERROR;
	qede_recycle_rx_bd_ring(rxq, 1);
...
}

This blindly recycles the poisoned BD via qede_recycle_rx_bd_ring(), placing
the NULL page pointer back into the software ring instead of dropping it.

When a subsequent normal packet arrives, won't the driver panic when
qede_rx_build_skb() calls skb_put_data(skb, page_address(bd->data) + offset)?

The NULL pointer appears to be introduced earlier in qede_tpa_start() when it
copies the mapping and page_offset on failure, but does not copy the data
pointer. Later, qede_tpa_end() calls qede_reuse_page(), pushing this defective
buffer into the active RX ring.

> +	}
> +
>  	/* 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);

[ ... ]

> @@ -987,6 +1004,10 @@ 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))
> +		goto err;
> +

[Severity: Critical]
By adding this early exit to the err label, do we bypass the teardown steps
required to keep the software and hardware rings in sync?

If a TPA aggregation starts successfully but fails later, tpa_info->state is
set to QEDE_AGG_STATE_ERROR. When the hardware subsequently delivers the
TPA_END completion, this early exit will skip the dma_unmap_page() call
for the initial tpa_info->buffer.

It also skips the loop over cqe->len_list:

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

Unlike the fix in qede_tpa_cont() which manually recycles the BDs, skipping
this abandons the BDs used by the hardware. Won't this permanently
desynchronize the software RX consumer index (sw_rx_cons) from the hardware
producer index, leading to out-of-bounds memory accesses?

>  	if (tpa_info->buffer.page_offset == PAGE_SIZE)
>  		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
>  			       PAGE_SIZE, rxq->data_direction);