rtw89: add Realtek 802.11ax driver

bug report Dan Carpenter 2 participants 3 messages Nov 12, 2021 – Nov 19, 2021

Share & tools

Pkshih

2021-11-15 07:34 UTC #1 lore raw

Quoted 48 lines

-----Original Message-----
From: Dan Carpenter <[email protected]>
Sent: Friday, November 12, 2021 3:52 PM
To: Pkshih <[email protected]>
Cc: [email protected]
Subject: [bug report] rtw89: add Realtek 802.11ax driver
Hello Ping-Ke Shih,
The patch e3ec7017f6a2: "rtw89: add Realtek 802.11ax driver" from Oct 11, 2021, leads to the following Smatch static checker warning:
	drivers/net/wireless/realtek/rtw89/fw.c:1383 rtw89_fw_h2c_rf_reg()
	error: buffer overflow 'info->rtw89_phy_config_rf_h2c' 3 <= 3
drivers/net/wireless/realtek/rtw89/phy.c
662 static int rtw89_phy_config_rf_reg_fw(struct rtw89_dev *rtwdev,
663 struct rtw89_fw_h2c_rf_reg_info *info)
   664  {
   665          u16 page = info->curr_idx / RTW89_H2C_RF_PAGE_SIZE;
   666          u16 len = (info->curr_idx % RTW89_H2C_RF_PAGE_SIZE) * 4;
   667          u8 i;
   668          int ret = 0;
   669
   670          if (page > RTW89_H2C_RF_PAGE_NUM) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Lets assume "page == RTW89_H2C_RF_PAGE_NUM.
   671                  rtw89_warn(rtwdev,
   672                             "rf reg h2c total page num %d larger than %d (RTW89_H2C_RF_PAGE_NUM)\n",
   673                             page, RTW89_H2C_RF_PAGE_NUM);
   674                  return -EINVAL;
   675          }
   676
   677          for (i = 0; i < page; i++) {
                            ^^^^^^^^^

   678                  ret = rtw89_fw_h2c_rf_reg(rtwdev, info,
   679                                            RTW89_H2C_RF_PAGE_SIZE * 4, i);
   680                  if (ret)
   681                          return ret;
   682          }
   683          ret = rtw89_fw_h2c_rf_reg(rtwdev, info, len, i);
                                                             ^
So "i" is now RTW89_H2C_RF_PAGE_NUM and it leads to off by one out of bounds error.

Quoted 7 lines

   684          if (ret)
   685                  return ret;
   686          info->curr_idx = 0;
   687
   688          return 0;
   689  }

When the info->curr_idx is between RTW89_H2C_RF_PAGE_SIZE * RTW89_H2C_RF_PAGE_NUM and RTW89_H2C_RF_PAGE_SIZE * (RTW89_H2C_RF_PAGE_NUM + 1), it falls into the case you mentioned. Fortunately, the input is predictable if we don't change RF parameters, so it doesn't lead wrong situation for now.

Anyway, I will refine this function to handle all cases. Thanks for your finding.

--
Ping-Ke

Pkshih

2021-11-19 06:02 UTC #2 lore raw

Quoted 64 lines

-----Original Message-----
From: Pkshih <[email protected]>
Sent: Monday, November 15, 2021 3:34 PM
To: Dan Carpenter <[email protected]>
Cc: [email protected]
Subject: RE: [bug report] rtw89: add Realtek 802.11ax driver
Quoted 26 lines
-----Original Message-----
From: Dan Carpenter <[email protected]>
Sent: Friday, November 12, 2021 3:52 PM
To: Pkshih <[email protected]>
Cc: [email protected]
Subject: [bug report] rtw89: add Realtek 802.11ax driver
Hello Ping-Ke Shih,
The patch e3ec7017f6a2: "rtw89: add Realtek 802.11ax driver" from Oct 11, 2021, leads to the following Smatch static checker warning:
	drivers/net/wireless/realtek/rtw89/fw.c:1383 rtw89_fw_h2c_rf_reg()
	error: buffer overflow 'info->rtw89_phy_config_rf_h2c' 3 <= 3
drivers/net/wireless/realtek/rtw89/phy.c
662 static int rtw89_phy_config_rf_reg_fw(struct rtw89_dev *rtwdev,
663 struct rtw89_fw_h2c_rf_reg_info *info)
   664  {
   665          u16 page = info->curr_idx / RTW89_H2C_RF_PAGE_SIZE;
   666          u16 len = (info->curr_idx % RTW89_H2C_RF_PAGE_SIZE) * 4;
   667          u8 i;
   668          int ret = 0;
   669
   670          if (page > RTW89_H2C_RF_PAGE_NUM) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 4 lines
Lets assume "page == RTW89_H2C_RF_PAGE_NUM.
671 rtw89_warn(rtwdev,
672 "rf reg h2c total page num %d larger than %d
(RTW89_H2C_RF_PAGE_NUM)\n",
Quoted 17 lines
   673                             page, RTW89_H2C_RF_PAGE_NUM);
   674                  return -EINVAL;
   675          }
   676
   677          for (i = 0; i < page; i++) {
                            ^^^^^^^^^
   678                  ret = rtw89_fw_h2c_rf_reg(rtwdev, info,
   679                                            RTW89_H2C_RF_PAGE_SIZE * 4, i);
   680                  if (ret)
   681                          return ret;
   682          }
   683          ret = rtw89_fw_h2c_rf_reg(rtwdev, info, len, i);
                                                             ^
So "i" is now RTW89_H2C_RF_PAGE_NUM and it leads to off by one out of bounds error.
Quoted 7 lines
   684          if (ret)
   685                  return ret;
   686          info->curr_idx = 0;
   687
   688          return 0;
   689  }

Quoted 8 lines

When the info->curr_idx is between RTW89_H2C_RF_PAGE_SIZE * RTW89_H2C_RF_PAGE_NUM and RTW89_H2C_RF_PAGE_SIZE * (RTW89_H2C_RF_PAGE_NUM + 1), it falls into the case you mentioned. Fortunately, the input is predictable if we don't change RF parameters, so it doesn't lead wrong situation for now.
Anyway, I will refine this function to handle all cases. Thanks for your finding.

I have sent a patch [1] to fix this.

[1] https://lore.kernel.org/linux-wireless/[email protected]/T/#u

--
Ping-Ke