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 <[email protected]> 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/[email protected]?part=1
