From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 97102A00C3 for ; Thu, 21 Apr 2022 07:14:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7325A4068E; Thu, 21 Apr 2022 07:14:30 +0200 (CEST) Received: from smtp-relay-internal-0.canonical.com (smtp-relay-internal-0.canonical.com [185.125.188.122]) by mails.dpdk.org (Postfix) with ESMTP id 208594068E for ; Thu, 21 Apr 2022 07:14:29 +0200 (CEST) Received: from mail-oo1-f71.google.com (mail-oo1-f71.google.com [209.85.161.71]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 739B73F1A1 for ; Thu, 21 Apr 2022 05:14:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1650518068; bh=aroICBY1YQJ2/vHc4fu0o/SN0T9U5LUHOpX0JH0OBAI=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=AvjOZ4a+wTBKANP9Ooa4ZGRKaX01zdR/TNwsBdEt4mXTkc4WiXUI8w9M+TO3JybEh EPK2vLiLtbHlnXQxW4rUiLByW2HYfCTpOEcc/9KALvW/XLsIbD+oQBjTSyU4EnqZnX RvnVsCuVNIKadUTtwmJhlZdFtrGNP/vTh3pkirgfTtVELyfqV/6al2O4EAY5Lwu1UB PO8voIvYJ2u8+ot/OiWJYJxPZerGNMtE5jKclOrHg6UqVzq5Xsq3KJm4iw3YMpacjg iMdaM5jrK2is1/qyy2a3cCekwvwgClhGr9ShdT6s+t4aLEwC3XGiaKwi6Ymwy/KPyM pX7xBQfl9Pb0A== Received: by mail-oo1-f71.google.com with SMTP id z4-20020a4a9c84000000b00329c324ddb7so1952458ooj.22 for ; Wed, 20 Apr 2022 22:14:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=aroICBY1YQJ2/vHc4fu0o/SN0T9U5LUHOpX0JH0OBAI=; b=V1TPxaNto6biOPlEnaUIzr21NI+aV0ZqvndKYG5+yGrGNJZ9m9ueVBRCuiVxB8e4KZ e5UzerbRP1mm5UNmXLIshXCMVeZ1PNO3CqLGSmYs2mK0a5i8oLG9JCCGh79JtLS6WUg8 SpP0rDrAL7ihoaUewLx81hjGVhouxou8qBZYyL9DBZHBhcwwjSAXK2oh8y4D231TXtWQ 2P3T2BZbTGoWopYAP7b3kcKr0eqBeXsnj0nsOo98GgRZC77rtXnp/b3oR6lDKNQKdjMl F9aZwtnnmupStLobK9RB/97utgQ0Iz1XCxjoYHbh485qku7hSaM1MoMtrlzuCT0kMn60 /uVw== X-Gm-Message-State: AOAM5332jNwV7sOA/w+iLb6BL5ZWSV1seXodKQHV9SzraCcvxz6JG9Pq yXqzLjNszSLyUv5LZ+OK5fTBl1LokQt04oNaLxY2u8N40/wCLdqVLAb5GqnKaeb8WMH8kK11OnN PS6Yjk1OVkVRz5GBvpkR7/Bsm+1LLe/WiOrfyMCcs X-Received: by 2002:a05:6870:a108:b0:e5:ce89:5034 with SMTP id m8-20020a056870a10800b000e5ce895034mr3238710oae.127.1650518067169; Wed, 20 Apr 2022 22:14:27 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxk4E8A5PhxrhTTjLPibMeYN4uQo6GU5qDUKVd5ZVUq0wxLXhagq6BH750hS/BaV9u34R63ekjrgRRXEqUksPw= X-Received: by 2002:a05:6870:a108:b0:e5:ce89:5034 with SMTP id m8-20020a056870a10800b000e5ce895034mr3238701oae.127.1650518066877; Wed, 20 Apr 2022 22:14:26 -0700 (PDT) MIME-Version: 1.0 References: <20220322103334.12446-1-lihuisong@huawei.com> In-Reply-To: <20220322103334.12446-1-lihuisong@huawei.com> From: Christian Ehrhardt Date: Thu, 21 Apr 2022 07:14:01 +0200 Message-ID: Subject: Re: [PATCH 19.11] net/hns3: fix insecure way to query MAC statistics To: Huisong Li Cc: stable@dpdk.org, huangdaode@huawei.com, humin29@huawei.com Content-Type: text/plain; charset="UTF-8" X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org On Tue, Mar 22, 2022 at 11:34 AM Huisong Li 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 > Signed-off-by: Min Hu (Connor) > --- > 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