patches for DPDK stable branches
 help / color / mirror / Atom feed
* [PATCH 19.11] net/hns3: fix insecure way to query MAC statistics
@ 2022-03-22 10:33 Huisong Li
  2022-04-21  1:23 ` lihuisong (C)
  2022-04-21  5:14 ` Christian Ehrhardt
  0 siblings, 2 replies; 3+ messages in thread
From: Huisong Li @ 2022-03-22 10:33 UTC (permalink / raw)
  To: stable, christian.ehrhardt; +Cc: lihuisong, huangdaode, humin29

[ upstream commit 6ee07e3cb589145d0f0826f918fd44eb3e160a08 ]

The query way of MAC statistics in HNS3 PF driver is as following:
1) get MAC statistics register number and calculate descriptor number.
2) use above descriptor number to send command to firmware to query all
   MAC statistics and copy to hns3_mac_stats struct in driver.

The preceding way does not verify the validity of the number of obtained
register, which may cause memory out-of-bounds.

Fixes: 8839c5e202f3 ("net/hns3: support device stats")

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/hns3/hns3_ethdev.c |   4 ++
 drivers/net/hns3/hns3_ethdev.h |   1 +
 drivers/net/hns3/hns3_stats.c  | 116 ++++++++++++++++-----------------
 drivers/net/hns3/hns3_stats.h  |  10 ++-
 4 files changed, 65 insertions(+), 66 deletions(-)

diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index 157dd34d4f..bbf671cc15 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -2930,6 +2930,10 @@ hns3_get_configuration(struct hns3_hw *hw)
 		return ret;
 	}
 
