Re: [PATCH iwl-net v3] idpf: keep the mailbox up while tearing down vports on shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: idpf: use-after-free of PTP tx_tstamp_caps during shutdown
--

commit 5449e12bf292e1f9d4473ed0d51fe60b2a67f6b3
Author: Tian Xun Ng <tianxun.ng@bytedance.com>

idpf: keep the mailbox up while tearing down vports on shutdown

Since commit 4c9106f4906a ("idpf: fix adapter NULL pointer dereference
on reboot"), idpf_shutdown() calls idpf_vc_core_deinit() directly instead
of idpf_remove(), so IDPF_REMOVE_IN_PROG is not set on shutdown.
idpf_vc_core_deinit() uses that flag to decide when to shut the virtchnl
transaction manager down.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
> index 129bccaa6baa8..ec751f4a6deb6 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_main.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
> @@ -194,8 +194,17 @@ static void idpf_shutdown(struct pci_dev *pdev)
>  {
>  	struct idpf_adapter *adapter = pci_get_drvdata(pdev);
>  
> +	/* As in idpf_remove(): keep the event task from being requeued, and
> +	 * destroy the vports while the mailbox is still up.
> +	 */
> +	set_bit(IDPF_REMOVE_IN_PROG, adapter->flags);

[Severity: High]
Does setting IDPF_REMOVE_IN_PROG here expose a use-after-free race during reboot?

When this flag is set, idpf_vc_core_deinit() defers shutting down the
transaction manager (via libie_ctlq_xn_shutdown()) until the very end.
However, it also calls idpf_ptp_release() earlier in the function, which
synchronously frees vport->tx_tstamp_caps via idpf_ptp_release_vport_tstamp()
in drivers/net/ethernet/intel/idpf/idpf_ptp.c.

Because the transaction manager and mailbox task remain active during this
window, an asynchronous PTP Tx timestamp reply can still be received.

If that happens, the mailbox handler idpf_ptp_get_tx_tstamp_async_handler()
in drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c can dereference the
freed tx_tstamp_caps pointer, leading to a panic.

While the commit message mentions that a race already exists in the remove
path, setting this flag appears to newly expose the same vulnerability in the
shutdown path.

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