diff mbox series

[RESEND,V2,net,6/7] net: hns3: fixed hclge_fetch_pf_reg accesses bar space out of bounds issue

Message ID 20241217010839.1742227-7-shaojijie@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series There are some bugfix for the HNS3 ethernet driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: leon@kernel.org; 2 maintainers not CCed: lanhao@huawei.com leon@kernel.org
netdev/build_clang success Errors and warnings before: 10 this patch: 10
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 49 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-12-19--00-00 (tests: 880)

Commit Message

Jijie Shao Dec. 17, 2024, 1:08 a.m. UTC 
From: Hao Lan <lanhao@huawei.com>

The TQP BAR space is divided into two segments. TQPs 0-1023 and TQPs
1024-1279 are in different BAR space addresses. However,
hclge_fetch_pf_reg does not distinguish the tqp space information when
reading the tqp space information. When the number of TQPs is greater
than 1024, access bar space overwriting occurs.
The problem of different segments has been considered during the
initialization of tqp.io_base. Therefore, tqp.io_base is directly used
when the queue is read in hclge_fetch_pf_reg.

The error message:

Unable to handle kernel paging request at virtual address ffff800037200000
pc : hclge_fetch_pf_reg+0x138/0x250 [hclge]
lr : hclge_get_regs+0x84/0x1d0 [hclge]
Call trace:
 hclge_fetch_pf_reg+0x138/0x250 [hclge]
 hclge_get_regs+0x84/0x1d0 [hclge]
 hns3_get_regs+0x2c/0x50 [hns3]
 ethtool_get_regs+0xf4/0x270
 dev_ethtool+0x674/0x8a0
 dev_ioctl+0x270/0x36c
 sock_do_ioctl+0x110/0x2a0
 sock_ioctl+0x2ac/0x530
 __arm64_sys_ioctl+0xa8/0x100
 invoke_syscall+0x4c/0x124
 el0_svc_common.constprop.0+0x140/0x15c
 do_el0_svc+0x30/0xd0
 el0_svc+0x1c/0x2c
 el0_sync_handler+0xb0/0xb4
 el0_sync+0x168/0x180

Fixes: 939ccd107ffc ("net: hns3: move dump regs function to a separate file")
Signed-off-by: Hao Lan <lanhao@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c  | 9 +++++----
 .../net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c    | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

Comments

