From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
To: Huisong Li <lihuisong@huawei.com>
Cc: stable@dpdk.org, huangdaode@huawei.com, humin29@huawei.com
Subject: Re: [PATCH 19.11] net/hns3: fix insecure way to query MAC statistics
Date: Thu, 21 Apr 2022 07:14:01 +0200 [thread overview]
Message-ID: <CAATJJ0+gF0ykrxS7J_WMgM8s1oML14qPZD7km4V_9OU-U_zWFw@mail.gmail.com> (raw)
In-Reply-To: <20220322103334.12446-1-lihuisong@huawei.com>
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
prev parent reply other threads:[~2022-04-21 5:14 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-22 10:33 Huisong Li
2022-04-21 1:23 ` lihuisong (C)
2022-04-21 5:14 ` Christian Ehrhardt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAATJJ0+gF0ykrxS7J_WMgM8s1oML14qPZD7km4V_9OU-U_zWFw@mail.gmail.com \
--to=christian.ehrhardt@canonical.com \
--cc=huangdaode@huawei.com \
--cc=humin29@huawei.com \
--cc=lihuisong@huawei.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).