[PATCH] net/bnxt: fix TFC mem free use-after-free race

[PATCH] net/bnxt: fix TFC mem free use-after-free race

[Manish Kurup]

From: Mohammad Shuab Siddique <mohammad-shuab.siddique@broadcom.com>

tfc_tbl_scope_mem_free() contains a race window: the
tfo_ts_get_mem_cfg() / tfo_ts_set_mem_cfg() pair allows concurrent
teardown paths to each observe a non-zero mem_cfg and both proceed
to call unlink_and_free() on the same backing-store memory, causing
a use-after-free.

Replace the get/set pair with tfo_ts_get_and_clear_mem_cfg(), which
atomically reads and zeros the slot under rte_spinlock in a single
operation. A concurrent caller receives num_lvl == 0 and skips the
free. Memory release happens outside the lock so the spinlock is
never held across blocking operations.

Fixes: 80317ff6adfd ("net/bnxt/tf_core: support Thor2")
Cc: stable@dpdk.org

Signed-off-by: Mohammad Shuab Siddique <mohammad-shuab.siddique@broadcom.com>
Signed-off-by: Manish Kurup <manish.kurup@broadcom.com>
---
 drivers/net/bnxt/tf_core/v3/tfc_tbl_scope.c | 17 +++---
 drivers/net/bnxt/tf_core/v3/tfo.c           | 57 +++++++++++++++++++++
 drivers/net/bnxt/tf_core/v3/tfo.h           | 30 +++++++++++
 3 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bnxt/tf_core/v3/tfc_tbl_scope.c b/drivers/net/bnxt/tf_core/v3/tfc_tbl_scope.c
