[PATCH iwl-net 0/2] idpf: fix use-after-free in PTP auxiliary worker

[PATCH iwl-net 0/2] idpf: fix use-after-free in PTP auxiliary worker

[Emil Tantilov]

The PTP auxiliary worker walks every vport and its Rx queues to refresh
the cached PHC time:

  idpf_ptp_do_aux_work()
    idpf_ptp_update_cached_phctime()
      idpf_for_each_vport(adapter, vport)
        rsrc = &vport->dflt_qv_rsrc;
        for each rsrc->rxq_grps[i]
          WRITE_ONCE(rxq->cached_phc_time, systime)

The worker is armed by idpf_ptp_init() during idpf_vc_core_init() and
re-arms itself every 500 ms. This series resolves use-after-free issues
that will happen when the worker attempts to access resources freed by
the driver on teardown and failed init.

Patch 1 introduces rxq_grps_lock and use it to serialize every transition
of rsrc->rxq_grps against the worker. Discovered in testing, the
conditions and trace are included in the patch description.

Patch 2 moves idpf_ptp_release() before deallocating the vports, in the
error path of idpf_init_task(), matching the logic in
idpf_vc_core_deinit(). Discovered while reviewing the fix in patch 1
with respect to other paths that may be problematic for the PTP worker.
The condition was triggered by injecting a failed vport init. Trace is 
included in the patch.

For both patches the issues are not HW specific and will manifest for as
long as PTP is enabled by FW.

Emil Tantilov (2):
  idpf: fix use-after-free in the PTP auxiliary worker
  idpf: release PTP before freeing vports on the  init task error path

 drivers/net/ethernet/intel/idpf/idpf.h      |  5 ++
 drivers/net/ethernet/intel/idpf/idpf_lib.c  |  9 ++-
 drivers/net/ethernet/intel/idpf/idpf_ptp.c  | 16 ++++-
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 65 +++++++++++++++------
 4 files changed, 73 insertions(+), 22 deletions(-)

-- 
2.37.3

[PATCH iwl-net 1/2] idpf: fix use-after-free in the PTP auxiliary worker

[Emil Tantilov]

idpf_ptp_update_cached_phctime() runs from the PTP auxiliary worker and
walks the Rx queues of every vport to refresh rxq->cached_phc_time. There
is a small window on down that can result in use-after-free, detected by
KASAN:

  BUG: KASAN: slab-use-after-free in idpf_ptp_update_cached_phctime+0x367/0x4a0 [idpf]
  Write of size 8 at addr ff11002146c2a4a0 by task ptp3/8084
  ...
  idpf_ptp_update_cached_phctime+0x367/0x4a0 [idpf]
  idpf_ptp_do_aux_work+0x37/0x50 [idpf]
  ptp_aux_kworker+0x64/0xd0
  kthread_worker_fn+0x289/0xa20
  ...
  Allocated by task 8615 on cpu 5 at 812.655509s:
  idpf_vport_queues_alloc+0x9e5/0x2580 [idpf]
  idpf_vport_open.constprop.0+0x12c/0x10f0 [idpf]
  idpf_open+0x1bb/0x230 [idpf]
  __dev_open+0x2f5/0x7c0
  ...
  Freed by task 8628 on cpu 10 at 814.925757s:
  idpf_rxq_group_rel.part.0+0xef/0x5a0 [idpf]
  idpf_vport_queues_rel+0xaf/0x140 [idpf]
  idpf_vport_stop+0x49e/0x850 [idpf]
  idpf_stop+0x6d/0xc0 [idpf]
  __dev_close_many+0x24c/0x650

The existing rxq_grps test does not help. It is evaluated before the
pointer is read again, so the worker can equally run past the kfree() and
dereference a NULL rxq_grps with a stale num_rxq_grp.