Michal Swiatkowski Dec. 18, 2024, 9:29 a.m. UTC | #1 
On Tue, Dec 17, 2024 at 09:08:38AM +0800, Jijie Shao wrote:
> From: Hao Lan <lanhao@huawei.com>
> 
> The TQP BAR space is divided into two segments. TQPs 0-1023 and TQPs
> 1024-1279 are in different BAR space addresses. However,
> hclge_fetch_pf_reg does not distinguish the tqp space information when
> reading the tqp space information. When the number of TQPs is greater
> than 1024, access bar space overwriting occurs.
> The problem of different segments has been considered during the
> initialization of tqp.io_base. Therefore, tqp.io_base is directly used
> when the queue is read in hclge_fetch_pf_reg.
> 
> The error message:
> 
> Unable to handle kernel paging request at virtual address ffff800037200000
> pc : hclge_fetch_pf_reg+0x138/0x250 [hclge]
> lr : hclge_get_regs+0x84/0x1d0 [hclge]
> Call trace:
>  hclge_fetch_pf_reg+0x138/0x250 [hclge]
>  hclge_get_regs+0x84/0x1d0 [hclge]
>  hns3_get_regs+0x2c/0x50 [hns3]
>  ethtool_get_regs+0xf4/0x270
>  dev_ethtool+0x674/0x8a0
>  dev_ioctl+0x270/0x36c
>  sock_do_ioctl+0x110/0x2a0
>  sock_ioctl+0x2ac/0x530
>  __arm64_sys_ioctl+0xa8/0x100
>  invoke_syscall+0x4c/0x124
>  el0_svc_common.constprop.0+0x140/0x15c
>  do_el0_svc+0x30/0xd0
>  el0_svc+0x1c/0x2c
>  el0_sync_handler+0xb0/0xb4
>  el0_sync+0x168/0x180
> 
> Fixes: 939ccd107ffc ("net: hns3: move dump regs function to a separate file")
> Signed-off-by: Hao Lan <lanhao@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c  | 9 +++++----
>  .../net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c    | 9 +++++----
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
> index 43c1c18fa81f..8c057192aae6 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
> @@ -510,9 +510,9 @@ static int hclge_get_dfx_reg(struct hclge_dev *hdev, void *data)
>  static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>  			      struct hnae3_knic_private_info *kinfo)
>  {
> -#define HCLGE_RING_REG_OFFSET		0x200
>  #define HCLGE_RING_INT_REG_OFFSET	0x4
>  
> +	struct hnae3_queue *tqp;
>  	int i, j, reg_num;
>  	int data_num_sum;
>  	u32 *reg = data;
> @@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>  	reg_num = ARRAY_SIZE(ring_reg_addr_list);
>  	for (j = 0; j < kinfo->num_tqps; j++) {
You can define struct hnae3_queue *tqp here to limit the scope
(same in VF case).
>  		reg += hclge_reg_get_tlv(HCLGE_REG_TAG_RING, reg_num, reg);
> +		tqp = kinfo->tqp[j];
>  		for (i = 0; i < reg_num; i++)
> -			*reg++ = hclge_read_dev(&hdev->hw,
> -						ring_reg_addr_list[i] +
> -						HCLGE_RING_REG_OFFSET * j);
> +			*reg++ = readl_relaxed(tqp->io_base -
> +					       HCLGE_TQP_REG_OFFSET +
> +					       ring_reg_addr_list[i]);
>  	}
>  	data_num_sum += (reg_num + HCLGE_REG_TLV_SPACE) * kinfo->num_tqps;
>  
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
> index 6db415d8b917..7d9d9dbc7560 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
> @@ -123,10 +123,10 @@ int hclgevf_get_regs_len(struct hnae3_handle *handle)
>  void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
>  		      void *data)
>  {
> -#define HCLGEVF_RING_REG_OFFSET		0x200
>  #define HCLGEVF_RING_INT_REG_OFFSET	0x4
>  
>  	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
> +	struct hnae3_queue *tqp;
>  	int i, j, reg_um;
>  	u32 *reg = data;
>  
> @@ -147,10 +147,11 @@ void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
>  	reg_um = ARRAY_SIZE(ring_reg_addr_list);
>  	for (j = 0; j < hdev->num_tqps; j++) {
>  		reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_RING, reg_um, reg);
> +		tqp = &hdev->htqp[j].q;
>  		for (i = 0; i < reg_um; i++)
> -			*reg++ = hclgevf_read_dev(&hdev->hw,
> -						  ring_reg_addr_list[i] +
> -						  HCLGEVF_RING_REG_OFFSET * j);
> +			*reg++ = readl_relaxed(tqp->io_base -
> +					       HCLGEVF_TQP_REG_OFFSET +
> +					       ring_reg_addr_list[i]);
>  	}
>  
>  	reg_um = ARRAY_SIZE(tqp_intr_reg_addr_list);
> -- 
> 2.33.0
Paolo Abeni Dec. 19, 2024, 9:51 a.m. UTC | #2 
On 12/18/24 10:29, Michal Swiatkowski wrote:
>> @@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>>  	reg_num = ARRAY_SIZE(ring_reg_addr_list);
>>  	for (j = 0; j < kinfo->num_tqps; j++) {
> You can define struct hnae3_queue *tqp here to limit the scope
> (same in VF case).

@Michal, please let me refer to prior feedback from Jakub:

https://lore.kernel.org/netdev/20241028163554.7dddff8b@kernel.org/

I also agree subjective stylistic feedback should be avoided unless the
style issue is really gross - in such a case the feedback should not be
subjective, so the original guidance still applies ;)

Thanks,

Paolo
Michal Swiatkowski Dec. 19, 2024, 10:23 a.m. UTC | #3 
On Thu, Dec 19, 2024 at 10:51:08AM +0100, Paolo Abeni wrote:
> On 12/18/24 10:29, Michal Swiatkowski wrote:
> >> @@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
> >>  	reg_num = ARRAY_SIZE(ring_reg_addr_list);
> >>  	for (j = 0; j < kinfo->num_tqps; j++) {
> > You can define struct hnae3_queue *tqp here to limit the scope
> > (same in VF case).
> 
> @Michal, please let me refer to prior feedback from Jakub:
> 
> https://lore.kernel.org/netdev/20241028163554.7dddff8b@kernel.org/
> 
> I also agree subjective stylistic feedback should be avoided unless the
> style issue is really gross - in such a case the feedback should not be
> subjective, so the original guidance still applies ;)
> 