index c06099af12..0a5dce43fa 100644
--- a/drivers/net/bnxt/tf_core/v3/tfc_tbl_scope.c
+++ b/drivers/net/bnxt/tf_core/v3/tfc_tbl_scope.c
@@ -1437,22 +1437,21 @@ int tfc_tbl_scope_mem_free(struct tfc *tfcp, uint16_t fid, uint8_t tsid,
 
 	for (region = 0; region < CFA_REGION_TYPE_MAX; region++) {
 		for (dir = 0; dir < CFA_DIR_MAX; dir++) {
-			lrc = tfo_ts_get_mem_cfg(tfcp->tfo, tsid, dir, region, &local,
-						 &mem_cfg);
+			lrc = tfo_ts_get_and_clear_mem_cfg(tfcp->tfo, tsid, dir, region,
+							    &local, &mem_cfg);
 			if (lrc) {
 				rc = lrc;
 				continue;
 			}
+			if (!mem_cfg.num_lvl) {
+				PMD_DRV_LOG_LINE(DEBUG,
+						 "tsid(%d) dir(%d) region(%d) already freed",
+						 tsid, dir, region);
+				continue;
+			}
 			/* memory only allocated on PF */
 			if (is_pf)
 				unlink_and_free(&mem_cfg, mem_cfg.pg_tbl[0].pg_size);
-
-			memset(&mem_cfg, 0, sizeof(mem_cfg));
-
-			/* memory freed, set local to false */
-			local = false;
-			(void)tfo_ts_set_mem_cfg(tfcp->tfo, tsid, dir, region, local,
-						 &mem_cfg);
 		}
 	}
 	if (rc) {
diff --git a/drivers/net/bnxt/tf_core/v3/tfo.c b/drivers/net/bnxt/tf_core/v3/tfo.c
index 681d1dd8d3..927b3a7640 100644
--- a/drivers/net/bnxt/tf_core/v3/tfo.c
+++ b/drivers/net/bnxt/tf_core/v3/tfo.c
@@ -36,6 +36,9 @@ struct tfc_global_object {
 	uint8_t gtsid;
 	struct tfc_tsid_db gtsid_db;
 	void *gts_tim;
+	rte_spinlock_t mem_cfg_lock; /**< serialises mem_cfg take/set across
+				       *  ports sharing this global scope
+				       */
 };
 
 struct tfc_global_object tfc_global;
@@ -58,6 +61,7 @@ struct tfc_object {
 	uint16_t sid; /**< Session ID */
 	bool is_pf; /**< port is a PF */
 	struct cfa_bld_mpcinfo mpc_info; /**< MPC ops handle */
+	rte_spinlock_t mem_cfg_lock; /**< serialises mem_cfg take/set */
 	struct tfc_tsid_db tsid_db[TFC_TBL_SCOPE_MAX]; /**< tsid database */
 	/** TIM instance pointer (PF) - this is where the 4 instances
 	 *  of the TPM (rx/tx_lkup, rx/tx_act) will be stored per shared
@@ -87,6 +91,7 @@ void tfo_open(void **tfo, bool is_pf)
 	tfco->is_pf = is_pf;
 	tfco->sid = INVALID_SID;
 	tfco->ts_tim = NULL;
+	rte_spinlock_init(&tfco->mem_cfg_lock);
 
 	/* Bind to the MPC builder */
 	rc = cfa_bld_mpc_bind(CFA_P70, &tfco->mpc_info);
@@ -408,6 +413,58 @@ int tfo_ts_get_mem_cfg(void *tfo, uint8_t ts_tsid, enum cfa_dir dir,
 	return rc;
 }
 
+/** Get and atomically clear the table scope memory configuration for this
+ *  direction
+ */
+int tfo_ts_get_and_clear_mem_cfg(void *tfo, uint8_t ts_tsid, enum cfa_dir dir,
+				  enum cfa_region_type region, bool *is_bs_owner,
+				  struct tfc_ts_mem_cfg *mem_cfg)
+{
+	struct tfc_ts_mem_cfg empty = { .num_lvl = 0 };
+	struct tfc_object *tfco = (struct tfc_object *)tfo;
+	struct tfc_global_object *tfgo;
+	struct tfc_tsid_db *tsid_db;
+	rte_spinlock_t *lock;
+	bool bs_owner;
+
+	if (tfo == NULL) {
+		PMD_DRV_LOG_LINE(ERR, "Invalid tfo pointer");
+		return -EINVAL;
+	}
+	if (tfco->signature != TFC_OBJ_SIGNATURE) {
+		PMD_DRV_LOG_LINE(ERR, "Invalid tfo object");
+		return -EINVAL;
+	}
+	if (mem_cfg == NULL) {
+		PMD_DRV_LOG_LINE(ERR, "Invalid mem_cfg pointer");
+		return -EINVAL;
+	}
+	if (ts_tsid >= TFC_TBL_SCOPE_MAX) {
+		PMD_DRV_LOG_LINE(ERR, "Invalid tsid %d", ts_tsid);
+		return -EINVAL;
+	}
+
+	tfgo = tfco->tfgo;
+	if (tfgo && tfgo->gtsid == ts_tsid) {
+		tsid_db = &tfgo->gtsid_db;
+		lock = &tfgo->mem_cfg_lock;
+	} else {
+		tsid_db = &tfco->tsid_db[ts_tsid];
+		lock = &tfco->mem_cfg_lock;
+	}
+
+	rte_spinlock_lock(lock);
+	*mem_cfg = tsid_db->ts_mem[region][dir];
+	tsid_db->ts_mem[region][dir] = empty;
+	bs_owner = tsid_db->ts_is_bs_owner;
+	rte_spinlock_unlock(lock);
+
+	if (is_bs_owner)
+		*is_bs_owner = bs_owner;
+
+	return 0;
+}
+
 /** Get the Pool Manager instance
  */
 int tfo_ts_get_cpm_inst(void *tfo, uint8_t ts_tsid, enum cfa_dir dir,
diff --git a/drivers/net/bnxt/tf_core/v3/tfo.h b/drivers/net/bnxt/tf_core/v3/tfo.h
index 93a6a5c064..a4c83a4ef4 100644
--- a/drivers/net/bnxt/tf_core/v3/tfo.h
+++ b/drivers/net/bnxt/tf_core/v3/tfo.h
@@ -268,6 +268,36 @@ int tfo_ts_get_mem_cfg(void *tfo, uint8_t ts_tsid, enum cfa_dir dir,
 		       enum cfa_region_type region, bool *is_bs_owner,
 		       struct tfc_ts_mem_cfg *mem_cfg);
 
+/**
+ * Get and atomically clear the table scope memory configuration.
+ *
+ * @param[in] tfo
+ *   Pointer to TFC object
+ *
+ * @param[in] ts_tsid
+ *   The table scope ID
+ *
+ * @param[in] dir
+ *   The direction (RX/TX)
+ *
+ * @param[in] region
+ *   The memory region type (lookup/action)
+ *
+ * @param[out] is_bs_owner
+ *   True if the caller is the owner of the backing store
+ *
+ * @param[out] mem_cfg
+ *   Receives the mem_cfg that was cleared from the database.
+ *   If a concurrent caller already claimed the slot,
+ *   mem_cfg->num_lvl will be 0 and the caller must skip the free.
+ *
+ * @return
+ *   0 for SUCCESS, negative error value for FAILURE (errno.h)
+ */
+int tfo_ts_get_and_clear_mem_cfg(void *tfo, uint8_t ts_tsid, enum cfa_dir dir,
+				  enum cfa_region_type region, bool *is_bs_owner,
+				  struct tfc_ts_mem_cfg *mem_cfg);
+
 /**
  * Set the pool memory configuration for this direction.
  *
-- 
2.31.1