staging: r8188eu: add error handling of usb read errors

PATCH Superseded Pavel Skripkin 3 participants 9 messages May 19, 2022 – Jun 6, 2022 Staging

lore

Share & tools

lore

Pavel Skripkin

author

2022-05-19 21:09 UTC #0 lore raw

v2 cover staging: r8188eu: add error handling of usb read errors

Hi,

it's reincarnation of my old series for adding sane error handling in r8818eu.

*Problem*

Old code was returning just stack variable in case of read error. It's not the best approach, since passing around stack data might cause device misconfiguration or even kernel data leakage

To solve this I've changed rtw_read{8,16,32} prototypes to return an error via return value and data via passed pointer. Some work should be done to propogate an error down to calltrace, but it's good way to at least start doing sane I/O error handling

Tested locally on qemu with TP-Link TL-WN722N v2/v3 [Realtek RTL8188EUS] device. More testing is welcomed, of course :)

_NOTE_
Series is based on top of staging-testing branch and should be
applied after Dan's patch https://lore.kernel.org/linux-staging/YoXS4OaD1oauPvmj@kili/

Changes since v1:
addresses issues found by Dan and self review. Mostly related to returning _FAIL instead of -errno, since callers expect _FAIL/_SUCCESS

v1: https://lore.kernel.org/linux-staging/[email protected]/

Pavel Skripkin (4):
staging: r8188eu: add error handling of rtw_read8
staging: r8188eu: add error handling of rtw_read16
staging: r8188eu: add error handling of rtw_read32
MAINTAINERS: add myself as r8188eu reviewer

 MAINTAINERS                                   |   1 +
 drivers/staging/r8188eu/core/rtw_cmd.c        |  15 +-
 drivers/staging/r8188eu/core/rtw_efuse.c      |  33 ++-
 drivers/staging/r8188eu/core/rtw_fw.c         |  72 ++++--
 drivers/staging/r8188eu/core/rtw_led.c        |  16 +-
 drivers/staging/r8188eu/core/rtw_mlme_ext.c   |  62 ++++-
 drivers/staging/r8188eu/core/rtw_pwrctrl.c    |   9 +-
 drivers/staging/r8188eu/core/rtw_wlan_util.c  |  20 +-
 .../r8188eu/hal/Hal8188ERateAdaptive.c        |  21 +-
 drivers/staging/r8188eu/hal/HalPhyRf_8188e.c  |  21 +-
 drivers/staging/r8188eu/hal/HalPwrSeqCmd.c    |   9 +-
 drivers/staging/r8188eu/hal/hal_com.c         |  27 +-
 drivers/staging/r8188eu/hal/rtl8188e_cmd.c    |  37 ++-
 drivers/staging/r8188eu/hal/rtl8188e_dm.c     |   6 +-
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 130 +++++++---
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  30 ++-
 drivers/staging/r8188eu/hal/usb_halinit.c     | 240 +++++++++++++++---
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |  33 ++-
 drivers/staging/r8188eu/include/rtw_io.h      |   6 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  |  47 +++-
 drivers/staging/r8188eu/os_dep/os_intfs.c     |  19 +-
 21 files changed, 679 insertions(+), 175 deletions(-)

Signature · 1 line

2.36.1

Pavel Skripkin

author

2022-05-19 21:09 UTC #1 lore raw

PATCH v2 1/4 staging: r8188eu: add error handling of rtw_read8

rtw_read8() reads data from device via USB API which may fail. In case of any failure previous code returned stack data to callers, which is wrong.

Fix it by changing rtw_read8() prototype and prevent caller from touching random stack data

Signed-off-by Pavel Skripkin <[email protected]>

---

Changes since v1:
- Fix build error reported by kernel test robot
- Fix res/ret confusion in rtl8188e_firmware_download
- Fix possible infinity loop in rtl8188e_firmware_download
- Undo unrelated white-space Changes
- Fix wront return value in c2h_evt_read

---
 drivers/staging/r8188eu/core/rtw_efuse.c      |  13 +-
 drivers/staging/r8188eu/core/rtw_fw.c         |  56 ++++--
 drivers/staging/r8188eu/core/rtw_led.c        |  16 +-
 drivers/staging/r8188eu/core/rtw_mlme_ext.c   |  49 +++++-
 drivers/staging/r8188eu/core/rtw_wlan_util.c  |  20 ++-
 drivers/staging/r8188eu/hal/HalPhyRf_8188e.c  |  18 +-
 drivers/staging/r8188eu/hal/HalPwrSeqCmd.c    |   9 +-
 drivers/staging/r8188eu/hal/hal_com.c         |  27 ++-
 drivers/staging/r8188eu/hal/rtl8188e_cmd.c    |  37 +++-
 drivers/staging/r8188eu/hal/rtl8188e_dm.c     |   6 +-
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   |  69 ++++++--
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  10 +-
 drivers/staging/r8188eu/hal/usb_halinit.c     | 160 ++++++++++++++----
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |   7 +-
 drivers/staging/r8188eu/include/rtw_io.h      |   2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  |  11 +-
 16 files changed, 410 insertions(+), 100 deletions(-)

drivers/staging/r8188eu/core/rtw_efuse.c +11 -2

index 0e0e60638880..a2691c7f96f6 100644
@@ -28,14 +28,21 @@ ReadEFuseByte(
 	u32 value32;
 	u8 readbyte;
 	u16 retry;
+	int res;
 
 	/* Write Address */
 	rtw_write8(Adapter, EFUSE_CTRL + 1, (_offset & 0xff));
-	readbyte = rtw_read8(Adapter, EFUSE_CTRL + 2);
+	res = rtw_read8(Adapter, EFUSE_CTRL + 2, &readbyte);
+	if (res)
+		return;
+
 	rtw_write8(Adapter, EFUSE_CTRL + 2, ((_offset >> 8) & 0x03) | (readbyte & 0xfc));
 
 	/* Write bit 32 0 */
-	readbyte = rtw_read8(Adapter, EFUSE_CTRL + 3);
+	res = rtw_read8(Adapter, EFUSE_CTRL + 3, &readbyte);
+	if (res)
+		return;
+
 	rtw_write8(Adapter, EFUSE_CTRL + 3, (readbyte & 0x7f));
 
 	/* Check bit 32 read-ready */

@@ -54,6 +61,8 @@ ReadEFuseByte(
 	value32 = rtw_read32(Adapter, EFUSE_CTRL);
 
 	*pbuf = (u8)(value32 & 0xff);
+
+	/* FIXME: return an error to caller */
 }
 
 /*-----------------------------------------------------------------------------

drivers/staging/r8188eu/core/rtw_fw.c +43 ~~-13~~

107 lines not on this page · raw message

drivers/staging/r8188eu/core/rtw_led.c +13 -3

index 2f3000428af7..25989acf5259 100644
@@ -35,11 +35,15 @@ static void ResetLedStatus(struct LED_871x *pLed)
 static void SwLedOn(struct adapter *padapter, struct LED_871x *pLed)
 {
 	u8	LedCfg;
+	int res;
 
 	if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
 		return;
 
-	LedCfg = rtw_read8(padapter, REG_LEDCFG2);
+	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);
+	if (res)
+		return;
+
 	rtw_write8(padapter, REG_LEDCFG2, (LedCfg & 0xf0) | BIT(5) | BIT(6)); /*  SW control led0 on. */
 	pLed->bLedOn = true;
 }

@@ -47,15 +51,21 @@ static void SwLedOn(struct adapter *padapter, struct LED_871x *pLed)
 static void SwLedOff(struct adapter *padapter, struct LED_871x *pLed)
 {
 	u8	LedCfg;
+	int res;
 
 	if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
 		goto exit;
 
-	LedCfg = rtw_read8(padapter, REG_LEDCFG2);/* 0x4E */
+	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);/* 0x4E */
+	if (res)
+		goto exit;
 
 	LedCfg &= 0x90; /*  Set to software control. */
 	rtw_write8(padapter, REG_LEDCFG2, (LedCfg | BIT(3)));
-	LedCfg = rtw_read8(padapter, REG_MAC_PINMUX_CFG);
+	res = rtw_read8(padapter, REG_MAC_PINMUX_CFG, &LedCfg);
+	if (res)
+		goto exit;
+
 	LedCfg &= 0xFE;
 	rtw_write8(padapter, REG_MAC_PINMUX_CFG, LedCfg);
 exit:

drivers/staging/r8188eu/core/rtw_mlme_ext.c +42 -7

107 lines not on this page · raw message

drivers/staging/r8188eu/core/rtw_wlan_util.c +17 -3

index 27035eac6e61..f5002c88a5ac 100644
@@ -279,8 +279,13 @@ void Restore_DM_Func_Flag(struct adapter *padapter)
 void Set_MSR(struct adapter *padapter, u8 type)
 {
 	u8 val8;
+	int res;
 
-	val8 = rtw_read8(padapter, MSR) & 0x0c;
+	res = rtw_read8(padapter, MSR, &val8);
+	if (res)
+		return;
+
+	val8 &= 0x0c;
 	val8 |= type;
 	rtw_write8(padapter, MSR, val8);
 }

@@ -505,7 +510,11 @@ int WMM_param_handler(struct adapter *padapter, struct ndis_802_11_var_ie *pIE)
 
 static void set_acm_ctrl(struct adapter *adapter, u8 acm_mask)
 {
-	u8 acmctrl = rtw_read8(adapter, REG_ACMHWCTRL);
+	u8 acmctrl;
+	int res = rtw_read8(adapter, REG_ACMHWCTRL, &acmctrl);
+
+	if (res)
+		return;
 
 	if (acm_mask > 1)
 		acmctrl = acmctrl | 0x1;

@@ -763,6 +772,7 @@ void HT_info_handler(struct adapter *padapter, struct ndis_802_11_var_ie *pIE)
 static void set_min_ampdu_spacing(struct adapter *adapter, u8 spacing)
 {
 	u8 sec_spacing;
+	int res;
 
 	if (spacing <= 7) {
 		switch (adapter->securitypriv.dot11PrivacyAlgrthm) {

@@ -784,8 +794,12 @@ static void set_min_ampdu_spacing(struct adapter *adapter, u8 spacing)
 		if (spacing < sec_spacing)
 			spacing = sec_spacing;
 
+		res = rtw_read8(adapter, REG_AMPDU_MIN_SPACE, &sec_spacing);
+		if (res)
+			return;
+
 		rtw_write8(adapter, REG_AMPDU_MIN_SPACE,
-			   (rtw_read8(adapter, REG_AMPDU_MIN_SPACE) & 0xf8) | spacing);
+			   (sec_spacing & 0xf8) | spacing);
 	}
 }

drivers/staging/r8188eu/hal/HalPhyRf_8188e.c +15 -3

index b944c8071a3b..a5b7980dfcee 100644
@@ -463,6 +463,7 @@ void _PHY_SaveADDARegisters(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup
 	}
 }
 
