staging/rtl8192u: Prefer kcalloc over open coded arithmetic

Avoid CameCase in the names of all local variables inside the function rtl8192_phy_SwChnlStepByStep().

Signed-off-by Len Baker <[email protected]>

---
 drivers/staging/rtl8192u/r819xU_phy.c | 92 +++++++++++++--------------
 1 file changed, 44 insertions(+), 48 deletions(-)

drivers/staging/rtl8192u/r819xU_phy.c +44 -49

index 37b82553412e..ff6fe2ee3349 100644
@@ -1185,30 +1185,30 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 				       u8 *stage, u8 *step, u32 *delay)
 {
 	struct r8192_priv *priv = ieee80211_priv(dev);
-	struct sw_chnl_cmd   *PreCommonCmd;
-	u32		   PreCommonCmdCnt;
-	struct sw_chnl_cmd   *PostCommonCmd;
-	u32		   PostCommonCmdCnt;
-	struct sw_chnl_cmd   *RfDependCmd;
-	u32		   RfDependCmdCnt;
-	struct sw_chnl_cmd  *CurrentCmd = NULL;
-	u8		   e_rfpath;
-	bool		   ret;
-
-	PreCommonCmd = kzalloc(sizeof(*PreCommonCmd) * MAX_PRECMD_CNT, GFP_KERNEL);
-	if (!PreCommonCmd)
+	struct sw_chnl_cmd *pre_cmd;
+	u32 pre_cmd_cnt = 0;
+	struct sw_chnl_cmd *post_cmd;
+	u32 post_cmd_cnt = 0;
+	struct sw_chnl_cmd *rf_cmd;
+	u32 rf_cmd_cnt = 0;
+	struct sw_chnl_cmd *current_cmd = NULL;
+	u8 e_rfpath;
+	bool ret;
+
+	pre_cmd = kzalloc(sizeof(*pre_cmd) * MAX_PRECMD_CNT, GFP_KERNEL);
+	if (!pre_cmd)
 		return false;
 
-	PostCommonCmd = kzalloc(sizeof(*PostCommonCmd) * MAX_POSTCMD_CNT, GFP_KERNEL);
-	if (!PostCommonCmd) {
-		kfree(PreCommonCmd);
+	post_cmd = kzalloc(sizeof(*post_cmd) * MAX_POSTCMD_CNT, GFP_KERNEL);
+	if (!post_cmd) {
+		kfree(pre_cmd);
 		return false;
 	}
 
-	RfDependCmd = kzalloc(sizeof(*RfDependCmd) * MAX_RFDEPENDCMD_CNT, GFP_KERNEL);
-	if (!RfDependCmd) {
-		kfree(PreCommonCmd);
-		kfree(PostCommonCmd);
+	rf_cmd = kzalloc(sizeof(*rf_cmd) * MAX_RFDEPENDCMD_CNT, GFP_KERNEL);
+	if (!rf_cmd) {
+		kfree(pre_cmd);
+		kfree(post_cmd);
 		return false;
 	}
 

@@ -1225,21 +1225,17 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 	/* FIXME: need to check whether channel is legal or not here */
 
 	/* <1> Fill up pre common command. */
-	PreCommonCmdCnt = 0;
-	rtl8192_phy_SetSwChnlCmdArray(PreCommonCmd, PreCommonCmdCnt++,
+	rtl8192_phy_SetSwChnlCmdArray(pre_cmd, pre_cmd_cnt++,
 				      MAX_PRECMD_CNT, CMD_ID_SET_TX_PWR_LEVEL,
 				      0, 0, 0);
-	rtl8192_phy_SetSwChnlCmdArray(PreCommonCmd, PreCommonCmdCnt++,
+	rtl8192_phy_SetSwChnlCmdArray(pre_cmd, pre_cmd_cnt++,
 				      MAX_PRECMD_CNT, CMD_ID_END, 0, 0, 0);
 
 	/* <2> Fill up post common command. */
-	PostCommonCmdCnt = 0;
-
-	rtl8192_phy_SetSwChnlCmdArray(PostCommonCmd, PostCommonCmdCnt++,
+	rtl8192_phy_SetSwChnlCmdArray(post_cmd, post_cmd_cnt++,
 				      MAX_POSTCMD_CNT, CMD_ID_END, 0, 0, 0);
 
 	/* <3> Fill up RF dependent command. */
-	RfDependCmdCnt = 0;
 	switch (priv->rf_chip) {
 	case RF_8225:
 		if (!(channel >= 1 && channel <= 14)) {

@@ -1249,13 +1245,13 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 			ret = true;
 			goto out;
 		}
-		rtl8192_phy_SetSwChnlCmdArray(RfDependCmd, RfDependCmdCnt++,
+		rtl8192_phy_SetSwChnlCmdArray(rf_cmd, rf_cmd_cnt++,
 					      MAX_RFDEPENDCMD_CNT,
 					      CMD_ID_RF_WRITE_REG,
 					      rZebra1_Channel,
 					      RF_CHANNEL_TABLE_ZEBRA[channel],
 					      10);
-		rtl8192_phy_SetSwChnlCmdArray(RfDependCmd, RfDependCmdCnt++,
+		rtl8192_phy_SetSwChnlCmdArray(rf_cmd, rf_cmd_cnt++,
 					      MAX_RFDEPENDCMD_CNT,
 					      CMD_ID_END, 0, 0, 0);
 		break;

@@ -1269,11 +1265,11 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 			ret = true;
 			goto out;
 		}
-		rtl8192_phy_SetSwChnlCmdArray(RfDependCmd, RfDependCmdCnt++,
+		rtl8192_phy_SetSwChnlCmdArray(rf_cmd, rf_cmd_cnt++,
 					      MAX_RFDEPENDCMD_CNT,
 					      CMD_ID_RF_WRITE_REG,
 					      rZebra1_Channel, channel, 10);
-		rtl8192_phy_SetSwChnlCmdArray(RfDependCmd, RfDependCmdCnt++,
+		rtl8192_phy_SetSwChnlCmdArray(rf_cmd, rf_cmd_cnt++,
 					      MAX_RFDEPENDCMD_CNT,
 					      CMD_ID_END, 0, 0, 0);
 		break;

@@ -1290,19 +1286,19 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 	do {
 		switch (*stage) {
 		case 0:
-			CurrentCmd = &PreCommonCmd[*step];
+			current_cmd = &pre_cmd[*step];
 			break;
 		case 1:
-			CurrentCmd = &RfDependCmd[*step];
+			current_cmd = &rf_cmd[*step];
 			break;
 		case 2:
-			CurrentCmd = &PostCommonCmd[*step];
+			current_cmd = &post_cmd[*step];
 			break;
 		}
 
-		if (CurrentCmd->cmd_id == CMD_ID_END) {
+		if (current_cmd->cmd_id == CMD_ID_END) {
 			if ((*stage) == 2) {
-				(*delay) = CurrentCmd->ms_delay;
+				*delay = current_cmd->ms_delay;
 				ret = true;
 				goto out;
 			}

@@ -1311,31 +1307,31 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 			continue;
 		}
 
-		switch (CurrentCmd->cmd_id) {
+		switch (current_cmd->cmd_id) {
 		case CMD_ID_SET_TX_PWR_LEVEL:
 			if (priv->card_8192_version == VERSION_819XU_A)
 				/* consider it later! */
 				rtl8192_SetTxPowerLevel(dev, channel);
 			break;
 		case CMD_ID_WRITE_PORT_ULONG:
-			write_nic_dword(dev, CurrentCmd->para_1,
-					CurrentCmd->para_2);
+			write_nic_dword(dev, current_cmd->para_1,
+					current_cmd->para_2);
 			break;
 		case CMD_ID_WRITE_PORT_USHORT:
-			write_nic_word(dev, CurrentCmd->para_1,
-				       (u16)CurrentCmd->para_2);
+			write_nic_word(dev, current_cmd->para_1,
+				       (u16)current_cmd->para_2);
 			break;
 		case CMD_ID_WRITE_PORT_UCHAR:
-			write_nic_byte(dev, CurrentCmd->para_1,
-				       (u8)CurrentCmd->para_2);
+			write_nic_byte(dev, current_cmd->para_1,
+				       (u8)current_cmd->para_2);
 			break;
 		case CMD_ID_RF_WRITE_REG:
 			for (e_rfpath = 0; e_rfpath < RF90_PATH_MAX; e_rfpath++) {
 				rtl8192_phy_SetRFReg(dev,
 						     (enum rf90_radio_path_e)e_rfpath,
-						     CurrentCmd->para_1,
+						     current_cmd->para_1,
 						     bZebra1_ChannelNum,
-						     CurrentCmd->para_2);
+						     current_cmd->para_2);
 			}
 			break;
 		default:

@@ -1345,14 +1341,14 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 		break;
 	} while (true);
 
-	(*delay) = CurrentCmd->ms_delay;
+	*delay = current_cmd->ms_delay;
 	(*step)++;
 	ret = false;
 
 out:
-	kfree(PreCommonCmd);
-	kfree(PostCommonCmd);
-	kfree(RfDependCmd);
+	kfree(pre_cmd);
+	kfree(post_cmd);
+	kfree(rf_cmd);
 
 	return ret;
 }
--

2.25.1

Quoted 2 lines · Len Baker

Avoid CameCase in the names of all local variables inside the function rtl8192_phy_SwChnlStepByStep().

This mixes decamelization with some (minor) logic changes (moving initializations earlier). I'd normally do this kind of thing with a sed script and not touch anything else, so that the results can be compared against the sed command. And I'd include the sed command in the commit log.

I'm actually not sure what the norm is in the kernel for doing decamelization -- should the entire driver get decamelized at once, instead of just one function at a time? Greg, do you have an opinion here?

Dynamic size calculations (especially multiplication) should not be performed in memory allocator (or similar) function arguments due to the risk of them overflowing. This could lead to values wrapping around and a smaller allocation being made than the caller was expecting. Using those allocations could lead to linear overflows of heap memory and other misbehaviors.

So, use the purpose specific kcalloc() function instead of the argument size * count in the kzalloc() function.

Signed-off-by Len Baker <[email protected]>

---
 drivers/staging/rtl8192u/r819xU_phy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/staging/rtl8192u/r819xU_phy.c +3 -4

index ff6fe2ee3349..97f4d89500ae 100644
@@ -1195,17 +1195,17 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 	u8 e_rfpath;
 	bool ret;
 
-	pre_cmd = kzalloc(sizeof(*pre_cmd) * MAX_PRECMD_CNT, GFP_KERNEL);
+	pre_cmd = kcalloc(MAX_PRECMD_CNT, sizeof(*pre_cmd), GFP_KERNEL);
 	if (!pre_cmd)
 		return false;
 
-	post_cmd = kzalloc(sizeof(*post_cmd) * MAX_POSTCMD_CNT, GFP_KERNEL);
+	post_cmd = kcalloc(MAX_POSTCMD_CNT, sizeof(*post_cmd), GFP_KERNEL);
 	if (!post_cmd) {
 		kfree(pre_cmd);
 		return false;
 	}
 
-	rf_cmd = kzalloc(sizeof(*rf_cmd) * MAX_RFDEPENDCMD_CNT, GFP_KERNEL);
+	rf_cmd = kcalloc(MAX_RFDEPENDCMD_CNT, sizeof(*rf_cmd), GFP_KERNEL);
 	if (!rf_cmd) {
 		kfree(pre_cmd);
 		kfree(post_cmd);
--

2.25.1

Quoted 7 lines · Len Baker

Dynamic size calculations (especially multiplication) should not be performed in memory allocator (or similar) function arguments due to the risk of them overflowing. This could lead to values wrapping around and a smaller allocation being made than the caller was expecting. Using those allocations could lead to linear overflows of heap memory and other misbehaviors.

Quoted 2 lines

So, use the purpose specific kcalloc() function instead of the argument size * count in the kzalloc() function.

It might be useful to reference the documentation on why this change is desired:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments

Here and in the docs, though, it's probably worth noting that these aren't actually dynamic sizes: both sides of the multiplication are constant values. I still think it's best to refactor these anyway, just to keep the open-coded math idiom out of code, though.

Also, have you looked at Coccinelle at all? I have a hideous pile of rules that try to find these instances, but it really needs improvement:
https://github.com/kees/coccinelle-linux-allocator-overflow/tree/trunk/array_size

Reviewed-by Kees Cook <[email protected]>

Quoted 32 lines

Signed-off-by Len Baker <[email protected]>

---
 drivers/staging/rtl8192u/r819xU_phy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/staging/rtl8192u/r819xU_phy.c +3 -4

index ff6fe2ee3349..97f4d89500ae 100644
@@ -1195,17 +1195,17 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
 	u8 e_rfpath;
 	bool ret;
 
-	pre_cmd = kzalloc(sizeof(*pre_cmd) * MAX_PRECMD_CNT, GFP_KERNEL);
+	pre_cmd = kcalloc(MAX_PRECMD_CNT, sizeof(*pre_cmd), GFP_KERNEL);
 	if (!pre_cmd)
 		return false;
 
-	post_cmd = kzalloc(sizeof(*post_cmd) * MAX_POSTCMD_CNT, GFP_KERNEL);
+	post_cmd = kcalloc(MAX_POSTCMD_CNT, sizeof(*post_cmd), GFP_KERNEL);
 	if (!post_cmd) {
 		kfree(pre_cmd);
 		return false;
 	}
 
-	rf_cmd = kzalloc(sizeof(*rf_cmd) * MAX_RFDEPENDCMD_CNT, GFP_KERNEL);
+	rf_cmd = kcalloc(MAX_RFDEPENDCMD_CNT, sizeof(*rf_cmd), GFP_KERNEL);
 	if (!rf_cmd) {
 		kfree(pre_cmd);
 		kfree(post_cmd);
--

Quoted 2 lines

2.25.1

--
Kees Cook