Add a per vport rxq_grps_lock and use it to serialize every transition of
rsrc->rxq_grps against the worker:

  - Teardown: idpf_rxq_group_rel() sets rsrc->rxq_grps to NULL under the
    lock before anything reachable through it is freed. The worker
    re-reads the pointer under the same lock, so it either sees a live
    array while the release path is still blocked on the lock, or sees
    NULL and skips that vport. The frees stay outside the lock.

  - Allocation: Build the rxq_grps array privately and publish it to
    rsrc->rxq_grps under the lock only once every group is complete. The
    freeing of the group contents moves to idpf_rxq_grps_free(), which
    takes the array explicitly so the allocation error path can unwind
    safely.

  - Soft reset: idpf_vport_stop() freed rxq_grps and cleared the live
    vport's copy under rxq_grps_lock. Drop the stale pointer in the clone
    so the memcpy() below does not republish the freed array to
    the PTP aux worker before idpf_vport_open() reallocates it. The lock
    sits past the offsetof(struct idpf_vport, link_up) boundary that
    idpf_initiate_soft_reset() stops its memcpy() at, next to the wait
    queues and mutexes that are already kept out of the copy, so a soft
    reset never byte-copies it. The data it guards, dflt_qv_rsrc, is
    inside that copied region, which is why the clone has to be cleared
    rather than relying on the lock alone.

Also, fix the NULL check in idpf_ptp_update_phctime_rxq_grp(). The split
queue case took the address of a member before testing it, which can never
be NULL, so test the rxq_sets[] entry itself.

Fixes: 1a49cf814fe1 ("idpf: add Tx timestamp flows")
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf.h      |  5 ++
 drivers/net/ethernet/intel/idpf/idpf_lib.c  |  6 ++
 drivers/net/ethernet/intel/idpf/idpf_ptp.c  | 16 ++++-
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 65 +++++++++++++++------
 4 files changed, 71 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