Sure, I thought sometimes there were a feedback about scoping, but maybe
not from the maintainers. I will drop such comments next time, thanks
for letting me know.

Side note is that by "You can define" I meant if you want, if you feel
so, not you have to (sorry, not a native speaker).
But I understand that this unnecessary slow down the process when there
is no other (valid) changes to do, so I won't do that next time.

Thanks

> Thanks,
> 
> Paolo
>
Jijie Shao Dec. 19, 2024, 11:52 a.m. UTC | #4 
on 2024/12/18 17:29, Michal Swiatkowski wrote:
> On Tue, Dec 17, 2024 at 09:08:38AM +0800, Jijie Shao wrote:
>> From: Hao Lan <lanhao@huawei.com>
>>
>> The TQP BAR space is divided into two segments. TQPs 0-1023 and TQPs
>> 1024-1279 are in different BAR space addresses. However,
>> hclge_fetch_pf_reg does not distinguish the tqp space information when
>> reading the tqp space information. When the number of TQPs is greater
>> than 1024, access bar space overwriting occurs.
>> The problem of different segments has been considered during the
>> initialization of tqp.io_base. Therefore, tqp.io_base is directly used
>> when the queue is read in hclge_fetch_pf_reg.
>>
>> The error message:
>>
>> Unable to handle kernel paging request at virtual address ffff800037200000
>> pc : hclge_fetch_pf_reg+0x138/0x250 [hclge]
>> lr : hclge_get_regs+0x84/0x1d0 [hclge]
>> Call trace:
>>   hclge_fetch_pf_reg+0x138/0x250 [hclge]
>>   hclge_get_regs+0x84/0x1d0 [hclge]
>>   hns3_get_regs+0x2c/0x50 [hns3]
>>   ethtool_get_regs+0xf4/0x270
>>   dev_ethtool+0x674/0x8a0
>>   dev_ioctl+0x270/0x36c
>>   sock_do_ioctl+0x110/0x2a0
>>   sock_ioctl+0x2ac/0x530
>>   __arm64_sys_ioctl+0xa8/0x100
>>   invoke_syscall+0x4c/0x124
>>   el0_svc_common.constprop.0+0x140/0x15c
>>   do_el0_svc+0x30/0xd0
>>   el0_svc+0x1c/0x2c
>>   el0_sync_handler+0xb0/0xb4
>>   el0_sync+0x168/0x180
>>
>> Fixes: 939ccd107ffc ("net: hns3: move dump regs function to a separate file")
>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c  | 9 +++++----
>>   .../net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c    | 9 +++++----
>>   2 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
>> index 43c1c18fa81f..8c057192aae6 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
>> @@ -510,9 +510,9 @@ static int hclge_get_dfx_reg(struct hclge_dev *hdev, void *data)
>>   static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>>   			      struct hnae3_knic_private_info *kinfo)
>>   {
>> -#define HCLGE_RING_REG_OFFSET		0x200
>>   #define HCLGE_RING_INT_REG_OFFSET	0x4
>>   
>> +	struct hnae3_queue *tqp;
>>   	int i, j, reg_num;
>>   	int data_num_sum;
>>   	u32 *reg = data;
>> @@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>>   	reg_num = ARRAY_SIZE(ring_reg_addr_list);
>>   	for (j = 0; j < kinfo->num_tqps; j++) {
> You can define struct hnae3_queue *tqp here to limit the scope
> (same in VF case).

Hi:

To keep consistent with other code styles of the driver, this may not be changed.

Thank you.
Jijie Shao

>>   		reg += hclge_reg_get_tlv(HCLGE_REG_TAG_RING, reg_num, reg);
>> +		tqp = kinfo->tqp[j];
>>   		for (i = 0; i < reg_num; i++)
>> -			*reg++ = hclge_read_dev(&hdev->hw,
>> -						ring_reg_addr_list[i] +
>> -						HCLGE_RING_REG_OFFSET * j);
>> +			*reg++ = readl_relaxed(tqp->io_base -
>> +					       HCLGE_TQP_REG_OFFSET +
>> +					       ring_reg_addr_list[i]);
>>   	}
>>   	data_num_sum += (reg_num + HCLGE_REG_TLV_SPACE) * kinfo->num_tqps;
>>   
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
>> index 6db415d8b917..7d9d9dbc7560 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
>> @@ -123,10 +123,10 @@ int hclgevf_get_regs_len(struct hnae3_handle *handle)
>>   void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
>>   		      void *data)
>>   {
>> -#define HCLGEVF_RING_REG_OFFSET		0x200
>>   #define HCLGEVF_RING_INT_REG_OFFSET	0x4
>>   
>>   	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
>> +	struct hnae3_queue *tqp;
>>   	int i, j, reg_um;
>>   	u32 *reg = data;
>>   
>> @@ -147,10 +147,11 @@ void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
>>   	reg_um = ARRAY_SIZE(ring_reg_addr_list);
>>   	for (j = 0; j < hdev->num_tqps; j++) {
>>   		reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_RING, reg_um, reg);
>> +		tqp = &hdev->htqp[j].q;
>>   		for (i = 0; i < reg_um; i++)
>> -			*reg++ = hclgevf_read_dev(&hdev->hw,
>> -						  ring_reg_addr_list[i] +
>> -						  HCLGEVF_RING_REG_OFFSET * j);
>> +			*reg++ = readl_relaxed(tqp->io_base -
>> +					       HCLGEVF_TQP_REG_OFFSET +
>> +					       ring_reg_addr_list[i]);
>>   	}
>>   
>>   	reg_um = ARRAY_SIZE(tqp_intr_reg_addr_list);
>> -- 
>> 2.33.0
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
index 43c1c18fa81f..8c057192aae6 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
@@ -510,9 +510,9 @@  static int hclge_get_dfx_reg(struct hclge_dev *hdev, void *data)
 static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
 			      struct hnae3_knic_private_info *kinfo)
 {
-#define HCLGE_RING_REG_OFFSET		0x200
 #define HCLGE_RING_INT_REG_OFFSET	0x4
 
+	struct hnae3_queue *tqp;
 	int i, j, reg_num;
 	int data_num_sum;
 	u32 *reg = data;
@@ -533,10 +533,11 @@  static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
 	reg_num = ARRAY_SIZE(ring_reg_addr_list);
 	for (j = 0; j < kinfo->num_tqps; j++) {
 		reg += hclge_reg_get_tlv(HCLGE_REG_TAG_RING, reg_num, reg);
+		tqp = kinfo->tqp[j];
 		for (i = 0; i < reg_num; i++)
-			*reg++ = hclge_read_dev(&hdev->hw,
-						ring_reg_addr_list[i] +
-						HCLGE_RING_REG_OFFSET * j);
+			*reg++ = readl_relaxed(tqp->io_base -
+					       HCLGE_TQP_REG_OFFSET +
+					       ring_reg_addr_list[i]);
 	}
 	data_num_sum += (reg_num + HCLGE_REG_TLV_SPACE) * kinfo->num_tqps;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
index 6db415d8b917..7d9d9dbc7560 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
@@ -123,10 +123,10 @@  int hclgevf_get_regs_len(struct hnae3_handle *handle)
 void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
 		      void *data)
 {
-#define HCLGEVF_RING_REG_OFFSET		0x200
 #define HCLGEVF_RING_INT_REG_OFFSET	0x4
 
 	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
+	struct hnae3_queue *tqp;
 	int i, j, reg_um;
 	u32 *reg = data;
 
@@ -147,10 +147,11 @@  void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
 	reg_um = ARRAY_SIZE(ring_reg_addr_list);
 	for (j = 0; j < hdev->num_tqps; j++) {
 		reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_RING, reg_um, reg);
+		tqp = &hdev->htqp[j].q;
 		for (i = 0; i < reg_um; i++)
-			*reg++ = hclgevf_read_dev(&hdev->hw,
-						  ring_reg_addr_list[i] +
-						  HCLGEVF_RING_REG_OFFSET * j);
+			*reg++ = readl_relaxed(tqp->io_base -
+					       HCLGEVF_TQP_REG_OFFSET +
+					       ring_reg_addr_list[i]);
 	}
 
 	reg_um = ARRAY_SIZE(tqp_intr_reg_addr_list);