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