index 470bc23c844c..9d63feda1bc6 100644
--- a/drivers/net/ethernet/intel/idpf/idpf.h
+++ b/drivers/net/ethernet/intel/idpf/idpf.h
@@ -373,6 +373,8 @@ struct idpf_q_vec_rsrc {
  * @default_vport: Use this vport if one isn't specified
  * @crc_enable: Enable CRC insertion offload
  * @link_up: True if link is up
+ * @rxq_grps_lock: Serializes every transition of the @dflt_qv_rsrc.rxq_grps
+ *		   pointer of a vport against the PTP auxiliary worker
  * @tx_tstamp_caps: Capabilities negotiated for Tx timestamping
  * @tstamp_config: The Tx tstamp config
  * @tstamp_task: Tx timestamping task
@@ -406,6 +408,9 @@ struct idpf_vport {
 	bool crc_enable;
 	bool link_up;
 
+	/* Must be past the point idpf_initiate_soft_reset() memcpy()s */
+	spinlock_t rxq_grps_lock;
+
 	struct idpf_ptp_vport_tx_tstamp_caps *tx_tstamp_caps;
 	struct kernel_hwtstamp_config tstamp_config;
 	struct work_struct tstamp_task;
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 827c795afcb6..a1c739cdd540 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1276,6 +1276,7 @@ static struct idpf_vport *idpf_vport_alloc(struct idpf_adapter *adapter,
 	vport->compln_clean_budget = IDPF_TX_COMPLQ_CLEAN_BUDGET;
 	vport->default_vport = adapter->num_alloc_vports <
 			       idpf_get_default_vports(adapter);
+	spin_lock_init(&vport->rxq_grps_lock);
 
 	rsrc = &vport->dflt_qv_rsrc;
 	rsrc->dev = &adapter->pdev->dev;
@@ -2075,6 +2076,11 @@ int idpf_initiate_soft_reset(struct idpf_vport *vport,
 	} else {
 		set_bit(IDPF_VPORT_DEL_QUEUES, vport->flags);
 		idpf_vport_stop(vport, false);
+
+		/* Drop the stale pointer in the clone, as rxq_grps was freed
+		 * by idpf_vport_stop().
+		 */
+		new_rsrc->rxq_grps = NULL;
 	}
 
 	err = idpf_send_add_queues_msg(adapter, vport_config, new_rsrc,
diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
index 71fe8b2a8b4e..9b15973d2f7e 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
@@ -348,9 +348,11 @@ idpf_ptp_update_phctime_rxq_grp(const struct idpf_rxq_group *grp, bool split,
 		}
 	} else {
 		for (i = 0; i < grp->splitq.num_rxq_sets; i++) {
+			if (!grp->splitq.rxq_sets[i])
+				continue;
+
 			rxq = &grp->splitq.rxq_sets[i]->rxq;
-			if (rxq)
-				WRITE_ONCE(rxq->cached_phc_time, systime);
+			WRITE_ONCE(rxq->cached_phc_time, systime);
 		}
 	}
 }
@@ -387,10 +389,17 @@ static int idpf_ptp_update_cached_phctime(struct idpf_adapter *adapter)
 		struct idpf_q_vec_rsrc *rsrc;
 		bool split;
 
-		if (!vport || !vport->dflt_qv_rsrc.rxq_grps)
+		if (!vport)
 			continue;
 
 		rsrc = &vport->dflt_qv_rsrc;
+
+		spin_lock(&vport->rxq_grps_lock);
+		if (!rsrc->rxq_grps) {
+			spin_unlock(&vport->rxq_grps_lock);
+			continue;
+		}
+
 		split = idpf_is_queue_model_split(rsrc->rxq_model);
 
 		for (u16 i = 0; i < rsrc->num_rxq_grp; i++) {
@@ -398,6 +407,7 @@ static int idpf_ptp_update_cached_phctime(struct idpf_adapter *adapter)
 
 			idpf_ptp_update_phctime_rxq_grp(grp, split, systime);
 		}
+		spin_unlock(&vport->rxq_grps_lock);
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index af3213eae393..96f79554a4d0 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -1358,16 +1358,15 @@ static void idpf_rxq_sw_queue_rel(struct idpf_rxq_group *rx_qgrp)
 }
 
 /**
- * idpf_rxq_group_rel - Release all resources for rxq groups
+ * idpf_rxq_grps_free - Free an Rx queue group array and everything in it
  * @rsrc: pointer to queue and vector resources
+ * @rxq_grps: group array to free, not reachable through @rsrc any more
  */
-static void idpf_rxq_group_rel(struct idpf_q_vec_rsrc *rsrc)
+static void idpf_rxq_grps_free(struct idpf_q_vec_rsrc *rsrc,
+			       struct idpf_rxq_group *rxq_grps)
 {
-	if (!rsrc->rxq_grps)
-		return;
-
 	for (unsigned int i = 0; i < rsrc->num_rxq_grp; i++) {
-		struct idpf_rxq_group *rx_qgrp = &rsrc->rxq_grps[i];
+		struct idpf_rxq_group *rx_qgrp = &rxq_grps[i];
 		u16 num_rxq;
 
 		if (idpf_is_queue_model_split(rsrc->rxq_model)) {
@@ -1388,19 +1387,40 @@ static void idpf_rxq_group_rel(struct idpf_q_vec_rsrc *rsrc)
 			}
 		}
 	}
-	kfree(rsrc->rxq_grps);
+	kfree(rxq_grps);
+}
+
+/**
+ * idpf_rxq_group_rel - Release all resources for rxq groups
+ * @vport: vport the queue groups belong to
+ * @rsrc: pointer to queue and vector resources
+ */
+static void idpf_rxq_group_rel(struct idpf_vport *vport,
+			       struct idpf_q_vec_rsrc *rsrc)
+{
+	struct idpf_rxq_group *rxq_grps;
+
+	if (!rsrc->rxq_grps)
+		return;
+
+	spin_lock(&vport->rxq_grps_lock);
+	rxq_grps = rsrc->rxq_grps;
 	rsrc->rxq_grps = NULL;
+	spin_unlock(&vport->rxq_grps_lock);
+
+	idpf_rxq_grps_free(rsrc, rxq_grps);
 }
 
 /**
  * idpf_vport_queue_grp_rel_all - Release all queue groups
- * @vport: vport to release queue groups for
+ * @vport: vport the queue groups belong to
  * @rsrc: pointer to queue and vector resources
  */