+	ret = hns3_query_mac_stats_reg_num(hw);
+	if (ret)
+		return ret;
+
 	/* Get pf resource */
 	ret = hns3_query_pf_resource(hw);
 	if (ret) {
diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
index 6c3ac6f8a9..fe4a189189 100644
--- a/drivers/net/hns3/hns3_ethdev.h
+++ b/drivers/net/hns3/hns3_ethdev.h
@@ -362,6 +362,7 @@ struct hns3_hw {
 	struct hns3_tqp_stats tqp_stats;
 	/* Include Mac stats | Rx stats | Tx stats */
 	struct hns3_mac_stats mac_stats;
+	uint32_t mac_stats_reg_num;
 	uint32_t fw_version;
 
 	uint16_t num_msi;
diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
index 3dff75c730..97ca66e735 100644
--- a/drivers/net/hns3/hns3_stats.c
+++ b/drivers/net/hns3/hns3_stats.c
@@ -248,24 +248,21 @@ static const struct hns3_xstats_name_offset hns3_rx_bd_error_strings[] = {
 #define HNS3_FIX_NUM_STATS (HNS3_NUM_MAC_STATS + HNS3_NUM_ERROR_INT_XSTATS + \
 			    HNS3_NUM_RESET_XSTATS)
 
-/*
- * Query all the MAC statistics data of Network ICL command ,opcode id: 0x0034.
- * This command is used before send 'query_mac_stat command', the descriptor
- * number of 'query_mac_stat command' must match with reg_num in this command.
- * @praram hw
- *   Pointer to structure hns3_hw.
- * @return
- *   0 on success.
- */
 static int
-hns3_update_mac_stats(struct hns3_hw *hw, const uint32_t desc_num)
+hns3_update_mac_stats(struct hns3_hw *hw)
 {
+#define HNS3_MAC_STATS_REG_NUM_PER_DESC	4
+
 	uint64_t *data = (uint64_t *)(&hw->mac_stats);
 	struct hns3_cmd_desc *desc;
+	uint32_t stats_iterms;
 	uint64_t *desc_data;
-	uint16_t i, k, n;
+	uint32_t desc_num;
+	uint16_t i;
 	int ret;
 
+	/* The first desc has a 64-bit header, so need to consider it. */
+	desc_num = hw->mac_stats_reg_num / HNS3_MAC_STATS_REG_NUM_PER_DESC + 1;
 	desc = rte_malloc("hns3_mac_desc",
 			  desc_num * sizeof(struct hns3_cmd_desc), 0);
 	if (desc == NULL) {
@@ -281,65 +278,71 @@ hns3_update_mac_stats(struct hns3_hw *hw, const uint32_t desc_num)
 		return ret;
 	}
 
-	for (i = 0; i < desc_num; i++) {
-		/* For special opcode 0034, only the first desc has the head */
-		if (i == 0) {
-			desc_data = (uint64_t *)(&desc[i].data[0]);
-			n = HNS3_RD_FIRST_STATS_NUM;
-		} else {
-			desc_data = (uint64_t *)(&desc[i]);
-			n = HNS3_RD_OTHER_STATS_NUM;
-		}
-
-		for (k = 0; k < n; k++) {
-			*data += rte_le_to_cpu_64(*desc_data);
-			data++;
-			desc_data++;
-		}
+	stats_iterms = RTE_MIN(sizeof(hw->mac_stats) / sizeof(uint64_t),
+			       hw->mac_stats_reg_num);
+	desc_data = (uint64_t *)(&desc[0].data[0]);
+	for (i = 0; i < stats_iterms; i++) {
+		/*
+		 * Data memory is continuous and only the first descriptor has a
+		 * header in this command.
+		 */
+		*data += rte_le_to_cpu_64(*desc_data);
+		data++;
+		desc_data++;
 	}
 	rte_free(desc);
 
 	return 0;
 }
 
-/*
- * Query Mac stat reg num command ,opcode id: 0x0033.
- * This command is used before send 'query_mac_stat command', the descriptor
- * number of 'query_mac_stat command' must match with reg_num in this command.
- * @praram rte_stats
- *   Pointer to structure rte_eth_stats.
- * @return
- *   0 on success.
- */
 static int
-hns3_mac_query_reg_num(struct rte_eth_dev *dev, uint32_t *desc_num)
+hns3_mac_query_reg_num(struct hns3_hw *hw, uint32_t *reg_num)
 {
-	struct hns3_adapter *hns = dev->data->dev_private;
-	struct hns3_hw *hw = &hns->hw;
+#define HNS3_MAC_STATS_RSV_REG_NUM_ON_HIP08_B	3
 	struct hns3_cmd_desc desc;
-	uint32_t *desc_data;
-	uint32_t reg_num;
 	int ret;
 
 	hns3_cmd_setup_basic_desc(&desc, HNS3_OPC_QUERY_MAC_REG_NUM, true);
 	ret = hns3_cmd_send(hw, &desc, 1);
-	if (ret)
+	if (ret) {
+		hns3_err(hw, "failed to query MAC statistic reg number, ret = %d",
+			 ret);
 		return ret;
+	}
 
-	/*
-	 * The num of MAC statistics registers that are provided by IMP in this
-	 * version.
-	 */
-	desc_data = (uint32_t *)(&desc.data[0]);
-	reg_num = rte_le_to_cpu_32(*desc_data);
+	/* The number of MAC statistics registers are provided by firmware. */
+	*reg_num = rte_le_to_cpu_32(desc.data[0]);
+	if (*reg_num == 0) {
+		hns3_err(hw, "MAC statistic reg number is invalid!");
+		return -ENODATA;
+	}
 
 	/*
-	 * The descriptor number of 'query_additional_mac_stat command' is
-	 * '1 + (reg_num-3)/4 + ((reg_num-3)%4 !=0)';
-	 * This value is 83 in this version
+	 * If driver doesn't request the firmware to report more MAC statistics
+	 * iterms and the total number of MAC statistics registers by using new
+	 * method, firmware will only reports the number of valid statistics
+	 * registers. However, structure hns3_mac_stats in driver contains valid
+	 * and reserved statistics iterms. In this case, the total register
+	 * number must be added to three reserved statistics registers.
 	 */
-	*desc_num = 1 + ((reg_num - 3) >> 2) +
-		    (uint32_t)(((reg_num - 3) & 0x3) ? 1 : 0);
+	*reg_num += HNS3_MAC_STATS_RSV_REG_NUM_ON_HIP08_B;
+
+	return 0;
+}
+
+int
+hns3_query_mac_stats_reg_num(struct hns3_hw *hw)
+{
+	uint32_t mac_stats_reg_num = 0;
+	int ret;
+
+	ret = hns3_mac_query_reg_num(hw, &mac_stats_reg_num);
+	if (ret)
+		return ret;
+
+	hw->mac_stats_reg_num = mac_stats_reg_num;
+	if (hw->mac_stats_reg_num > sizeof(hw->mac_stats) / sizeof(uint64_t))
+		hns3_warn(hw, "MAC stats reg number from firmware is greater than stats iterms in driver.");
 
 	return 0;
 }
@@ -349,15 +352,8 @@ hns3_query_update_mac_stats(struct rte_eth_dev *dev)
 {
 	struct hns3_adapter *hns = dev->data->dev_private;
 	struct hns3_hw *hw = &hns->hw;
-	uint32_t desc_num;
-	int ret;
 
-	ret = hns3_mac_query_reg_num(dev, &desc_num);
-	if (ret == 0)
-		ret = hns3_update_mac_stats(hw, desc_num);
-	else
-		hns3_err(hw, "Query mac reg num fail : %d", ret);
-	return ret;
+	return hns3_update_mac_stats(hw);
 }
 
 /* Get tqp stats from register */
diff --git a/drivers/net/hns3/hns3_stats.h b/drivers/net/hns3/hns3_stats.h
index c51e835cde..be51bbca8b 100644
--- a/drivers/net/hns3/hns3_stats.h
+++ b/drivers/net/hns3/hns3_stats.h
@@ -5,10 +5,6 @@
 #ifndef _HNS3_STATS_H_
 #define _HNS3_STATS_H_
 
-/* stats macro */
-#define HNS3_MAC_CMD_NUM		21
-#define HNS3_RD_FIRST_STATS_NUM		2
-#define HNS3_RD_OTHER_STATS_NUM		4
 #define HNS3_VALUES_BYTES		8
 
 /* TQP stats */
@@ -23,6 +19,7 @@ struct hns3_tqp_stats {
 struct hns3_mac_stats {
 	uint64_t mac_tx_mac_pause_num;
 	uint64_t mac_rx_mac_pause_num;
+	uint64_t rsv0;
 	uint64_t mac_tx_pfc_pri0_pkt_num;
 	uint64_t mac_tx_pfc_pri1_pkt_num;
 	uint64_t mac_tx_pfc_pri2_pkt_num;
@@ -59,7 +56,7 @@ struct hns3_mac_stats {
 	uint64_t mac_tx_1519_2047_oct_pkt_num;
 	uint64_t mac_tx_2048_4095_oct_pkt_num;
 	uint64_t mac_tx_4096_8191_oct_pkt_num;
-	uint64_t rsv0;
+	uint64_t rsv1;
 	uint64_t mac_tx_8192_9216_oct_pkt_num;
 	uint64_t mac_tx_9217_12287_oct_pkt_num;
 	uint64_t mac_tx_12288_16383_oct_pkt_num;
@@ -86,7 +83,7 @@ struct hns3_mac_stats {
 	uint64_t mac_rx_1519_2047_oct_pkt_num;
 	uint64_t mac_rx_2048_4095_oct_pkt_num;
 	uint64_t mac_rx_4096_8191_oct_pkt_num;
-	uint64_t rsv1;
+	uint64_t rsv2;
 	uint64_t mac_rx_8192_9216_oct_pkt_num;
 	uint64_t mac_rx_9217_12287_oct_pkt_num;
 	uint64_t mac_rx_12288_16383_oct_pkt_num;
@@ -149,4 +146,5 @@ int hns3_dev_xstats_get_names_by_id(struct rte_eth_dev *dev,
 				    const uint64_t *ids,
 				    uint32_t size);
 int hns3_stats_reset(struct rte_eth_dev *dev);
+int hns3_query_mac_stats_reg_num(struct hns3_hw *hw);
 #endif /* _HNS3_STATS_H_ */
-- 
2.33.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 19.11] net/hns3: fix insecure way to query MAC statistics
  2022-03-22 10:33 [PATCH 19.11] net/hns3: fix insecure way to query MAC statistics Huisong Li
@ 2022-04-21  1:23 ` lihuisong (C)
  2022-04-21  5:14 ` Christian Ehrhardt
  1 sibling, 0 replies; 3+ messages in thread
From: lihuisong (C) @ 2022-04-21  1:23 UTC (permalink / raw)
  To: stable, christian.ehrhardt, Kevin Traynor; +Cc: huangdaode, humin29

Hi, Kevin and Christian,

Can you help us merge this patch to 19.11.13?

在 2022/3/22 18:33, Huisong Li 写道:
> [ upstream commit 6ee07e3cb589145d0f0826f918fd44eb3e160a08 ]
>
> The query way of MAC statistics in HNS3 PF driver is as following:
> 1) get MAC statistics register number and calculate descriptor number.
> 2) use above descriptor number to send command to firmware to query all
>     MAC statistics and copy to hns3_mac_stats struct in driver.
>
> The preceding way does not verify the validity of the number of obtained
> register, which may cause memory out-of-bounds.
>
> Fixes: 8839c5e202f3 ("net/hns3: support device stats")
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   drivers/net/hns3/hns3_ethdev.c |   4 ++
>   drivers/net/hns3/hns3_ethdev.h |   1 +
>   drivers/net/hns3/hns3_stats.c  | 116 ++++++++++++++++-----------------
>   drivers/net/hns3/hns3_stats.h  |  10 ++-
>   4 files changed, 65 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
> index 157dd34d4f..bbf671cc15 100644
> --- a/drivers/net/hns3/hns3_ethdev.c
> +++ b/drivers/net/hns3/hns3_ethdev.c
> @@ -2930,6 +2930,10 @@ hns3_get_configuration(struct hns3_hw *hw)
>   		return ret;
>   	}
>   
> +	ret = hns3_query_mac_stats_reg_num(hw);
> +	if (ret)
> +		return ret;
> +
>   	/* Get pf resource */
>   	ret = hns3_query_pf_resource(hw);
>   	if (ret) {
> diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
> index 6c3ac6f8a9..fe4a189189 100644
> --- a/drivers/net/hns3/hns3_ethdev.h
> +++ b/drivers/net/hns3/hns3_ethdev.h
> @@ -362,6 +362,7 @@ struct hns3_hw {
>   	struct hns3_tqp_stats tqp_stats;
>   	/* Include Mac stats | Rx stats | Tx stats */
>   	struct hns3_mac_stats mac_stats;
> +	uint32_t mac_stats_reg_num;
>   	uint32_t fw_version;
>   
>   	uint16_t num_msi;
> diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
> index 3dff75c730..97ca66e735 100644
> --- a/drivers/net/hns3/hns3_stats.c
> +++ b/drivers/net/hns3/hns3_stats.c
> @@ -248,24 +248,21 @@ static const struct hns3_xstats_name_offset hns3_rx_bd_error_strings[] = {
>   #define HNS3_FIX_NUM_STATS (HNS3_NUM_MAC_STATS + HNS3_NUM_ERROR_INT_XSTATS + \
>   			    HNS3_NUM_RESET_XSTATS)
>   
> -/*
> - * Query all the MAC statistics data of Network ICL command ,opcode id: 0x0034.
> - * This command is used before send 'query_mac_stat command', the descriptor
> - * number of 'query_mac_stat command' must match with reg_num in this command.
> - * @praram hw
> - *   Pointer to structure hns3_hw.
> - * @return
> - *   0 on success.
> - */
>   static int
> -hns3_update_mac_stats(struct hns3_hw *hw, const uint32_t desc_num)
> +hns3_update_mac_stats(struct hns3_hw *hw)
>   {
> +#define HNS3_MAC_STATS_REG_NUM_PER_DESC	4
> +
>   	uint64_t *data = (uint64_t *)(&hw->mac_stats);
>   	struct hns3_cmd_desc *desc;
> +	uint32_t stats_iterms;
>   	uint64_t *desc_data;
> -	uint16_t i, k, n;
> +	uint32_t desc_num;
> +	uint16_t i;
>   	int ret;
>   
> +	/* The first desc has a 64-bit header, so need to consider it. */
> +	desc_num = hw->mac_stats_reg_num / HNS3_MAC_STATS_REG_NUM_PER_DESC + 1;
>   	desc = rte_malloc("hns3_mac_desc",
>   			  desc_num * sizeof(struct hns3_cmd_desc), 0);
>   	if (desc == NULL) {
> @@ -281,65 +278,71 @@ hns3_update_mac_stats(struct hns3_hw *hw, const uint32_t desc_num)
>   		return ret;
>   	}
>   
> -	for (i = 0; i < desc_num; i++) {
> -		/* For special opcode 0034, only the first desc has the head */
> -		if (i == 0) {
> -			desc_data = (uint64_t *)(&desc[i].data[0]);
> -			n = HNS3_RD_FIRST_STATS_NUM;
> -		} else {
> -			desc_data = (uint64_t *)(&desc[i]);
> -			n = HNS3_RD_OTHER_STATS_NUM;
> -		}
> -
> -		for (k = 0; k < n; k++) {
> -			*data += rte_le_to_cpu_64(*desc_data);
> -			data++;
> -			desc_data++;
> -		}
> +	stats_iterms = RTE_MIN(sizeof(hw->mac_stats) / sizeof(uint64_t),
> +			       hw->mac_stats_reg_num);
> +	desc_data = (uint64_t *)(&desc[0].data[0]);
> +	for (i = 0; i < stats_iterms; i++) {
> +		/*
> +		 * Data memory is continuous and only the first descriptor has a
> +		 * header in this command.
> +		 */
> +		*data += rte_le_to_cpu_64(*desc_data);
> +		data++;
> +		desc_data++;
>   	}
>   	rte_free(desc);
>   
>   	return 0;
>   }
>   
> -/*
> - * Query Mac stat reg num command ,opcode id: 0x0033.
> - * This command is used before send 'query_mac_stat command', the descriptor
> - * number of 'query_mac_stat command' must match with reg_num in this command.
> - * @praram rte_stats
> - *   Pointer to structure rte_eth_stats.
> - * @return
> - *   0 on success.
> - */
>   static int
> -hns3_mac_query_reg_num(struct rte_eth_dev *dev, uint32_t *desc_num)
> +hns3_mac_query_reg_num(struct hns3_hw *hw, uint32_t *reg_num)
>   {
> -	struct hns3_adapter *hns = dev->data->dev_private;
> -	struct hns3_hw *hw = &hns->hw;
> +#define HNS3_MAC_STATS_RSV_REG_NUM_ON_HIP08_B	3
>   	struct hns3_cmd_desc desc;
> -	uint32_t *desc_data;
> -	uint32_t reg_num;
>   	int ret;
>   
>   	hns3_cmd_setup_basic_desc(&desc, HNS3_OPC_QUERY_MAC_REG_NUM, true);
>   	ret = hns3_cmd_send(hw, &desc, 1);
> -	if (ret)
> +	if (ret) {
> +		hns3_err(hw, "failed to query MAC statistic reg number, ret = %d",
> +			 ret);
>   		return ret;
> +	}
>   
> -	/*
> -	 * The num of MAC statistics registers that are provided by IMP in this
> -	 * version.
> -	 */
> -	desc_data = (uint32_t *)(&desc.data[0]);
> -	reg_num = rte_le_to_cpu_32(*desc_data);
> +	/* The number of MAC statistics registers are provided by firmware. */
> +	*reg_num = rte_le_to_cpu_32(desc.data[0]);
> +	if (*reg_num == 0) {
> +		hns3_err(hw, "MAC statistic reg number is invalid!");
> +		return -ENODATA;
> +	}
>   
>   	/*
> -	 * The descriptor number of 'query_additional_mac_stat command' is
> -	 * '1 + (reg_num-3)/4 + ((reg_num-3)%4 !=0)';
> -	 * This value is 83 in this version
> +	 * If driver doesn't request the firmware to report more MAC statistics
> +	 * iterms and the total number of MAC statistics registers by using new
> +	 * method, firmware will only reports the number of valid statistics
> +	 * registers. However, structure hns3_mac_stats in driver contains valid
> +	 * and reserved statistics iterms. In this case, the total register
> +	 * number must be added to three reserved statistics registers.
>   	 */
> -	*desc_num = 1 + ((reg_num - 3) >> 2) +
> -		    (uint32_t)(((reg_num - 3) & 0x3) ? 1 : 0);
> +	*reg_num += HNS3_MAC_STATS_RSV_REG_NUM_ON_HIP08_B;
> +
> +	return 0;
> +}
> +
> +int
> +hns3_query_mac_stats_reg_num(struct hns3_hw *hw)
> +{
> +	uint32_t mac_stats_reg_num = 0;
> +	int ret;
> +
> +	ret = hns3_mac_query_reg_num(hw, &mac_stats_reg_num);
> +	if (ret)
> +		return ret;
> +
> +	hw->mac_stats_reg_num = mac_stats_reg_num;
> +	if (hw->mac_stats_reg_num > sizeof(hw->mac_stats) / sizeof(uint64_t))
> +		hns3_warn(hw, "MAC stats reg number from firmware is greater than stats iterms in driver.");
>   
>   	return 0;
>   }
> @@ -349,15 +352,8 @@ hns3_query_update_mac_stats(struct rte_eth_dev *dev)
>   {
>   	struct hns3_adapter *hns = dev->data->dev_private;
>   	struct hns3_hw *hw = &hns->hw;
> -	uint32_t desc_num;
> -	int ret;
>   
> -	ret = hns3_mac_query_reg_num(dev, &desc_num);
> -	if (ret == 0)
> -		ret = hns3_update_mac_stats(hw, desc_num);
> -	else
> -		hns3_err(hw, "Query mac reg num fail : %d", ret);
> -	return ret;
> +	return hns3_update_mac_stats(hw);
>   }
>   
>   /* Get tqp stats from register */
> diff --git a/drivers/net/hns3/hns3_stats.h b/drivers/net/hns3/hns3_stats.h
> index c51e835cde..be51bbca8b 100644
> --- a/drivers/net/hns3/hns3_stats.h
> +++ b/drivers/net/hns3/hns3_stats.h
> @@ -5,10 +5,6 @@
>   #ifndef _HNS3_STATS_H_
>   #define _HNS3_STATS_H_
>   
> -/* stats macro */
> -#define HNS3_MAC_CMD_NUM		21
> -#define HNS3_RD_FIRST_STATS_NUM		2
> -#define HNS3_RD_OTHER_STATS_NUM		4
>   #define HNS3_VALUES_BYTES		8
>   
>   /* TQP stats */
> @@ -23,6 +19,7 @@ struct hns3_tqp_stats {
>   struct hns3_mac_stats {
>   	uint64_t mac_tx_mac_pause_num;
>   	uint64_t mac_rx_mac_pause_num;
> +	uint64_t rsv0;
>   	uint64_t mac_tx_pfc_pri0_pkt_num;
>   	uint64_t mac_tx_pfc_pri1_pkt_num;
>   	uint64_t mac_tx_pfc_pri2_pkt_num;
> @@ -59,7 +56,7 @@ struct hns3_mac_stats {
>   	uint64_t mac_tx_1519_2047_oct_pkt_num;
>   	uint64_t mac_tx_2048_4095_oct_pkt_num;
>   	uint64_t mac_tx_4096_8191_oct_pkt_num;
> -	uint64_t rsv0;
> +	uint64_t rsv1;
>   	uint64_t mac_tx_8192_9216_oct_pkt_num;
>   	uint64_t mac_tx_9217_12287_oct_pkt_num;
>   	uint64_t mac_tx_12288_16383_oct_pkt_num;
> @@ -86,7 +83,7 @@ struct hns3_mac_stats {
>   	uint64_t mac_rx_1519_2047_oct_pkt_num;
>   	uint64_t mac_rx_2048_4095_oct_pkt_num;
>   	uint64_t mac_rx_4096_8191_oct_pkt_num;
> -	uint64_t rsv1;
> +	uint64_t rsv2;
>   	uint64_t mac_rx_8192_9216_oct_pkt_num;
>   	uint64_t mac_rx_9217_12287_oct_pkt_num;
>   	uint64_t mac_rx_12288_16383_oct_pkt_num;
> @@ -149,4 +146,5 @@ int hns3_dev_xstats_get_names_by_id(struct rte_eth_dev *dev,
>   				    const uint64_t *ids,
>   				    uint32_t size);
>   int hns3_stats_reset(struct rte_eth_dev *dev);
> +int hns3_query_mac_stats_reg_num(struct hns3_hw *hw);
>   #endif /* _HNS3_STATS_H_ */

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 19.11] net/hns3: fix insecure way to query MAC statistics
  2022-03-22 10:33 [PATCH 19.11] net/hns3: fix insecure way to query MAC statistics Huisong Li
  2022-04-21  1:23 ` lihuisong (C)
@ 2022-04-21  5:14 ` Christian Ehrhardt
  1 sibling, 0 replies; 3+ messages in thread
From: Christian Ehrhardt @ 2022-04-21  5:14 UTC (permalink / raw)
  To: Huisong Li; +Cc: stable, huangdaode, humin29

On Tue, Mar 22, 2022 at 11:34 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> [ upstream commit 6ee07e3cb589145d0f0826f918fd44eb3e160a08 ]

Hi,
thanks for the backport.
Queued for 19.11.13

> The query way of MAC statistics in HNS3 PF driver is as following:
> 1) get MAC statistics register number and calculate descriptor number.
> 2) use above descriptor number to send command to firmware to query all
>    MAC statistics and copy to hns3_mac_stats struct in driver.
>
> The preceding way does not verify the validity of the number of obtained
> register, which may cause memory out-of-bounds.
>
> Fixes: 8839c5e202f3 ("net/hns3: support device stats")
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/hns3/hns3_ethdev.c |   4 ++
>  drivers/net/hns3/hns3_ethdev.h |   1 +
>  drivers/net/hns3/hns3_stats.c  | 116 ++++++++++++++++-----------------
>  drivers/net/hns3/hns3_stats.h  |  10 ++-
>  4 files changed, 65 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
> index 157dd34d4f..bbf671cc15 100644
> --- a/drivers/net/hns3/hns3_ethdev.c
> +++ b/drivers/net/hns3/hns3_ethdev.c
> @@ -2930,6 +2930,10 @@ hns3_get_configuration(struct hns3_hw *hw)
>                 return ret;
>         }
>
> +       ret = hns3_query_mac_stats_reg_num(hw);
> +       if (ret)
> +               return ret;
> +
>         /* Get pf resource */
>         ret = hns3_query_pf_resource(hw);
>         if (ret) {
> diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
> index 6c3ac6f8a9..fe4a189189 100644
> --- a/drivers/net/hns3/hns3_ethdev.h
> +++ b/drivers/net/hns3/hns3_ethdev.h
> @@ -362,6 +362,7 @@ struct hns3_hw {
>         struct hns3_tqp_stats tqp_stats;
>         /* Include Mac stats | Rx stats | Tx stats */
>         struct hns3_mac_stats mac_stats;
> +       uint32_t mac_stats_reg_num;
>         uint32_t fw_version;
>
>         uint16_t num_msi;
> diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
> index 3dff75c730..97ca66e735 100644
> --- a/drivers/net/hns3/hns3_stats.c
> +++ b/drivers/net/hns3/hns3_stats.c
> @@ -248,24 +248,21 @@ static const struct hns3_xstats_name_offset hns3_rx_bd_error_strings[] = {
>  #define HNS3_FIX_NUM_STATS (HNS3_NUM_MAC_STATS + HNS3_NUM_ERROR_INT_XSTATS + \
>                             HNS3_NUM_RESET_XSTATS)
>
> -/*
> - * Query all the MAC statistics data of Network ICL command ,opcode id: 0x0034.
> - * This command is used before send 'query_mac_stat command', the descriptor
> - * number of 'query_mac_stat command' must match with reg_num in this command.
> - * @praram hw
> - *   Pointer to structure hns3_hw.
> - * @return
> - *   0 on success.
> - */
>  static int
> -hns3_update_mac_stats(struct hns3_hw *hw, const uint32_t desc_num)
> +hns3_update_mac_stats(struct hns3_hw *hw)
>  {
> +#define HNS3_MAC_STATS_REG_NUM_PER_DESC        4
> +
>         uint64_t *data = (uint64_t *)(&hw->mac_stats);
>         struct hns3_cmd_desc *desc;
> +       uint32_t stats_iterms;
>         uint64_t *desc_data;
> -       uint16_t i, k, n;
> +       uint32_t desc_num;
> +       uint16_t i;
>         int ret;
>
> +       /* The first desc has a 64-bit header, so need to consider it. */
> +       desc_num = hw->mac_stats_reg_num / HNS3_MAC_STATS_REG_NUM_PER_DESC + 1;
>         desc = rte_malloc("hns3_mac_desc",
>                           desc_num * sizeof(struct hns3_cmd_desc), 0);
>         if (desc == NULL) {
> @@ -281,65 +278,71 @@ hns3_update_mac_stats(struct hns3_hw *hw, const uint32_t desc_num)
>                 return ret;
>         }
>
> -       for (i = 0; i < desc_num; i++) {
> -               /* For special opcode 0034, only the first desc has the head */
> -               if (i == 0) {
> -                       desc_data = (uint64_t *)(&desc[i].data[0]);
> -                       n = HNS3_RD_FIRST_STATS_NUM;
> -               } else {
> -                       desc_data = (uint64_t *)(&desc[i]);
> -                       n = HNS3_RD_OTHER_STATS_NUM;
> -               }
> -
> -               for (k = 0; k < n; k++) {
> -                       *data += rte_le_to_cpu_64(*desc_data);
> -                       data++;
> -                       desc_data++;
> -               }
> +       stats_iterms = RTE_MIN(sizeof(hw->mac_stats) / sizeof(uint64_t),
> +                              hw->mac_stats_reg_num);
> +       desc_data = (uint64_t *)(&desc[0].data[0]);
> +       for (i = 0; i < stats_iterms; i++) {
> +               /*
> +                * Data memory is continuous and only the first descriptor has a
> +                * header in this command.
> +                */
> +               *data += rte_le_to_cpu_64(*desc_data);
> +               data++;
> +               desc_data++;
>         }
>         rte_free(desc);
>
>         return 0;
>  }
>
> -/*
> - * Query Mac stat reg num command ,opcode id: 0x0033.
> - * This command is used before send 'query_mac_stat command', the descriptor
> - * number of 'query_mac_stat command' must match with reg_num in this command.
> - * @praram rte_stats
> - *   Pointer to structure rte_eth_stats.
> - * @return
> - *   0 on success.
> - */
>  static int
> -hns3_mac_query_reg_num(struct rte_eth_dev *dev, uint32_t *desc_num)
> +hns3_mac_query_reg_num(struct hns3_hw *hw, uint32_t *reg_num)
>  {
> -       struct hns3_adapter *hns = dev->data->dev_private;
> -       struct hns3_hw *hw = &hns->hw;
> +#define HNS3_MAC_STATS_RSV_REG_NUM_ON_HIP08_B  3
>         struct hns3_cmd_desc desc;
> -       uint32_t *desc_data;
> -       uint32_t reg_num;
>         int ret;
>
>         hns3_cmd_setup_basic_desc(&desc, HNS3_OPC_QUERY_MAC_REG_NUM, true);
>         ret = hns3_cmd_send(hw, &desc, 1);
> -       if (ret)
> +       if (ret) {
> +               hns3_err(hw, "failed to query MAC statistic reg number, ret = %d",
> +                        ret);
>                 return ret;
> +       }
>
> -       /*
> -        * The num of MAC statistics registers that are provided by IMP in this
> -        * version.
> -        */
> -       desc_data = (uint32_t *)(&desc.data[0]);
> -       reg_num = rte_le_to_cpu_32(*desc_data);
> +       /* The number of MAC statistics registers are provided by firmware. */
> +       *reg_num = rte_le_to_cpu_32(desc.data[0]);
> +       if (*reg_num == 0) {
> +               hns3_err(hw, "MAC statistic reg number is invalid!");
> +               return -ENODATA;
> +       }
>
>         /*
> -        * The descriptor number of 'query_additional_mac_stat command' is
> -        * '1 + (reg_num-3)/4 + ((reg_num-3)%4 !=0)';
> -        * This value is 83 in this version
> +        * If driver doesn't request the firmware to report more MAC statistics
> +        * iterms and the total number of MAC statistics registers by using new
> +        * method, firmware will only reports the number of valid statistics
> +        * registers. However, structure hns3_mac_stats in driver contains valid
> +        * and reserved statistics iterms. In this case, the total register
> +        * number must be added to three reserved statistics registers.
>          */
> -       *desc_num = 1 + ((reg_num - 3) >> 2) +
> -                   (uint32_t)(((reg_num - 3) & 0x3) ? 1 : 0);
> +       *reg_num += HNS3_MAC_STATS_RSV_REG_NUM_ON_HIP08_B;
> +
> +       return 0;
> +}
> +
> +int
> +hns3_query_mac_stats_reg_num(struct hns3_hw *hw)
> +{
> +       uint32_t mac_stats_reg_num = 0;
> +       int ret;
> +
> +       ret = hns3_mac_query_reg_num(hw, &mac_stats_reg_num);
> +       if (ret)
> +               return ret;
> +
> +       hw->mac_stats_reg_num = mac_stats_reg_num;
> +       if (hw->mac_stats_reg_num > sizeof(hw->mac_stats) / sizeof(uint64_t))
> +               hns3_warn(hw, "MAC stats reg number from firmware is greater than stats iterms in driver.");
>
>         return 0;
>  }
> @@ -349,15 +352,8 @@ hns3_query_update_mac_stats(struct rte_eth_dev *dev)
>  {
>         struct hns3_adapter *hns = dev->data->dev_private;
>         struct hns3_hw *hw = &hns->hw;
> -       uint32_t desc_num;
> -       int ret;
>
> -       ret = hns3_mac_query_reg_num(dev, &desc_num);
> -       if (ret == 0)
> -               ret = hns3_update_mac_stats(hw, desc_num);
> -       else
> -               hns3_err(hw, "Query mac reg num fail : %d", ret);
> -       return ret;
> +       return hns3_update_mac_stats(hw);
>  }
>
>  /* Get tqp stats from register */
> diff --git a/drivers/net/hns3/hns3_stats.h b/drivers/net/hns3/hns3_stats.h
> index c51e835cde..be51bbca8b 100644
> --- a/drivers/net/hns3/hns3_stats.h
> +++ b/drivers/net/hns3/hns3_stats.h
> @@ -5,10 +5,6 @@
>  #ifndef _HNS3_STATS_H_
>  #define _HNS3_STATS_H_
>
> -/* stats macro */
> -#define HNS3_MAC_CMD_NUM               21
> -#define HNS3_RD_FIRST_STATS_NUM                2
> -#define HNS3_RD_OTHER_STATS_NUM                4
>  #define HNS3_VALUES_BYTES              8
>
>  /* TQP stats */
> @@ -23,6 +19,7 @@ struct hns3_tqp_stats {
>  struct hns3_mac_stats {
>         uint64_t mac_tx_mac_pause_num;
>         uint64_t mac_rx_mac_pause_num;
> +       uint64_t rsv0;
>         uint64_t mac_tx_pfc_pri0_pkt_num;
>         uint64_t mac_tx_pfc_pri1_pkt_num;
>         uint64_t mac_tx_pfc_pri2_pkt_num;
> @@ -59,7 +56,7 @@ struct hns3_mac_stats {
>         uint64_t mac_tx_1519_2047_oct_pkt_num;
>         uint64_t mac_tx_2048_4095_oct_pkt_num;
>         uint64_t mac_tx_4096_8191_oct_pkt_num;
> -       uint64_t rsv0;
> +       uint64_t rsv1;
>         uint64_t mac_tx_8192_9216_oct_pkt_num;
>         uint64_t mac_tx_9217_12287_oct_pkt_num;
>         uint64_t mac_tx_12288_16383_oct_pkt_num;
> @@ -86,7 +83,7 @@ struct hns3_mac_stats {
>         uint64_t mac_rx_1519_2047_oct_pkt_num;
>         uint64_t mac_rx_2048_4095_oct_pkt_num;
>         uint64_t mac_rx_4096_8191_oct_pkt_num;
> -       uint64_t rsv1;
> +       uint64_t rsv2;
>         uint64_t mac_rx_8192_9216_oct_pkt_num;
>         uint64_t mac_rx_9217_12287_oct_pkt_num;
>         uint64_t mac_rx_12288_16383_oct_pkt_num;
> @@ -149,4 +146,5 @@ int hns3_dev_xstats_get_names_by_id(struct rte_eth_dev *dev,
>                                     const uint64_t *ids,
>                                     uint32_t size);
>  int hns3_stats_reset(struct rte_eth_dev *dev);
> +int hns3_query_mac_stats_reg_num(struct hns3_hw *hw);
>  #endif /* _HNS3_STATS_H_ */
> --
> 2.33.0
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-04-21  5:14 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-22 10:33 [PATCH 19.11] net/hns3: fix insecure way to query MAC statistics Huisong Li
2022-04-21  1:23 ` lihuisong (C)
2022-04-21  5:14 ` Christian Ehrhardt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).