+/* FIXME: return an error to caller */
 static void _PHY_SaveMACRegisters(
 		struct adapter *adapt,
 		u32 *MACReg,

@@ -470,9 +471,17 @@ static void _PHY_SaveMACRegisters(
 	)
 {
 	u32 i;
+	int res;
 
-	for (i = 0; i < (IQK_MAC_REG_NUM - 1); i++)
-		MACBackup[i] = rtw_read8(adapt, MACReg[i]);
+	for (i = 0; i < (IQK_MAC_REG_NUM - 1); i++) {
+		u8 reg;
+
+		res = rtw_read8(adapt, MACReg[i], &reg);
+		if (res)
+			return;
+
+		MACBackup[i] = reg;
+	}
 
 	MACBackup[i] = rtw_read32(adapt, MACReg[i]);
 }

@@ -739,9 +748,12 @@ static void phy_LCCalibrate_8188E(struct adapter *adapt)
 {
 	u8 tmpreg;
 	u32 RF_Amode = 0, LC_Cal;
+	int res;
 
 	/* Check continuous TX and Packet TX */
-	tmpreg = rtw_read8(adapt, 0xd03);
+	res = rtw_read8(adapt, 0xd03, &tmpreg);
+	if (res)
+		return;
 
 	if ((tmpreg & 0x70) != 0)			/* Deal with contisuous TX case */
 		rtw_write8(adapt, 0xd03, tmpreg & 0x8F);	/* disable all continuous TX */

drivers/staging/r8188eu/hal/HalPwrSeqCmd.c +7 -2

index 5b91aec6a7e3..fe2fe63dbc18 100644
@@ -34,6 +34,7 @@ u8 HalPwrSeqCmdParsing(struct adapter *padapter, struct wl_pwr_cfg pwrseqcmd[])
 	u32 offset = 0;
 	u32 poll_count = 0; /*  polling autoload done. */
 	u32 max_poll_count = 5000;
+	int res;
 
 	do {
 		pwrcfgcmd = pwrseqcmd[aryidx];

@@ -43,7 +44,9 @@ u8 HalPwrSeqCmdParsing(struct adapter *padapter, struct wl_pwr_cfg pwrseqcmd[])
 			offset = GET_PWR_CFG_OFFSET(pwrcfgcmd);
 
 			/*  Read the value from system register */
-			value = rtw_read8(padapter, offset);
+			res = rtw_read8(padapter, offset, &value);
+			if (res)
+				return false;
 
 			value &= ~(GET_PWR_CFG_MASK(pwrcfgcmd));
 			value |= (GET_PWR_CFG_VALUE(pwrcfgcmd) & GET_PWR_CFG_MASK(pwrcfgcmd));

@@ -55,7 +58,9 @@ u8 HalPwrSeqCmdParsing(struct adapter *padapter, struct wl_pwr_cfg pwrseqcmd[])
 			poll_bit = false;
 			offset = GET_PWR_CFG_OFFSET(pwrcfgcmd);
 			do {
-				value = rtw_read8(padapter, offset);
+				res = rtw_read8(padapter, offset, &value);
+				if (res)
+					return false;
 
 				value &= GET_PWR_CFG_MASK(pwrcfgcmd);
 				if (value == (GET_PWR_CFG_VALUE(pwrcfgcmd) & GET_PWR_CFG_MASK(pwrcfgcmd)))

drivers/staging/r8188eu/hal/hal_com.c +21 -6

index 910cc07f656c..e9a32dd84a8e 100644
@@ -303,7 +303,9 @@ s32 c2h_evt_read(struct adapter *adapter, u8 *buf)
 	if (!buf)
 		goto exit;
 
-	trigger = rtw_read8(adapter, REG_C2HEVT_CLEAR);
+	ret = rtw_read8(adapter, REG_C2HEVT_CLEAR, &trigger);
+	if (ret)
+		return _FAIL;
 
 	if (trigger == C2H_EVT_HOST_CLOSE)
 		goto exit; /* Not ready */

@@ -314,13 +316,26 @@ s32 c2h_evt_read(struct adapter *adapter, u8 *buf)
 
 	memset(c2h_evt, 0, 16);
 
-	*buf = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL);
-	*(buf + 1) = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL + 1);
+	ret = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL, buf);
+	if (ret) {
+		ret = _FAIL;
+		goto clear_evt;
+	}
 
+	ret = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL + 1, buf + 1);
+	if (ret) {
+		ret = _FAIL;
+		goto clear_evt;
+	}
 	/* Read the content */
-	for (i = 0; i < c2h_evt->plen; i++)
-		c2h_evt->payload[i] = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL +
-						sizeof(*c2h_evt) + i);
+	for (i = 0; i < c2h_evt->plen; i++) {
+		ret = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL +
+						sizeof(*c2h_evt) + i, c2h_evt->payload + i);
+		if (ret) {
+			ret = _FAIL;
+			goto clear_evt;
+		}
+	}
 
 	ret = _SUCCESS;

drivers/staging/r8188eu/hal/rtl8188e_cmd.c +31 -6