-static void idpf_vport_queue_grp_rel_all(struct idpf_q_vec_rsrc *rsrc)
+static void idpf_vport_queue_grp_rel_all(struct idpf_vport *vport,
+					 struct idpf_q_vec_rsrc *rsrc)
 {
 	idpf_txq_group_rel(rsrc);
-	idpf_rxq_group_rel(rsrc);
+	idpf_rxq_group_rel(vport, rsrc);
 }
 
 /**
@@ -1419,7 +1439,7 @@ void idpf_vport_queues_rel(struct idpf_vport *vport,
 	idpf_rx_desc_rel_all(rsrc);
 
 	idpf_xdpsqs_put(vport);
-	idpf_vport_queue_grp_rel_all(rsrc);
+	idpf_vport_queue_grp_rel_all(vport, rsrc);
 
 	kfree(vport->txqs);
 	vport->txqs = NULL;
@@ -1803,18 +1823,19 @@ static int idpf_rxq_group_alloc(struct idpf_vport *vport,
 				u16 num_rxq)
 {
 	struct idpf_adapter *adapter = vport->adapter;
+	struct idpf_rxq_group *rxq_grps;
 	bool hs, rsc;
-	int err = 0;
+	int err;
 
-	rsrc->rxq_grps = kzalloc_objs(struct idpf_rxq_group, rsrc->num_rxq_grp);
-	if (!rsrc->rxq_grps)
+	rxq_grps = kzalloc_objs(struct idpf_rxq_group, rsrc->num_rxq_grp);
+	if (!rxq_grps)
 		return -ENOMEM;
 
 	hs = idpf_vport_get_hsplit(vport) == ETHTOOL_TCP_DATA_SPLIT_ENABLED;
 	rsc = idpf_is_feature_ena(vport, NETIF_F_GRO_HW);
 
 	for (unsigned int i = 0; i < rsrc->num_rxq_grp; i++) {
-		struct idpf_rxq_group *rx_qgrp = &rsrc->rxq_grps[i];
+		struct idpf_rxq_group *rx_qgrp = &rxq_grps[i];
 
 		rx_qgrp->vport = vport;
 		if (!idpf_is_queue_model_split(rsrc->rxq_model)) {
@@ -1915,9 +1936,17 @@ static int idpf_rxq_group_alloc(struct idpf_vport *vport,
 		}
 	}
 
+	/* Publish only once every group is built, the PTP aux worker reads
+	 * the array as soon as it becomes visible.
+	 */
+	spin_lock(&vport->rxq_grps_lock);
+	rsrc->rxq_grps = rxq_grps;
+	spin_unlock(&vport->rxq_grps_lock);
+
+	return 0;
+
 err_alloc:
-	if (err)
-		idpf_rxq_group_rel(rsrc);
+	idpf_rxq_grps_free(rsrc, rxq_grps);
 
 	return err;
 }
@@ -1948,7 +1977,7 @@ static int idpf_vport_queue_grp_alloc_all(struct idpf_vport *vport,
 	return 0;
 
 err_out:
-	idpf_vport_queue_grp_rel_all(rsrc);
+	idpf_vport_queue_grp_rel_all(vport, rsrc);
 
 	return err;
 }
-- 
2.37.3

[PATCH iwl-net 2/2] idpf: release PTP before freeing vports on the init task error path

[Emil Tantilov]

