Re: [PATCH iwl-net v4 1/2] idpf: don't use the Tx timestamp trackers once removal has started

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: idpf: UAF in idpf_handle_event_link during vport teardown
--

commit 5304a7984fe1d9aebd2f5a30c9206db3b232eff1
Author: Tian Xun Ng <tianxun.ng@bytedance.com>

idpf: don't use the Tx timestamp trackers once removal has started
    
On remove, idpf_vc_core_deinit() keeps the virtchnl transaction manager
up until the vports have been torn down, so mbx_task keeps processing
replies while idpf_ptp_release() frees the Tx timestamp trackers. An
async VIRTCHNL2_OP_PTP_GET_VPORT_TX_TSTAMP reply handled in that window
reaches idpf_ptp_get_tx_tstamp_async_handler(), which reads
vport->tx_tstamp_caps and takes its lock after
idpf_ptp_release_vport_tstamp() may already have freed it.

Once IDPF_REMOVE_IN_PROG is set, wait for a mailbox task that may be
running a callback before releasing anything, and make the Tx timestamp
callback return when the bit is set. Matched replies to synchronous
transactions are not affected, so the vport teardown still receives
them; mbx_task is requeued straight away for that.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 1caf527069737..d0db6064757cf 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -3204,8 +3204,18 @@ void idpf_vc_core_deinit(struct idpf_adapter *adapter)
>  
>  	/* Avoid transaction timeouts when called during reset */
>  	remove_in_prog = test_bit(IDPF_REMOVE_IN_PROG, adapter->flags);
> -	if (!remove_in_prog)
> +	if (!remove_in_prog) {
>  		libie_ctlq_xn_shutdown(adapter->xnm);
> +	} else {
> +		/* The transaction manager stays up for the teardown below, so
> +		 * async replies can still arrive. Wait for a mailbox task that
> +		 * may be running an async callback; callbacks that run later
> +		 * see IDPF_REMOVE_IN_PROG and leave the state released below
> +		 * alone.
> +		 */
> +		cancel_delayed_work_sync(&adapter->mbx_task);
> +		queue_delayed_work(adapter->mbx_wq, &adapter->mbx_task, 0);

[Severity: High]
Does requeuing the mailbox task here introduce a use-after-free window
during vport teardown?

While the task is running, it can process an async
VIRTCHNL2_EVENT_LINK_CHANGE message and trigger idpf_handle_event_link(),
which searches adapter->vports without locks via idpf_vid_to_vport():

drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:idpf_handle_event_link() {
	...
	vport = idpf_vid_to_vport(adapter, le32_to_cpu(v2e->vport_id));
	if (!vport) {
		...
	}
	np = netdev_priv(vport->netdev);

	np->link_speed_mbps = le32_to_cpu(v2e->link_speed);
	...
}

> +	}
>  
>  	idpf_ptp_release(adapter);
>  	idpf_deinit_task(adapter);

Concurrently, idpf_deinit_task() iterates over vports and frees them:

drivers/net/ethernet/intel/idpf/idpf_lib.c:idpf_vport_dealloc() {
	...
	idpf_vport_rel(vport);

	adapter->vports[i] = NULL;
	...
}

Because idpf_vport_rel() calls kfree(vport) before the adapter->vports[i]
array element is set to NULL, idpf_vid_to_vport() in the concurrently
running mbx_task could find and return a pointer to freed memory, leading
to a use-after-free when idpf_handle_event_link() dereferences it.

Could idpf_handle_event_link() check for IDPF_REMOVE_IN_PROG, or could the
vports array be updated before the memory is freed?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012063027.27750-1-luckilystar08@gmail.com?part=1