index 475650dc7301..b01ee1695fee 100644
@@ -18,13 +18,18 @@
 
 static u8 _is_fw_read_cmd_down(struct adapter *adapt, u8 msgbox_num)
 {
-	u8 read_down = false;
+	u8 read_down = false, reg;
 	int	retry_cnts = 100;
+	int res;
 
 	u8 valid;
 
 	do {
-		valid = rtw_read8(adapt, REG_HMETFR) & BIT(msgbox_num);
+		res = rtw_read8(adapt, REG_HMETFR, &reg);
+		if (res)
+			continue;
+
+		valid = reg & BIT(msgbox_num);
 		if (0 == valid)
 			read_down = true;
 	} while ((!read_down) && (retry_cnts--));

@@ -533,6 +538,8 @@ void rtl8188e_set_FwJoinBssReport_cmd(struct adapter *adapt, u8 mstatus)
 	bool	bcn_valid = false;
 	u8 DLBcnCount = 0;
 	u32 poll = 0;
+	u8 reg;
+	int res;
 
 	if (mstatus == 1) {
 		/*  We should set AID, correct TSF, HW seq enable before set JoinBssReport to Fw in 88/92C. */

@@ -547,8 +554,17 @@ void rtl8188e_set_FwJoinBssReport_cmd(struct adapter *adapt, u8 mstatus)
 		/*  Disable Hw protection for a time which revserd for Hw sending beacon. */
 		/*  Fix download reserved page packet fail that access collision with the protection time. */
 		/*  2010.05.11. Added by tynli. */
-		rtw_write8(adapt, REG_BCN_CTRL, rtw_read8(adapt, REG_BCN_CTRL) & (~BIT(3)));
-		rtw_write8(adapt, REG_BCN_CTRL, rtw_read8(adapt, REG_BCN_CTRL) | BIT(4));
+		res = rtw_read8(adapt, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapt, REG_BCN_CTRL, reg & (~BIT(3)));
+
+		res = rtw_read8(adapt, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapt, REG_BCN_CTRL, reg | BIT(4));
 
 		if (haldata->RegFwHwTxQCtrl & BIT(6))
 			bSendBeacon = true;

@@ -581,8 +597,17 @@ void rtl8188e_set_FwJoinBssReport_cmd(struct adapter *adapt, u8 mstatus)
 		/*  */
 
 		/*  Enable Bcn */
-		rtw_write8(adapt, REG_BCN_CTRL, rtw_read8(adapt, REG_BCN_CTRL) | BIT(3));
-		rtw_write8(adapt, REG_BCN_CTRL, rtw_read8(adapt, REG_BCN_CTRL) & (~BIT(4)));
+		res = rtw_read8(adapt, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapt, REG_BCN_CTRL, reg | BIT(3));
+
+		res = rtw_read8(adapt, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapt, REG_BCN_CTRL, reg & (~BIT(4)));
 
 		/*  To make sure that if there exists an adapter which would like to send beacon. */
 		/*  If exists, the origianl value of 0x422[6] will be 1, we should check this to */

drivers/staging/r8188eu/hal/rtl8188e_dm.c +5 -1

index 6d28e3dc0d26..0399872c4546 100644
@@ -12,8 +12,12 @@
 static void dm_InitGPIOSetting(struct adapter *Adapter)
 {
 	u8	tmp1byte;
+	int res;
+
+	res = rtw_read8(Adapter, REG_GPIO_MUXCFG, &tmp1byte);
+	if (res)
+		return;
 
-	tmp1byte = rtw_read8(Adapter, REG_GPIO_MUXCFG);
 	tmp1byte &= (GPIOSEL_GPIO | ~GPIOSEL_ENBT);
 
 	rtw_write8(Adapter, REG_GPIO_MUXCFG, tmp1byte);

drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +55 ~~-14~~

120 lines not on this page · raw message

drivers/staging/r8188eu/hal/rtl8188e_phycfg.c +8 -2

index 4864dafd887b..985339a974fc 100644
@@ -594,6 +594,7 @@ _PHY_SetBWMode92C(
 	struct hal_data_8188e *pHalData = &Adapter->haldata;
 	u8 regBwOpMode;
 	u8 regRRSR_RSC;
+	int res;
 
 	if (Adapter->bDriverStopped)
 		return;

@@ -602,8 +603,13 @@ _PHY_SetBWMode92C(
 	/* 3<1>Set MAC register */
 	/* 3 */
 
-	regBwOpMode = rtw_read8(Adapter, REG_BWOPMODE);
-	regRRSR_RSC = rtw_read8(Adapter, REG_RRSR + 2);
+	res = rtw_read8(Adapter, REG_BWOPMODE, &regBwOpMode);
+	if (res)
+		return;
+
+	res = rtw_read8(Adapter, REG_RRSR + 2, &regRRSR_RSC);
+	if (res)
+		return;
 
 	switch (pHalData->CurrentChannelBW) {
 	case HT_CHANNEL_WIDTH_20:

drivers/staging/r8188eu/hal/usb_halinit.c +131 ~~-29~~

326 lines not on this page · raw message

drivers/staging/r8188eu/hal/usb_ops_linux.c +2 -5

index d5e674542a78..f399a7fd8b97 100644
@@ -94,16 +94,13 @@ static int usb_write(struct intf_hdl *intf, u16 value, void *data, u8 size)
 	return status;
 }
 
-u8 rtw_read8(struct adapter *adapter, u32 addr)
+int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intf = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	u8 data;
 
-	usb_read(intf, value, &data, 1);
-
-	return data;
+	return usb_read(intf, value, data, 1);
 }
 
 u16 rtw_read16(struct adapter *adapter, u32 addr)

drivers/staging/r8188eu/include/rtw_io.h +1 -1

index 6910e2b430e2..1198d3850a6d 100644
@@ -220,7 +220,7 @@ void unregister_intf_hdl(struct intf_hdl *pintfhdl);
 void _rtw_attrib_read(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
-u8 rtw_read8(struct adapter *adapter, u32 addr);
+int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
 u16 rtw_read16(struct adapter *adapter, u32 addr);
 u32 rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);

drivers/staging/r8188eu/os_dep/ioctl_linux.c +8 -3

index a802c1e7b456..6e5ee2a74ee4 100644
@@ -3177,6 +3177,7 @@ static void rtw_set_dynamic_functions(struct adapter *adapter, u8 dm_func)
 {
 	struct hal_data_8188e *haldata = &adapter->haldata;
 	struct odm_dm_struct *odmpriv = &haldata->odmpriv;
+	int res;
 
 	switch (dm_func) {
 	case 0:

@@ -3192,7 +3193,9 @@ static void rtw_set_dynamic_functions(struct adapter *adapter, u8 dm_func)
 		if (!(odmpriv->SupportAbility & DYNAMIC_BB_DIG)) {
 			struct rtw_dig *digtable = &odmpriv->DM_DigTable;
 
-			digtable->CurIGValue = rtw_read8(adapter, 0xc50);
+			res = rtw_read8(adapter, 0xc50, &digtable->CurIGValue);
+			(void)res;
+			/* FIXME: return an error to caller */
 		}
 		odmpriv->SupportAbility = DYNAMIC_ALL_FUNC_ENABLE;
 		break;

@@ -3328,8 +3331,9 @@ static int rtw_dbg_port(struct net_device *dev,
 			u16 reg = arg;
 			u16 start_value = 0;
 			u32 write_num = extra_arg;
-			int i;
+			int i, res;
 			struct xmit_frame	*xmit_frame;
+			u8 val8;
 
 			xmit_frame = rtw_IOL_accquire_xmit_frame(padapter);
 			if (!xmit_frame) {

@@ -3342,7 +3346,8 @@ static int rtw_dbg_port(struct net_device *dev,
 			if (rtl8188e_IOL_exec_cmds_sync(padapter, xmit_frame, 5000, 0) != _SUCCESS)
 				ret = -EPERM;
 
-			rtw_read8(padapter, reg);
+			/* FIXME: is this read necessary? */
+			res = rtw_read8(padapter, reg, &val8);
 		}
 			break;

Signature · 1 line

2.36.1

Pavel Skripkin

author

2022-05-19 21:09 UTC #4 lore raw

PATCH v2 2/4 staging: r8188eu: add error handling of rtw_read16

In reply to Pavel Skripkin

rtw_read16() reads data from device via USB API which may fail. In case of any failure previous code returned stack data to callers, which is wrong.

Fix it by changing rtw_read16() prototype and prevent caller from touching random stack data

Signed-off-by Pavel Skripkin <[email protected]>

---

Changes since v1:
- Fixed style issue

---
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 21 ++++++++++++---
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  8 ++++--
 drivers/staging/r8188eu/hal/usb_halinit.c     | 27 ++++++++++++++++---
 drivers/staging/r8188eu/hal/usb_ops_linux.c   | 13 ++++++---
 drivers/staging/r8188eu/include/rtw_io.h      |  2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  |  9 ++++---
 drivers/staging/r8188eu/os_dep/os_intfs.c     |  6 ++++-
 7 files changed, 67 insertions(+), 19 deletions(-)

drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +17 -4

index e67ecbd1ba79..6662ebc30f7b 100644
@@ -249,11 +249,14 @@ static void efuse_read_phymap_from_txpktbuf(
 		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));
 
 		if (i == 0) {
+			u16 reg;
+
 			/* Although lenc is only used in a debug statement,
 			 * do not remove it as the rtw_read16() call consumes
 			 * 2 bytes from the EEPROM source.
 			 */
-			rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L);
+			res = rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L, &reg);
+			(void)res;
 
 			len = le32_to_cpu(lo32) & 0x0000ffff;

@@ -355,25 +358,35 @@ int rtl8188e_IOL_exec_cmds_sync(struct adapter *adapter, struct xmit_frame *xmit
 void rtl8188e_EfusePowerSwitch(struct adapter *pAdapter, u8 PwrState)
 {
 	u16	tmpV16;
+	int res;
 
 	if (PwrState) {
 		rtw_write8(pAdapter, REG_EFUSE_ACCESS, EFUSE_ACCESS_ON);
 
 		/*  1.2V Power: From VDDON with Power Cut(0x0000h[15]), defualt valid */
-		tmpV16 = rtw_read16(pAdapter, REG_SYS_ISO_CTRL);
+		res = rtw_read16(pAdapter, REG_SYS_ISO_CTRL, &tmpV16);
+		if (res)
+			return;
+
 		if (!(tmpV16 & PWC_EV12V)) {
 			tmpV16 |= PWC_EV12V;
 			rtw_write16(pAdapter, REG_SYS_ISO_CTRL, tmpV16);
 		}
 		/*  Reset: 0x0000h[28], default valid */
-		tmpV16 =  rtw_read16(pAdapter, REG_SYS_FUNC_EN);
+		res = rtw_read16(pAdapter, REG_SYS_FUNC_EN, &tmpV16);
+		if (res)
+			return;
+
 		if (!(tmpV16 & FEN_ELDR)) {
 			tmpV16 |= FEN_ELDR;
 			rtw_write16(pAdapter, REG_SYS_FUNC_EN, tmpV16);
 		}
 
 		/*  Clock: Gated(0x0008h[5]) 8M(0x0008h[1]) clock from ANA, default valid */
-		tmpV16 = rtw_read16(pAdapter, REG_SYS_CLKR);
+		res = rtw_read16(pAdapter, REG_SYS_CLKR, &tmpV16);
+		if (res)
+			return;
+
 		if ((!(tmpV16 & LOADER_CLK_EN))  || (!(tmpV16 & ANA8M))) {
 			tmpV16 |= (LOADER_CLK_EN | ANA8M);
 			rtw_write16(pAdapter, REG_SYS_CLKR, tmpV16);

drivers/staging/r8188eu/hal/rtl8188e_phycfg.c +6 -2

index 985339a974fc..298c3d9bc7be 100644
@@ -484,13 +484,17 @@ PHY_BBConfig8188E(
 {
 	int	rtStatus = _SUCCESS;
 	struct hal_data_8188e *pHalData = &Adapter->haldata;
-	u32 RegVal;
+	u16 RegVal;
 	u8 CrystalCap;
+	int res;
 
 	phy_InitBBRFRegisterDefinition(Adapter);
 
 	/*  Enable BB and RF */
-	RegVal = rtw_read16(Adapter, REG_SYS_FUNC_EN);
+	res = rtw_read16(Adapter, REG_SYS_FUNC_EN, &RegVal);
+	if (res)
+		return _FAIL;
+
 	rtw_write16(Adapter, REG_SYS_FUNC_EN, (u16)(RegVal | BIT(13) | BIT(0) | BIT(1)));
 
 	/*  20090923 Joseph: Advised by Steven and Jenyu. Power sequence before init RF. */

drivers/staging/r8188eu/hal/usb_halinit.c +23 -4

index 5daef0117a5c..40d907dd2b58 100644
@@ -52,6 +52,8 @@ void rtl8188eu_interface_configure(struct adapter *adapt)
 u32 rtl8188eu_InitPowerOn(struct adapter *adapt)
 {
 	u16 value16;
+	int res;
+
 	/*  HW Power on sequence */
 	struct hal_data_8188e *haldata = &adapt->haldata;
 	if (haldata->bMacPwrCtrlOn)

@@ -65,7 +67,10 @@ u32 rtl8188eu_InitPowerOn(struct adapter *adapt)
 	rtw_write16(adapt, REG_CR, 0x00);  /* suggseted by zhouzhou, by page, 20111230 */
 
 		/*  Enable MAC DMA/WMAC/SCHEDULE/SEC block */
-	value16 = rtw_read16(adapt, REG_CR);
+	res = rtw_read16(adapt, REG_CR, &value16);
+	if (res)
+		return _FAIL;
+
 	value16 |= (HCI_TXDMA_EN | HCI_RXDMA_EN | TXDMA_EN | RXDMA_EN
 				| PROTOCOL_EN | SCHEDULE_EN | ENSEC | CALTMR_EN);
 	/*  for SDIO - Set CR bit10 to enable 32k calibration. Suggested by SD1 Gimmy. Added by tynli. 2011.08.31. */

@@ -166,7 +171,14 @@ static void _InitNormalChipRegPriority(struct adapter *Adapter, u16 beQ,
 				       u16 bkQ, u16 viQ, u16 voQ, u16 mgtQ,
 				       u16 hiQ)
 {
-	u16 value16	= (rtw_read16(Adapter, REG_TRXDMA_CTRL) & 0x7);
+	u16 value16;
+	int res;
+
+	res = rtw_read16(Adapter, REG_TRXDMA_CTRL, &value16);
+	if (res)
+		return;
+
+	value16 &= 0x7;
 
 	value16 |= _TXDMA_BEQ_MAP(beQ)	| _TXDMA_BKQ_MAP(bkQ) |
 		   _TXDMA_VIQ_MAP(viQ)	| _TXDMA_VOQ_MAP(voQ) |

@@ -631,7 +643,10 @@ u32 rtl8188eu_hal_init(struct adapter *Adapter)
 	/*  Hw bug which Hw initials RxFF boundary size to a value which is larger than the real Rx buffer size in 88E. */
 	/*  */
 	/*  Enable MACTXEN/MACRXEN block */
-	value16 = rtw_read16(Adapter, REG_CR);
+	res = rtw_read16(Adapter, REG_CR, &value16);
+	if (res)
+		return _FAIL;
+
 	value16 |= (MACTXEN | MACRXEN);
 	rtw_write8(Adapter, REG_CR, value16);

@@ -704,7 +719,11 @@ u32 rtl8188eu_hal_init(struct adapter *Adapter)
 	rtw_write16(Adapter, REG_TX_RPT_TIME, 0x3DF0);
 
 	/* enable tx DMA to drop the redundate data of packet */
-	rtw_write16(Adapter, REG_TXDMA_OFFSET_CHK, (rtw_read16(Adapter, REG_TXDMA_OFFSET_CHK) | DROP_DATA_EN));
+	res = rtw_read16(Adapter, REG_TXDMA_OFFSET_CHK, &value16);
+	if (res)
+		return _FAIL;
+
+	rtw_write16(Adapter, REG_TXDMA_OFFSET_CHK, (value16 | DROP_DATA_EN));
 
 	/*  2010/08/26 MH Merge from 8192CE. */
 	if (pwrctrlpriv->rf_pwrstate == rf_on) {

drivers/staging/r8188eu/hal/usb_ops_linux.c +9 -4

index f399a7fd8b97..7d62f1f3d26e 100644
@@ -103,16 +103,21 @@ int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data)
 	return usb_read(intf, value, data, 1);
 }
 
-u16 rtw_read16(struct adapter *adapter, u32 addr)
+int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intf = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	__le16 data;
+	__le16 le_data;
+	int res;
 
-	usb_read(intf, value, &data, 2);
+	res = usb_read(intf, value, &le_data, 2);
+	if (res)
+		return res;
 
-	return le16_to_cpu(data);
+	*data = le16_to_cpu(le_data);
+
+	return 0;
 }
 
 u32 rtw_read32(struct adapter *adapter, u32 addr)

drivers/staging/r8188eu/include/rtw_io.h +1 -1

index 1198d3850a6d..ce3369e33d66 100644
@@ -221,7 +221,7 @@ void _rtw_attrib_read(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
 int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
-u16 rtw_read16(struct adapter *adapter, u32 addr);
+int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data);
 u32 rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 rtw_read_port(struct adapter *adapter, u8 *pmem);

drivers/staging/r8188eu/os_dep/ioctl_linux.c +6 -3

index 6e5ee2a74ee4..6ed1439b940c 100644
@@ -3348,6 +3348,7 @@ static int rtw_dbg_port(struct net_device *dev,
 
 			/* FIXME: is this read necessary? */
 			res = rtw_read8(padapter, reg, &val8);
+			(void)res;
 		}
 			break;

@@ -3356,8 +3357,8 @@ static int rtw_dbg_port(struct net_device *dev,
 			u16 reg = arg;
 			u16 start_value = 200;
 			u32 write_num = extra_arg;
-
-			int i;
+			u16 val16;
+			int i, res;
 			struct xmit_frame	*xmit_frame;
 
 			xmit_frame = rtw_IOL_accquire_xmit_frame(padapter);

@@ -3371,7 +3372,9 @@ static int rtw_dbg_port(struct net_device *dev,
 			if (rtl8188e_IOL_exec_cmds_sync(padapter, xmit_frame, 5000, 0) != _SUCCESS)
 				ret = -EPERM;
 
-			rtw_read16(padapter, reg);
+			/* FIXME: is this read necessary? */
+			res = rtw_read16(padapter, reg, &val16);
+			(void)res;
 		}
 			break;
 		case 0x08: /* continuous write dword test */

drivers/staging/r8188eu/os_dep/os_intfs.c +5 -1

index 891c85b088ca..d9325ef6ac28 100644
@@ -740,12 +740,16 @@ static void rtw_fifo_cleanup(struct adapter *adapter)
 {
 	struct pwrctrl_priv *pwrpriv = &adapter->pwrctrlpriv;
 	u8 trycnt = 100;
+	int res;
 
 	/* pause tx */
 	rtw_write8(adapter, REG_TXPAUSE, 0xff);
 
 	/* keep sn */
-	adapter->xmitpriv.nqos_ssn = rtw_read16(adapter, REG_NQOS_SEQ);
+	/* FIXME: return an error to caller */
+	res = rtw_read16(adapter, REG_NQOS_SEQ, &adapter->xmitpriv.nqos_ssn);
+	if (res)
+		return;
 
 	if (!pwrpriv->bkeepfwalive) {
 		/* RX DMA stop */

Signature · 1 line

2.36.1

Pavel Skripkin

author

2022-05-19 21:10 UTC #7 lore raw

PATCH v2 3/4 staging: r8188eu: add error handling of rtw_read32

In reply to Pavel Skripkin

rtw_read32() reads data from device via USB API which may fail. In case of any failure previous code returned stack data to callers, which is wrong.

Fix it by changing rtw_read32() prototype and prevent caller from touching random stack data

Signed-off-by Pavel Skripkin <[email protected]>

---

Changes since v1:
- Fixed couple of infinity while loops
- Fixed style issues

---
 drivers/staging/r8188eu/core/rtw_cmd.c        | 15 +++++-
 drivers/staging/r8188eu/core/rtw_efuse.c      | 20 ++++---
 drivers/staging/r8188eu/core/rtw_fw.c         | 16 ++++--
 drivers/staging/r8188eu/core/rtw_mlme_ext.c   | 13 ++++-
 drivers/staging/r8188eu/core/rtw_pwrctrl.c    |  9 +++-
 .../r8188eu/hal/Hal8188ERateAdaptive.c        | 21 ++++++--
 drivers/staging/r8188eu/hal/HalPhyRf_8188e.c  |  3 +-
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 40 +++++++++-----
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c | 12 ++++-
 drivers/staging/r8188eu/hal/usb_halinit.c     | 53 ++++++++++++++++---
 drivers/staging/r8188eu/hal/usb_ops_linux.c   | 13 +++--
 drivers/staging/r8188eu/include/rtw_io.h      |  2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  | 27 ++++++++--
 drivers/staging/r8188eu/os_dep/os_intfs.c     | 13 ++++-
 14 files changed, 201 insertions(+), 56 deletions(-)

drivers/staging/r8188eu/core/rtw_cmd.c +13 -2

index 06523d91939a..5b6a891b5d67 100644
@@ -898,8 +898,12 @@ static void traffic_status_watchdog(struct adapter *padapter)
 static void rtl8188e_sreset_xmit_status_check(struct adapter *padapter)
 {
 	u32 txdma_status;
+	int res;
+
+	res = rtw_read32(padapter, REG_TXDMA_STATUS, &txdma_status);
+	if (res)
+		return;
 
-	txdma_status = rtw_read32(padapter, REG_TXDMA_STATUS);
 	if (txdma_status != 0x00)
 		rtw_write32(padapter, REG_TXDMA_STATUS, txdma_status);
 	/* total xmit irp = 4 */

@@ -1177,7 +1181,14 @@ u8 rtw_ps_cmd(struct adapter *padapter)
 
 static bool rtw_is_hi_queue_empty(struct adapter *adapter)
 {
-	return (rtw_read32(adapter, REG_HGQ_INFORMATION) & 0x0000ff00) == 0;
+	int res;
+	u32 reg;
+
+	res = rtw_read32(adapter, REG_HGQ_INFORMATION, &reg);
+	if (res)
+		return false;
+
+	return (reg & 0x0000ff00) == 0;
 }
 
 static void rtw_chk_hi_queue_hdl(struct adapter *padapter)

drivers/staging/r8188eu/core/rtw_efuse.c +14 -6

index a2691c7f96f6..8005ed8d3a20 100644
@@ -46,11 +46,17 @@ ReadEFuseByte(
 	rtw_write8(Adapter, EFUSE_CTRL + 3, (readbyte & 0x7f));
 
 	/* Check bit 32 read-ready */
-	retry = 0;
-	value32 = rtw_read32(Adapter, EFUSE_CTRL);
-	while (!(((value32 >> 24) & 0xff) & 0x80)  && (retry < 10000)) {
-		value32 = rtw_read32(Adapter, EFUSE_CTRL);
-		retry++;
+	res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+	if (res)
+		return;
+
+	for (retry = 0; retry < 10000; retry++) {
+		res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+		if (res)
+			continue;
+
+		if (((value32 >> 24) & 0xff) & 0x80)
+			break;
 	}
 
 	/*  20100205 Joseph: Add delay suggested by SD1 Victor. */

@@ -58,7 +64,9 @@ ReadEFuseByte(
 	/*  Designer says that there shall be some delay after ready bit is set, or the */
 	/*  result will always stay on last data we read. */
 	udelay(50);
-	value32 = rtw_read32(Adapter, EFUSE_CTRL);
+	res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+	if (res)
+		return;
 
 	*pbuf = (u8)(value32 & 0xff);

drivers/staging/r8188eu/core/rtw_fw.c +12 -4

index 638dc3ddfbcc..83a2f3476f6e 100644
@@ -194,10 +194,14 @@ static int fw_free_to_go(struct adapter *padapter)
 {
 	u32	counter = 0;
 	u32	value32;
+	int res;
 
 	/*  polling CheckSum report */
 	do {
-		value32 = rtw_read32(padapter, REG_MCUFWDL);
+		res = rtw_read32(padapter, REG_MCUFWDL, &value32);
+		if (res)
+			continue;
+
 		if (value32 & FWDL_CHKSUM_RPT)
 			break;
 	} while (counter++ < POLLING_READY_TIMEOUT_COUNT);

@@ -205,7 +209,10 @@ static int fw_free_to_go(struct adapter *padapter)
 	if (counter >= POLLING_READY_TIMEOUT_COUNT)
 		return _FAIL;
 
-	value32 = rtw_read32(padapter, REG_MCUFWDL);
+	res = rtw_read32(padapter, REG_MCUFWDL, &value32);
+	if (res)
+		return _FAIL;
+
 	value32 |= MCUFWDL_RDY;
 	value32 &= ~WINTINI_RDY;
 	rtw_write32(padapter, REG_MCUFWDL, value32);

@@ -215,9 +222,10 @@ static int fw_free_to_go(struct adapter *padapter)
 	/*  polling for FW ready */
 	counter = 0;
 	do {
-		value32 = rtw_read32(padapter, REG_MCUFWDL);
-		if (value32 & WINTINI_RDY)
+		res = rtw_read32(padapter, REG_MCUFWDL, &value32);
+		if (!res && value32 & WINTINI_RDY)
 			return _SUCCESS;
+
 		udelay(5);
 	} while (counter++ < POLLING_READY_TIMEOUT_COUNT);

drivers/staging/r8188eu/core/rtw_mlme_ext.c +11 -2

index d4e59fab367c..4bfe2441ee8d 100644
@@ -6022,6 +6022,7 @@ static void mlme_join(struct adapter *adapter, int type)
 {
 	struct mlme_priv *mlmepriv = &adapter->mlmepriv;
 	u8 retry_limit = 0x30, reg;
+	u32 reg32;
 	int res;
 
 	switch (type) {

@@ -6030,8 +6031,12 @@ static void mlme_join(struct adapter *adapter, int type)
 		/* enable to rx data frame, accept all data frame */
 		rtw_write16(adapter, REG_RXFLTMAP2, 0xFFFF);
 
+		res = rtw_read32(adapter, REG_RCR, &reg32);
+		if (res)
+			return;
+
 		rtw_write32(adapter, REG_RCR,
-			    rtw_read32(adapter, REG_RCR) | RCR_CBSSID_DATA | RCR_CBSSID_BCN);
+			    reg32 | RCR_CBSSID_DATA | RCR_CBSSID_BCN);
 
 		if (check_fwstate(mlmepriv, WIFI_STATION_STATE)) {
 			retry_limit = 48;

@@ -6844,10 +6849,14 @@ static u8 chk_ap_is_alive(struct sta_info *psta)
 
 static void rtl8188e_sreset_linked_status_check(struct adapter *padapter)
 {
-	u32 rx_dma_status =  rtw_read32(padapter, REG_RXDMA_STATUS);
+	u32 rx_dma_status;
 	int res;
 	u8 reg;
 
+	res = rtw_read32(padapter, REG_RXDMA_STATUS, &rx_dma_status);
+	if (res)
+		return;
+
 	if (rx_dma_status != 0x00)
 		rtw_write32(padapter, REG_RXDMA_STATUS, rx_dma_status);

drivers/staging/r8188eu/core/rtw_pwrctrl.c +8 -1

index 6990808ef353..1fe3d3d9cfb9 100644
@@ -229,6 +229,9 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 ps_mode, u8 smart_ps, u8 bcn_a
 
 static bool lps_rf_on(struct adapter *adapter)
 {
+	int res;
+	u32 reg;
+
 	/* When we halt NIC, we should check if FW LPS is leave. */
 	if (adapter->pwrctrlpriv.rf_pwrstate == rf_off) {
 		/*  If it is in HW/SW Radio OFF or IPS state, we do not check Fw LPS Leave, */

@@ -236,7 +239,11 @@ static bool lps_rf_on(struct adapter *adapter)
 		return true;
 	}
 
-	if (rtw_read32(adapter, REG_RCR) & 0x00070000)
+	res = rtw_read32(adapter, REG_RCR, &reg);
+	if (res)
+		return false;
+
+	if (reg & 0x00070000)
 		return false;
 
 	return true;

drivers/staging/r8188eu/hal/Hal8188ERateAdaptive.c +17 -4

index 57e8f5573846..3cefdf90d6e0 100644
@@ -279,6 +279,7 @@ static int odm_ARFBRefresh_8188E(struct odm_dm_struct *dm_odm, struct odm_ra_inf
 {  /*  Wilson 2011/10/26 */
 	u32 MaskFromReg;
 	s8 i;
+	int res;
 
 	switch (pRaInfo->RateID) {
 	case RATR_INX_WIRELESS_NGB:

@@ -303,19 +304,31 @@ static int odm_ARFBRefresh_8188E(struct odm_dm_struct *dm_odm, struct odm_ra_inf
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & 0x0000000d;
 		break;
 	case 12:
-		MaskFromReg = rtw_read32(dm_odm->Adapter, REG_ARFR0);
+		res = rtw_read32(dm_odm->Adapter, REG_ARFR0, &MaskFromReg);
+		if (res)
+			return res;
+
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & MaskFromReg;
 		break;
 	case 13:
-		MaskFromReg = rtw_read32(dm_odm->Adapter, REG_ARFR1);
+		res = rtw_read32(dm_odm->Adapter, REG_ARFR1, &MaskFromReg);
+		if (res)
+			return res;
+
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & MaskFromReg;
 		break;
 	case 14:
-		MaskFromReg = rtw_read32(dm_odm->Adapter, REG_ARFR2);
+		res = rtw_read32(dm_odm->Adapter, REG_ARFR2, &MaskFromReg);
+		if (res)
+			return res;
+
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & MaskFromReg;
 		break;
 	case 15:
-		MaskFromReg = rtw_read32(dm_odm->Adapter, REG_ARFR3);
+		res = rtw_read32(dm_odm->Adapter, REG_ARFR3, &MaskFromReg);
+		if (res)
+			return res;
+
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & MaskFromReg;
 		break;
 	default:

drivers/staging/r8188eu/hal/HalPhyRf_8188e.c +2 -1

index a5b7980dfcee..525deab10820 100644
@@ -483,7 +483,8 @@ static void _PHY_SaveMACRegisters(
 		MACBackup[i] = reg;
 	}
 
-	MACBackup[i] = rtw_read32(adapt, MACReg[i]);
+	res = rtw_read32(adapt, MACReg[i], MACBackup + i);
+	(void)res;
 }
 
 static void reload_adda_reg(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup, u32 RegiesterNum)

drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +26 ~~-14~~

index 6662ebc30f7b..c9464a826f7c 100644
@@ -215,6 +215,7 @@ static void efuse_read_phymap_from_txpktbuf(
 	u16 limit = *size;
 	u8 reg;
 	u8 *pos = content;
+	u32 reg32;
 
 	if (bcnhead < 0) { /* if not valid */
 		res = rtw_read8(adapter, REG_TDECTRL + 1, &reg);

@@ -245,8 +246,17 @@ static void efuse_read_phymap_from_txpktbuf(
 		} while (time_before(jiffies, timeout));
 
 		/* data from EEPROM needs to be in LE */
-		lo32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_L));
-		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));
+		res = rtw_read32(adapter, REG_PKTBUF_DBG_DATA_L, &reg32);
+		if (res)
+			return;
+
+		lo32 = cpu_to_le32(reg32);
+
+		res = rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H, &reg32);
+		if (res)
+			return;
+
+		hi32 = cpu_to_le32(reg32);
 
 		if (i == 0) {
 			u16 reg;

@@ -544,8 +554,12 @@ void rtl8188e_read_chip_version(struct adapter *padapter)
 	u32				value32;
 	struct HAL_VERSION		ChipVersion;
 	struct hal_data_8188e *pHalData = &padapter->haldata;
+	int res;
+
+	res = rtw_read32(padapter, REG_SYS_CFG, &value32);
+	if (res)
+		return;
 
-	value32 = rtw_read32(padapter, REG_SYS_CFG);
 	ChipVersion.ChipType = ((value32 & RTL_ID) ? TEST_CHIP : NORMAL_CHIP);
 
 	ChipVersion.VendorType = ((value32 & VENDOR_ID) ? CHIP_VENDOR_UMC : CHIP_VENDOR_TSMC);

@@ -592,26 +606,24 @@ void hal_notch_filter_8188e(struct adapter *adapter, bool enable)
 /*  */
 static s32 _LLTWrite(struct adapter *padapter, u32 address, u32 data)
 {
-	s32	status = _SUCCESS;
-	s32	count = 0;
+	s32	count;
 	u32	value = _LLT_INIT_ADDR(address) | _LLT_INIT_DATA(data) | _LLT_OP(_LLT_WRITE_ACCESS);
 	u16	LLTReg = REG_LLT_INIT;
+	int res;
 
 	rtw_write32(padapter, LLTReg, value);
 
 	/* polling */
-	do {
-		value = rtw_read32(padapter, LLTReg);
-		if (_LLT_NO_ACTIVE == _LLT_OP_VALUE(value))
-			break;
+	for (count = 0; count <= POLLING_LLT_THRESHOLD; count++) {
+		res = rtw_read32(padapter, LLTReg, &value);
+		if (res)
+			continue;
 
-		if (count > POLLING_LLT_THRESHOLD) {
-			status = _FAIL;
+		if (_LLT_NO_ACTIVE == _LLT_OP_VALUE(value))
 			break;
-		}
-	} while (count++);
+	}
 
-	return status;
+	return count > POLLING_LLT_THRESHOLD ? _FAIL : _SUCCESS;
 }
 
 s32 InitLLTTable(struct adapter *padapter, u8 txpktbuf_bndy)

drivers/staging/r8188eu/hal/rtl8188e_phycfg.c +10 -2

index 298c3d9bc7be..dea6d915a1f4 100644
@@ -56,8 +56,12 @@ rtl8188e_PHY_QueryBBReg(
 	)
 {
 	u32 ReturnValue = 0, OriginalValue, BitShift;
+	int res;
+
+	res = rtw_read32(Adapter, RegAddr, &OriginalValue);
+	if (res)
+		return 0;
 
-	OriginalValue = rtw_read32(Adapter, RegAddr);
 	BitShift = phy_CalculateBitShift(BitMask);
 	ReturnValue = (OriginalValue & BitMask) >> BitShift;
 	return ReturnValue;

@@ -84,9 +88,13 @@ rtl8188e_PHY_QueryBBReg(
 void rtl8188e_PHY_SetBBReg(struct adapter *Adapter, u32 RegAddr, u32 BitMask, u32 Data)
 {
 	u32 OriginalValue, BitShift;
+	int res;
 
 	if (BitMask != bMaskDWord) { /* if not "double word" write */
-		OriginalValue = rtw_read32(Adapter, RegAddr);
+		res = rtw_read32(Adapter, RegAddr, &OriginalValue);
+		if (res)
+			return;
+
 		BitShift = phy_CalculateBitShift(BitMask);
 		Data = ((OriginalValue & (~BitMask)) | (Data << BitShift));
 	}

drivers/staging/r8188eu/hal/usb_halinit.c +45 -8

124 lines not on this page · raw message

drivers/staging/r8188eu/hal/usb_ops_linux.c +9 -4

index 7d62f1f3d26e..c1a4d023f627 100644
@@ -120,16 +120,21 @@ int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data)
 	return 0;
 }
 
-u32 rtw_read32(struct adapter *adapter, u32 addr)
+int __must_check rtw_read32(struct adapter *adapter, u32 addr, u32 *data)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intf = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	__le32 data;
+	__le32 le_data;
+	int res;
 
-	usb_read(intf, value, &data, 4);
+	res = usb_read(intf, value, &le_data, 4);
+	if (res)
+		return res;
 
-	return le32_to_cpu(data);
+	*data = le32_to_cpu(le_data);
+
+	return 0;
 }
 
 int rtw_write8(struct adapter *adapter, u32 addr, u8 val)

drivers/staging/r8188eu/include/rtw_io.h +1 -1

index ce3369e33d66..1c6097367a67 100644
@@ -222,7 +222,7 @@ void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
 int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
 int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data);
-u32 rtw_read32(struct adapter *adapter, u32 addr);
+int __must_check rtw_read32(struct adapter *adapter, u32 addr, u32 *data);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 rtw_read_port(struct adapter *adapter, u8 *pmem);
 void rtw_read_port_cancel(struct adapter *adapter);

drivers/staging/r8188eu/os_dep/ioctl_linux.c +22 -5

index 6ed1439b940c..0b10ebb12c57 100644
@@ -3125,18 +3125,29 @@ static int rtw_rereg_nd_name(struct net_device *dev,
 static void mac_reg_dump(struct adapter *padapter)
 {
 	int i, j = 1;
+	u32 reg;
+	int res;
+
 	pr_info("\n ======= MAC REG =======\n");
 	for (i = 0x0; i < 0x300; i += 4) {
 		if (j % 4 == 1)
 			pr_info("0x%02x", i);
-		pr_info(" 0x%08x ", rtw_read32(padapter, i));
+
+		res = rtw_read32(padapter, i, &reg);
+		if (!res)
+			pr_info(" 0x%08x ", reg);
+
 		if ((j++) % 4 == 0)
 			pr_info("\n");
 	}
 	for (i = 0x400; i < 0x800; i += 4) {
 		if (j % 4 == 1)
 			pr_info("0x%02x", i);
-		pr_info(" 0x%08x ", rtw_read32(padapter, i));
+
+		res = rtw_read32(padapter, i, &reg);
+		if (!res)
+			pr_info(" 0x%08x ", reg);
+
 		if ((j++) % 4 == 0)
 			pr_info("\n");
 	}

@@ -3144,13 +3155,18 @@ static void mac_reg_dump(struct adapter *padapter)
 
 static void bb_reg_dump(struct adapter *padapter)
 {
-	int i, j = 1;
+	int i, j = 1, res;
+	u32 reg;
+
 	pr_info("\n ======= BB REG =======\n");
 	for (i = 0x800; i < 0x1000; i += 4) {
 		if (j % 4 == 1)
 			pr_info("0x%02x", i);
 
-		pr_info(" 0x%08x ", rtw_read32(padapter, i));
+		res = rtw_read32(padapter, i, &reg);
+		if (!res)
+			pr_info(" 0x%08x ", reg);
+
 		if ((j++) % 4 == 0)
 			pr_info("\n");
 	}

@@ -3397,7 +3413,8 @@ static int rtw_dbg_port(struct net_device *dev,
 			if (rtl8188e_IOL_exec_cmds_sync(padapter, xmit_frame, 5000, 0) != _SUCCESS)
 				ret = -EPERM;
 
-			rtw_read32(padapter, reg);
+			/* FIXME: is this read necessary? */
+			ret = rtw_read32(padapter, reg, &write_num);
 		}
 			break;
 		}

drivers/staging/r8188eu/os_dep/os_intfs.c +11 -2

index d9325ef6ac28..cac9553666e6 100644
@@ -741,6 +741,7 @@ static void rtw_fifo_cleanup(struct adapter *adapter)
 	struct pwrctrl_priv *pwrpriv = &adapter->pwrctrlpriv;
 	u8 trycnt = 100;
 	int res;
+	u32 reg;
 
 	/* pause tx */
 	rtw_write8(adapter, REG_TXPAUSE, 0xff);

@@ -753,10 +754,18 @@ static void rtw_fifo_cleanup(struct adapter *adapter)
 
 	if (!pwrpriv->bkeepfwalive) {
 		/* RX DMA stop */
+		res = rtw_read32(adapter, REG_RXPKT_NUM, &reg);
+		if (res)
+			return;
+
 		rtw_write32(adapter, REG_RXPKT_NUM,
-			    (rtw_read32(adapter, REG_RXPKT_NUM) | RW_RELEASE_EN));
+			    (reg | RW_RELEASE_EN));
 		do {
-			if (!(rtw_read32(adapter, REG_RXPKT_NUM) & RXDMA_IDLE))
+			res = rtw_read32(adapter, REG_RXPKT_NUM, &reg);
+			if (res)
+				continue;
+
+			if (!(reg & RXDMA_IDLE))
 				break;
 		} while (trycnt--);

Signature · 1 line

2.36.1

Dan Carpenter

Reviewed-by on the v2 series

2022-05-20 07:43 UTC #8 lore raw

In reply to Pavel Skripkin

This looks okay to me now. Thanks!

Reviewed-by Dan Carpenter <[email protected]>

regards,
dan carpenter

Greg KH

maintainer

2022-06-06 05:48 UTC #2 lore raw

Re: [PATCH v2 1/4] staging: r8188eu: add error handling of rtw_read8

In reply to Pavel Skripkin

Quoted 320 lines · Pavel Skripkin

rtw_read8() reads data from device via USB API which may fail. In case of any failure previous code returned stack data to callers, which is wrong.

Fix it by changing rtw_read8() prototype and prevent caller from touching random stack data

Signed-off-by Pavel Skripkin <[email protected]>

---

---
 drivers/staging/r8188eu/core/rtw_efuse.c      |  13 +-
 drivers/staging/r8188eu/core/rtw_fw.c         |  56 ++++--
 drivers/staging/r8188eu/core/rtw_led.c        |  16 +-
 drivers/staging/r8188eu/core/rtw_mlme_ext.c   |  49 +++++-
 drivers/staging/r8188eu/core/rtw_wlan_util.c  |  20 ++-
 drivers/staging/r8188eu/hal/HalPhyRf_8188e.c  |  18 +-
 drivers/staging/r8188eu/hal/HalPwrSeqCmd.c    |   9 +-
 drivers/staging/r8188eu/hal/hal_com.c         |  27 ++-
 drivers/staging/r8188eu/hal/rtl8188e_cmd.c    |  37 +++-
 drivers/staging/r8188eu/hal/rtl8188e_dm.c     |   6 +-
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   |  69 ++++++--
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  10 +-
 drivers/staging/r8188eu/hal/usb_halinit.c     | 160 ++++++++++++++----
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |   7 +-
 drivers/staging/r8188eu/include/rtw_io.h      |   2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  |  11 +-
 16 files changed, 410 insertions(+), 100 deletions(-)

drivers/staging/r8188eu/core/rtw_efuse.c +11 -2

index 0e0e60638880..a2691c7f96f6 100644
@@ -28,14 +28,21 @@ ReadEFuseByte(
 	u32 value32;
 	u8 readbyte;
 	u16 retry;
+	int res;
 
 	/* Write Address */
 	rtw_write8(Adapter, EFUSE_CTRL + 1, (_offset & 0xff));
-	readbyte = rtw_read8(Adapter, EFUSE_CTRL + 2);
+	res = rtw_read8(Adapter, EFUSE_CTRL + 2, &readbyte);
+	if (res)
+		return;
+
 	rtw_write8(Adapter, EFUSE_CTRL + 2, ((_offset >> 8) & 0x03) | (readbyte & 0xfc));
 
 	/* Write bit 32 0 */
-	readbyte = rtw_read8(Adapter, EFUSE_CTRL + 3);
+	res = rtw_read8(Adapter, EFUSE_CTRL + 3, &readbyte);
+	if (res)
+		return;
+
 	rtw_write8(Adapter, EFUSE_CTRL + 3, (readbyte & 0x7f));
 
 	/* Check bit 32 read-ready */

@@ -54,6 +61,8 @@ ReadEFuseByte(
 	value32 = rtw_read32(Adapter, EFUSE_CTRL);
 
 	*pbuf = (u8)(value32 & 0xff);
+
+	/* FIXME: return an error to caller */
 }
 
 /*-----------------------------------------------------------------------------

drivers/staging/r8188eu/core/rtw_fw.c +43 ~~-13~~

index bf077876ed3d..638dc3ddfbcc 100644
@@ -44,18 +44,28 @@ static_assert(sizeof(struct rt_firmware_hdr) == 32);
 static void fw_download_enable(struct adapter *padapter, bool enable)
 {
 	u8 tmp;
+	int res;
 
 	if (enable) {
 		/*  MCU firmware download enable. */
-		tmp = rtw_read8(padapter, REG_MCUFWDL);
+		res = rtw_read8(padapter, REG_MCUFWDL, &tmp);
+		if (res)
+			return;
+
 		rtw_write8(padapter, REG_MCUFWDL, tmp | 0x01);
 
 		/*  8051 reset */
-		tmp = rtw_read8(padapter, REG_MCUFWDL + 2);
+		res = rtw_read8(padapter, REG_MCUFWDL + 2, &tmp);
+		if (res)
+			return;
+
 		rtw_write8(padapter, REG_MCUFWDL + 2, tmp & 0xf7);
 	} else {
 		/*  MCU firmware download disable. */
-		tmp = rtw_read8(padapter, REG_MCUFWDL);
+		res = rtw_read8(padapter, REG_MCUFWDL, &tmp);
+		if (res)
+			return;
+
 		rtw_write8(padapter, REG_MCUFWDL, tmp & 0xfe);
 
 		/*  Reserved for fw extension. */

@@ -125,8 +135,13 @@ static int page_write(struct adapter *padapter, u32 page, u8 *buffer, u32 size)
 {
 	u8 value8;
 	u8 u8Page = (u8)(page & 0x07);
+	int res;
+
+	res = rtw_read8(padapter, REG_MCUFWDL + 2, &value8);
+	if (res)
+		return _FAIL;
 
-	value8 = (rtw_read8(padapter, REG_MCUFWDL + 2) & 0xF8) | u8Page;
+	value8 = (value8 & 0xF8) | u8Page;
 	rtw_write8(padapter, REG_MCUFWDL + 2, value8);
 
 	return block_write(padapter, buffer, size);

@@ -165,8 +180,12 @@ static int write_fw(struct adapter *padapter, u8 *buffer, u32 size)
 void rtw_reset_8051(struct adapter *padapter)
 {
 	u8 val8;
+	int res;
+
+	res = rtw_read8(padapter, REG_SYS_FUNC_EN + 1, &val8);
+	if (res)
+		return;
 
-	val8 = rtw_read8(padapter, REG_SYS_FUNC_EN + 1);
 	rtw_write8(padapter, REG_SYS_FUNC_EN + 1, val8 & (~BIT(2)));
 	rtw_write8(padapter, REG_SYS_FUNC_EN + 1, val8 | (BIT(2)));
 }

@@ -239,7 +258,7 @@ static int load_firmware(struct rt_firmware *rtfw, struct device *device)
 int rtl8188e_firmware_download(struct adapter *padapter)
 {
 	int ret = _SUCCESS;
-	u8 write_fw_retry = 0;
+	u8 reg;
 	unsigned long fwdl_timeout;
 	struct dvobj_priv *dvobj = adapter_to_dvobj(padapter);
 	struct device *device = dvobj_to_dev(dvobj);

@@ -269,23 +288,34 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 
 	/*  Suggested by Filen. If 8051 is running in RAM code, driver should inform Fw to reset by itself, */
 	/*  or it will cause download Fw fail. 2010.02.01. by tynli. */
-	if (rtw_read8(padapter, REG_MCUFWDL) & RAM_DL_SEL) { /* 8051 RAM code */
+	ret = rtw_read8(padapter, REG_MCUFWDL, &reg);
+	if (ret) {
+		ret = _FAIL;
+		goto exit;
+	}
+
+	if (reg & RAM_DL_SEL) { /* 8051 RAM code */
 		rtw_write8(padapter, REG_MCUFWDL, 0x00);
 		rtw_reset_8051(padapter);
 	}
 
 	fw_download_enable(padapter, true);
 	fwdl_timeout = jiffies + msecs_to_jiffies(500);
-	while (1) {
+	do {
 		/* reset the FWDL chksum */
-		rtw_write8(padapter, REG_MCUFWDL, rtw_read8(padapter, REG_MCUFWDL) | FWDL_CHKSUM_RPT);
+		ret = rtw_read8(padapter, REG_MCUFWDL, &reg);
+		if (ret) {
+			ret = _FAIL;
+			continue;
+		}
 
-		ret = write_fw(padapter, fw_data, fw_size);
+		rtw_write8(padapter, REG_MCUFWDL, reg | FWDL_CHKSUM_RPT);
 
-		if (ret == _SUCCESS ||
-		    (time_after(jiffies, fwdl_timeout) && write_fw_retry++ >= 3))
+		ret = write_fw(padapter, fw_data, fw_size);
+		if (ret == _SUCCESS)
 			break;
-	}
+	} while (!time_after(jiffies, fwdl_timeout));
+
 	fw_download_enable(padapter, false);
 	if (ret != _SUCCESS)
 		goto exit;

drivers/staging/r8188eu/core/rtw_led.c +13 -3

index 2f3000428af7..25989acf5259 100644
@@ -35,11 +35,15 @@ static void ResetLedStatus(struct LED_871x *pLed)
 static void SwLedOn(struct adapter *padapter, struct LED_871x *pLed)
 {
 	u8	LedCfg;
+	int res;
 
 	if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
 		return;
 
-	LedCfg = rtw_read8(padapter, REG_LEDCFG2);
+	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);
+	if (res)
+		return;
+
 	rtw_write8(padapter, REG_LEDCFG2, (LedCfg & 0xf0) | BIT(5) | BIT(6)); /*  SW control led0 on. */
 	pLed->bLedOn = true;
 }

@@ -47,15 +51,21 @@ static void SwLedOn(struct adapter *padapter, struct LED_871x *pLed)
 static void SwLedOff(struct adapter *padapter, struct LED_871x *pLed)
 {
 	u8	LedCfg;
+	int res;
 
 	if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
 		goto exit;
 
-	LedCfg = rtw_read8(padapter, REG_LEDCFG2);/* 0x4E */
+	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);/* 0x4E */
+	if (res)
+		goto exit;
 
 	LedCfg &= 0x90; /*  Set to software control. */
 	rtw_write8(padapter, REG_LEDCFG2, (LedCfg | BIT(3)));
-	LedCfg = rtw_read8(padapter, REG_MAC_PINMUX_CFG);
+	res = rtw_read8(padapter, REG_MAC_PINMUX_CFG, &LedCfg);
+	if (res)
+		goto exit;
+
 	LedCfg &= 0xFE;
 	rtw_write8(padapter, REG_MAC_PINMUX_CFG, LedCfg);
 exit:

drivers/staging/r8188eu/core/rtw_mlme_ext.c +32 -5

index 848b5051aa13..d4e59fab367c 100644
@@ -5672,14 +5672,28 @@ unsigned int send_beacon(struct adapter *padapter)
 
 bool get_beacon_valid_bit(struct adapter *adapter)
 {
+	int res;
+	u8 reg;
+
+	res = rtw_read8(adapter, REG_TDECTRL + 2, &reg);
+	if (res)
+		return false;
+
 	/* BIT(16) of REG_TDECTRL = BIT(0) of REG_TDECTRL+2 */
-	return BIT(0) & rtw_read8(adapter, REG_TDECTRL + 2);
+	return BIT(0) & reg;
 }
 
 void clear_beacon_valid_bit(struct adapter *adapter)
 {
+	int res;
+	u8 reg;
+
+	res = rtw_read8(adapter, REG_TDECTRL + 2, &reg);
+	if (res)
+		return;
+
 	/* BIT(16) of REG_TDECTRL = BIT(0) of REG_TDECTRL+2, write 1 to clear, Clear by sw */
-	rtw_write8(adapter, REG_TDECTRL + 2, rtw_read8(adapter, REG_TDECTRL + 2) | BIT(0));
+	rtw_write8(adapter, REG_TDECTRL + 2, reg | BIT(0));
 }
 
 /****************************************************************************

@@ -6007,7 +6021,8 @@ static void rtw_set_bssid(struct adapter *adapter, u8 *bssid)
 static void mlme_join(struct adapter *adapter, int type)
 {
 	struct mlme_priv *mlmepriv = &adapter->mlmepriv;
-	u8 retry_limit = 0x30;
+	u8 retry_limit = 0x30, reg;
+	int res;
 
 	switch (type) {
 	case 0:

@@ -6032,7 +6047,11 @@ static void mlme_join(struct adapter *adapter, int type)
 	case 2:
 		/* sta add event call back */
 		/* enable update TSF */
-		rtw_write8(adapter, REG_BCN_CTRL, rtw_read8(adapter, REG_BCN_CTRL) & (~BIT(4)));
+		res = rtw_read8(adapter, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapter, REG_BCN_CTRL, reg & (~BIT(4)));
 
 		if (check_fwstate(mlmepriv, WIFI_ADHOC_STATE | WIFI_ADHOC_MASTER_STATE))
 			retry_limit = 0x7;

@@ -6753,6 +6772,9 @@ void mlmeext_sta_add_event_callback(struct adapter *padapter, struct sta_info *p
 
 static void mlme_disconnect(struct adapter *adapter)
 {
+	int res;
+	u8 reg;
+
 	/* Set RCR to not to receive data frame when NO LINK state */
 	/* reject all data frames */
 	rtw_write16(adapter, REG_RXFLTMAP2, 0x00);

@@ -6761,7 +6783,12 @@ static void mlme_disconnect(struct adapter *adapter)
 	rtw_write8(adapter, REG_DUAL_TSF_RST, (BIT(0) | BIT(1)));
 
 	/* disable update TSF */
-	rtw_write8(adapter, REG_BCN_CTRL, rtw_read8(adapter, REG_BCN_CTRL) | BIT(4));
+
+	res = rtw_read8(adapter, REG_BCN_CTRL, &reg);
+	if (res)
+		return;
+
+	rtw_write8(adapter, REG_BCN_CTRL, reg | BIT(4));
 }
 
 void mlmeext_sta_del_event_callback(struct adapter *padapter)

@@ -6818,11 +6845,15 @@ static u8 chk_ap_is_alive(struct sta_info *psta)
 static void rtl8188e_sreset_linked_status_check(struct adapter *padapter)
 {
 	u32 rx_dma_status =  rtw_read32(padapter, REG_RXDMA_STATUS);

drivers/staging/r8188eu/core/rtw_mlme_ext.c static u8 chk_ap_is_alive(struct sta_info *psta) · line 6856

Quoted 10 lines

drivers/staging/r8188eu/core/rtw_mlme_ext.c +5 -1

+	int res;
+	u8 reg;
 
 	if (rx_dma_status != 0x00)
 		rtw_write32(padapter, REG_RXDMA_STATUS, rx_dma_status);
 
-	rtw_read8(padapter, REG_FMETHR);
+	/* FIXME: should this read be removed? */
+	res = rtw_read8(padapter, REG_FMETHR, &reg);
+	(void)res;

What is that? We don't do "empty" lines like this in the kernel for no good reason. If the return value must be checked, then that's fine, but don't do it this way. Shouldn't the function itself return an error?

And reading a value is sometimes required by hardware in order to have the write call go through. But that's for PCI devices, not normally USB devices, but we could be wrong. I wouldn't put a FIXME in here unless you have some plan for how to eventually solve it, otherwise someone might just drop it without knowing why the FIXME was ever added.

Quoted 82 lines

drivers/staging/r8188eu/core/rtw_mlme_ext.c +5 -1

 }
 
 void linked_status_chk(struct adapter *padapter)

@@ -7224,6 +7255,7 @@ u8 disconnect_hdl(struct adapter *padapter, unsigned char *pbuf)
 	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
 	struct wlan_bssid_ex *pnetwork = (struct wlan_bssid_ex *)(&pmlmeinfo->network);
 	u8 val8;
+	int res;
 
 	if (is_client_associated_to_ap(padapter))
 		issue_deauth_ex(padapter, pnetwork->MacAddress, WLAN_REASON_DEAUTH_LEAVING, param->deauth_timeout_ms / 100, 100);

@@ -7236,7 +7268,10 @@ u8 disconnect_hdl(struct adapter *padapter, unsigned char *pbuf)
 
 	if (((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE) || ((pmlmeinfo->state & 0x03) == WIFI_FW_AP_STATE)) {
 		/* Stop BCN */
-		val8 = rtw_read8(padapter, REG_BCN_CTRL);
+		res = rtw_read8(padapter, REG_BCN_CTRL, &val8);
+		if (res)
+			return H2C_DROPPED;
+
 		rtw_write8(padapter, REG_BCN_CTRL, val8 & (~(EN_BCN_FUNCTION | EN_TXBCN_RPT)));
 	}

drivers/staging/r8188eu/core/rtw_wlan_util.c +17 -3

index 27035eac6e61..f5002c88a5ac 100644
@@ -279,8 +279,13 @@ void Restore_DM_Func_Flag(struct adapter *padapter)
 void Set_MSR(struct adapter *padapter, u8 type)
 {
 	u8 val8;
+	int res;
 
-	val8 = rtw_read8(padapter, MSR) & 0x0c;
+	res = rtw_read8(padapter, MSR, &val8);
+	if (res)
+		return;
+
+	val8 &= 0x0c;
 	val8 |= type;
 	rtw_write8(padapter, MSR, val8);
 }

@@ -505,7 +510,11 @@ int WMM_param_handler(struct adapter *padapter, struct ndis_802_11_var_ie *pIE)
 
 static void set_acm_ctrl(struct adapter *adapter, u8 acm_mask)
 {
-	u8 acmctrl = rtw_read8(adapter, REG_ACMHWCTRL);
+	u8 acmctrl;
+	int res = rtw_read8(adapter, REG_ACMHWCTRL, &acmctrl);
+
+	if (res)
+		return;
 
 	if (acm_mask > 1)
 		acmctrl = acmctrl | 0x1;

@@ -763,6 +772,7 @@ void HT_info_handler(struct adapter *padapter, struct ndis_802_11_var_ie *pIE)
 static void set_min_ampdu_spacing(struct adapter *adapter, u8 spacing)
 {
 	u8 sec_spacing;
+	int res;
 
 	if (spacing <= 7) {
 		switch (adapter->securitypriv.dot11PrivacyAlgrthm) {

@@ -784,8 +794,12 @@ static void set_min_ampdu_spacing(struct adapter *adapter, u8 spacing)
 		if (spacing < sec_spacing)
 			spacing = sec_spacing;
 
+		res = rtw_read8(adapter, REG_AMPDU_MIN_SPACE, &sec_spacing);
+		if (res)
+			return;
+
 		rtw_write8(adapter, REG_AMPDU_MIN_SPACE,
-			   (rtw_read8(adapter, REG_AMPDU_MIN_SPACE) & 0xf8) | spacing);
+			   (sec_spacing & 0xf8) | spacing);
 	}
 }

drivers/staging/r8188eu/hal/HalPhyRf_8188e.c +0 -0

index b944c8071a3b..a5b7980dfcee 100644

drivers/staging/r8188eu/hal/HalPhyRf_8188e.c void _PHY_SaveADDARegisters(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup · line 466

Quoted 4 lines

drivers/staging/r8188eu/hal/HalPhyRf_8188e.c +1 -0

@@ -463,6 +463,7 @@ void _PHY_SaveADDARegisters(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup
 	}
 }
 
+/* FIXME: return an error to caller */

When are these FIXME going to be resolved? I don't like adding them for no good reason.

thanks,

greg k-h

Greg KH

maintainer

2022-06-06 05:48 UTC #5 lore raw

Re: [PATCH v2 2/4] staging: r8188eu: add error handling of rtw_read16

In reply to Pavel Skripkin

Quoted 30 lines · Pavel Skripkin

rtw_read16() reads data from device via USB API which may fail. In case of any failure previous code returned stack data to callers, which is wrong.
Fix it by changing rtw_read16() prototype and prevent caller from touching random stack data
Signed-off-by Pavel Skripkin <[email protected]>
---
Changes since v1:
- Fixed style issue
---
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 21 ++++++++++++---
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  8 ++++--
 drivers/staging/r8188eu/hal/usb_halinit.c     | 27 ++++++++++++++++---
 drivers/staging/r8188eu/hal/usb_ops_linux.c   | 13 ++++++---
 drivers/staging/r8188eu/include/rtw_io.h      |  2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  |  9 ++++---
 drivers/staging/r8188eu/os_dep/os_intfs.c     |  6 ++++-
 7 files changed, 67 insertions(+), 19 deletions(-)
drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +0 -0
index e67ecbd1ba79..6662ebc30f7b 100644
@@ -249,11 +249,14 @@ static void efuse_read_phymap_from_txpktbuf(
 		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));
 

drivers/staging/r8188eu/hal/rtl8188e_hal_init.c static void efuse_read_phymap_from_txpktbuf( · line 259

Quoted 10 lines

drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +4 -1

 		if (i == 0) {
+			u16 reg;
+
 			/* Although lenc is only used in a debug statement,
 			 * do not remove it as the rtw_read16() call consumes
 			 * 2 bytes from the EEPROM source.
 			 */
-			rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L);
+			res = rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L, &reg);
+			(void)res;

Same here, that line isn't ok.

thanks,

greg k-h

Pavel Skripkin

author

2022-06-06 18:57 UTC #3 lore raw

Re: [PATCH v2 1/4] staging: r8188eu: add error handling of rtw_read8

In reply to Greg KH

Hi Greg,

Quoted 9 lines · Greg KH

drivers/staging/r8188eu/core/rtw_mlme_ext.c static u8 chk_ap_is_alive(struct sta_info *psta) · line 6856
Quoted 4 lines
drivers/staging/r8188eu/core/rtw_mlme_ext.c +3 -1
-	rtw_read8(padapter, REG_FMETHR);
+	/* FIXME: should this read be removed? */
+	res = rtw_read8(padapter, REG_FMETHR, &reg);
+	(void)res;
What is that? We don't do "empty" lines like this in the kernel for no good reason. If the return value must be checked, then that's fine, but don't do it this way. Shouldn't the function itself return an error?

Quoted 7 lines

And reading a value is sometimes required by hardware in order to have the write call go through. But that's for PCI devices, not normally USB devices, but we could be wrong. I wouldn't put a FIXME in here unless you have some plan for how to eventually solve it, otherwise someone might just drop it without knowing why the FIXME was ever added.

Ok, I will just make this function return an error. Thanks

[snip]

Quoted 9 lines

drivers/staging/r8188eu/hal/HalPhyRf_8188e.c +0 -0

index b944c8071a3b..a5b7980dfcee 100644
@@ -463,6 +463,7 @@ void _PHY_SaveADDARegisters(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup
 	}
 }

Quoted 5 lines

drivers/staging/r8188eu/hal/HalPhyRf_8188e.c void _PHY_SaveADDARegisters(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup · line 466
Quoted 1 line
drivers/staging/r8188eu/hal/HalPhyRf_8188e.c +1 -0
+/* FIXME: return an error to caller */
When are these FIXME going to be resolved? I don't like adding them for no good reason.

After this series will go in. I really don't want to make this series huge, since ideally read errors should be passed up to callers. This driver has a lot of very deep call-chains, so fixing them in one series is just unreal

I have a plan to address these FIXMEs, that's why I've planted them.

thanks for review,

With regards,
Pavel Skripkin

Pavel Skripkin

author

2022-06-06 19:01 UTC #6 lore raw

Re: [PATCH v2 2/4] staging: r8188eu: add error handling of rtw_read16

In reply to Greg KH

Hi Greg,

Quoted 7 lines · Greg KH

Quoted 7 lines

drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +0 -0

index e67ecbd1ba79..6662ebc30f7b 100644
@@ -249,11 +249,14 @@ static void efuse_read_phymap_from_txpktbuf(
 		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));

Quoted 13 lines

drivers/staging/r8188eu/hal/rtl8188e_hal_init.c static void efuse_read_phymap_from_txpktbuf( · line 259
Quoted 10 lines
drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +4 -1
 		if (i == 0) {
+			u16 reg;
+
 			/* Although lenc is only used in a debug statement,
 			 * do not remove it as the rtw_read16() call consumes
 			 * 2 bytes from the EEPROM source.
 			 */
-			rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L);
+			res = rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L, &reg);
+			(void)res;
Same here, that line isn't ok.

hm, that place is not obvious.

I guess, on-chip EEPROM acts like a FIFO, so this read must be completed w/o any errors, so I will simply propagate an error to callers

And will place another "FIXME" for further work :)

thanks,

With regards,
Pavel Skripkin