Release PTP, which cancels the aux worker, before the unwind frees the
vports, matching the idpf_vc_core_deinit() order. Previously it was
possible for idpf_ptp_update_cached_phctime() which runs via the PTP
auxiliary worker to read a non-NULL but already freed slot, pass its NULL
check, and dereference the freed vport. This can happen on driver load,
where any vport past the first fails to initialize:

  BUG: KASAN: slab-use-after-free in idpf_ptp_update_cached_phctime+0x4ad/0x4d0 [idpf]
  Read of size 8 at addr ff11002129215050 by task ptp3/10120
  ...
  idpf_ptp_update_cached_phctime+0x4ad/0x4d0 [idpf]
  idpf_ptp_do_aux_work+0x37/0x50 [idpf]
  ptp_aux_kworker+0x64/0xd0
  kthread_worker_fn+0x289/0xa20
  ...
  Allocated by task 8820 on cpu 32 at 2113.761375s:
  idpf_init_task+0x321/0x21c0 [idpf]a
  process_one_work+0x917/0x1940
  worker_thread+0x5a5/0xfe0
  ...
  Freed by task 8820 on cpu 32 at 2113.809014s:
  idpf_vport_dealloc+0x8cf/0x16b0 [idpf]
  idpf_init_task+0x245/0x21c0 [idpf]
  process_one_work+0x917/0x1940
  worker_thread+0x5a5/0xfe0

The aux worker is armed in idpf_vc_core_init() before idpf_init_task() is
queued, so it is live throughout a failing init, leading to the scenario
above.

Fixes: 1a49cf814fe1 ("idpf: add Tx timestamp flows")
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reported-by: Konstantin Ilichev <konstantin.ilichev@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index a1c739cdd540..9c541dd6ed4d 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1747,6 +1747,8 @@ void idpf_init_task(struct work_struct *work)
 	return;
 
 unwind_vports:
+	idpf_ptp_release(adapter);
+
 	if (default_vport) {
 		for (index = 0; index < adapter->max_vports; index++) {
 			if (adapter->vports[index])
@@ -1760,7 +1762,6 @@ void idpf_init_task(struct work_struct *work)
 		cancel_delayed_work_sync(&adapter->serv_task);
 		cancel_delayed_work_sync(&adapter->mbx_task);
 	}
-	idpf_ptp_release(adapter);
 
 	clear_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
 }
-- 
2.37.3

Re: [PATCH iwl-net 2/2] idpf: release PTP before freeing vports on the init task error path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] idpf: double free of adapter->ptp due to concurrent idpf_ptp_release()
--

commit 140d271eb2e765967ee1a351714e7ad1ecc7d8f6
Author: Emil Tantilov <emil.s.tantilov@intel.com>

idpf: release PTP before freeing vports on the init task error path

Release PTP, which cancels the aux worker, before the unwind frees the
vports, matching the idpf_vc_core_deinit() order. Previously it was
possible for idpf_ptp_update_cached_phctime() which runs via the PTP
auxiliary worker to read a non-NULL but already freed slot, pass its NULL
check, and dereference the freed vport.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index a1c739cdd5406..9c541dd6ed4d4 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -1747,6 +1747,8 @@ void idpf_init_task(struct work_struct *work)
>  	return;
>  
>  unwind_vports:
> +	idpf_ptp_release(adapter);
> +

[Severity: High]
Does this create a race condition that can result in a double free of
adapter->ptp?

If idpf_remove() or a hard reset is triggered, it invokes idpf_vc_core_deinit().

idpf_vc_core_deinit() calls idpf_ptp_release() before it calls
idpf_deinit_task(), which is what cancels this init_task:

drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:idpf_vc_core_deinit() {
    ...
    idpf_ptp_release(adapter);
    idpf_deinit_task(adapter);
    ...
}

Concurrently, if this asynchronous idpf_init_task() fails during
initialization, its error path (unwind_vports) also calls idpf_ptp_release()
without locking or serialization against concurrent teardown.

idpf_ptp_release() performs an unlocked check-and-free:

drivers/net/ethernet/intel/idpf/idpf_ptp.c:idpf_ptp_release() {
    struct idpf_ptp *ptp = adapter->ptp;

    if (!ptp)
        return;
    ...
    kfree(ptp);
    adapter->ptp = NULL;
}

If both threads execute this concurrently, could both read the non-NULL pointer,
leading to concurrent executions of ptp_clock_unregister() and a double free
via kfree(ptp)?

>  	if (default_vport) {
>  		for (index = 0; index < adapter->max_vports; index++) {
>  			if (adapter->vports[index])
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009212537.3996-1-emil.s.tantilov@intel.com?part=2