Re: [PATCH net] octeontx2-pf: fix NULL deref of af_xdp_zc_qidx on rep setup

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent NAPI execution during AF_XDP pool setup/disable causes data races and Use-After-Free.
- [High] Disabling AF_XDP zero-copy leaves the receive queue permanently disabled and lacking a page_pool.
- [High] The AF_XDP zero-copy RSS reconfiguration logic fails to remove the AF_XDP queue from hardware RSS and desyncs from ethtool.
- [High] Prematurely cancelling a work item before unregistering the network device allows it to be re-queued, leading to a Use-After-Free.
- [High] Incorrect index calculation for attaching the XSK buffer pool to XDP queues.
--

--- Patch [1]: [PATCH net] octeontx2-pf: fix NULL deref of af_xdp_zc_qidx on rep setup ---
commit c986623a4193b783891a92b6914ca95ab75cf969
Author: Suman Ghosh <[email protected]>

octeontx2-pf: fix NULL deref of af_xdp_zc_qidx on rep setup

This commit adds NULL checks for af_xdp_zc_qidx before calling test_bit()
in several paths (RSS, ethtool, XSK, pool init) to prevent a NULL pointer
dereference during representor setup in devlink eswitch mode.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 3d253132a17f0..8a36ab8ab19ed 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -333,7 +333,8 @@ int otx2_set_rss_table(struct otx2_nic *pfvf, int ctx_id, const u32 *ind_tbl)
>  	/* Get memory to put this msg */
>  	for (idx = 0; idx < rss->rss_size; idx++) {
>  		/* Ignore the queue if AF_XDP zero copy is enabled */
> -		if (test_bit(ind_tbl[idx], pfvf->af_xdp_zc_qidx))
> +		if (pfvf->af_xdp_zc_qidx &&
> +		    test_bit(ind_tbl[idx], pfvf->af_xdp_zc_qidx))
>  			continue;

[Severity: High]
This isn't a bug introduced by this patch, but does this continue statement
leave the NIX_AQ_CTYPE_RSS initialization message unsent for this index?

If the message is skipped, the hardware RSS table might still point to the
AF_XDP queue. This could cause normal RSS traffic to be incorrectly routed
to the AF_XDP queue.

[ ... ]

[Severity: High]
This is a pre-existing issue in otx2_common.c, but does otx2_sq_init() use
the correct variable for checking XDP queues?

The function currently checks:

    if (qidx > pfvf->hw.xdp_queues)
        otx2_attach_xsk_buff(pfvf, sq, (qidx - pfvf->hw.xdp_queues));

Since XDP queues start at the tx_queues offset, should this check instead
evaluate against pfvf->hw.tx_queues? If tx_queues differs from the number
of XDP (RX) queues, this logic might skip attaching the XSK buffer pool to
the correct XDP queues and instead attach it to normal TX queues.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> index a0340f3422bf9..9bee1b91eeaa6 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> @@ -939,7 +939,8 @@ static int otx2_get_rxfh(struct net_device *dev,
>  
>  	for (idx = 0; idx < rss->rss_size; idx++) {
>  		/* Ignore if the rx queue is AF_XDP zero copy enabled */
> -		if (test_bit(rss->ind_tbl[idx], pfvf->af_xdp_zc_qidx))
> +		if (pfvf->af_xdp_zc_qidx &&
> +		    test_bit(rss->ind_tbl[idx], pfvf->af_xdp_zc_qidx))
>  			continue;
>  		indir[idx] = rss->ind_tbl[idx];
>  	}

[Severity: High]
This is a pre-existing issue, but does skipping this index leave it
unmodified (typically zero) in the ethtool output?

If the index is skipped rather than properly mapped, ethtool might falsely
report queue 0, causing a silent desynchronization with the actual hardware
state.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_xsk.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_xsk.c
> index 7d67b4cbaf716..0e8a6a6486c4a 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_xsk.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_xsk.c
> @@ -193,7 +193,8 @@ int otx2_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
>  
>  void otx2_attach_xsk_buff(struct otx2_nic *pfvf, struct otx2_snd_queue *sq, int qidx)
>  {
> -	if (test_bit(qidx, pfvf->af_xdp_zc_qidx))
> +	if (pfvf->af_xdp_zc_qidx &&
> +	    test_bit(qidx, pfvf->af_xdp_zc_qidx))
>  		sq->xsk_pool = xsk_get_pool_from_qid(pfvf->netdev, qidx);
>  }

[Severity: High]
This is a pre-existing issue in otx2_xsk.c, but is it safe to call
otx2_clean_up_rq() in otx2_xsk_pool_enable() without first stopping the
receive queue or disabling NAPI?

If NAPI is not disabled (e.g., via napi_disable()), the NAPI softirq could
concurrently execute otx2_napi_handler(). This could lead to a
use-after-free as the xdp_rxq_info and hardware buffers are destroyed while
still in use, potentially corrupting the hardware completion queue ring.

[Severity: High]
This is another pre-existing issue in otx2_xsk.c, but does
otx2_xsk_pool_disable() leave the receive queue permanently disabled?

The function disables the hardware queue context via otx2_clean_up_rq() ->
otx2_xsk_ctx_disable(), but doesn't appear to re-enable it or recreate the
standard page_pool buffers for the queue. If otx2_set_rss_table() still
points normal RSS traffic to this queue, the packets could be permanently
dropped until the device is administratively brought down and up.

[Severity: High]
This is a pre-existing issue in otx2_pf.c, but can the reset_task be safely
cancelled while the device is still UP?

In otx2_remove(), cancel_work_sync(&pf->reset_task) is called before
unregister_netdev() shuts down the device. Since interrupts are still
active, could an incoming hardware error interrupt (otx2_q_intr_handler)
fire immediately after the cancellation and place the work item back on
the workqueue? Once the netdev is freed, the subsequently executed
reset_task would dereference freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1