LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic
@ 2021-08-22 14:28 Len Baker
  2021-08-22 14:28 ` [PATCH 1/2] staging/rtl8192u: Avoid CamelCase in names of variables Len Baker
  2021-08-22 14:28 ` [PATCH 2/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
  0 siblings, 2 replies; 7+ messages in thread
From: Len Baker @ 2021-08-22 14:28 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Len Baker, Kees Cook, Michael Straube, Lee Jones, linux-staging,
	linux-kernel, linux-hardening

The main reason of this patch serie is to avoid dynamic size
calculations (especially multiplication) in memory allocator 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.

However, there is a previous patch to avoid CamelCase in the name of
variables.

Len Baker (2):
  staging/rtl8192u: Avoid CamelCase in names of variables
  staging/rtl8192u: Prefer kcalloc over open coded arithmetic

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

--
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] staging/rtl8192u: Avoid CamelCase in names of variables
  2021-08-22 14:28 [PATCH 0/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
@ 2021-08-22 14:28 ` Len Baker
  2021-08-22 15:02   ` Kees Cook
  2021-08-22 14:28 ` [PATCH 2/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
  1 sibling, 1 reply; 7+ messages in thread
From: Len Baker @ 2021-08-22 14:28 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Len Baker, Kees Cook, Michael Straube, Lee Jones, linux-staging,
	linux-kernel, linux-hardening

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

Signed-off-by: Len Baker <len.baker@gmx.com>
---
 drivers/staging/rtl8192u/r819xU_phy.c | 92 +++++++++++++--------------
 1 file changed, 44 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/rtl8192u/r819xU_phy.c b/drivers/staging/rtl8192u/r819xU_phy.c
index 37b82553412e..ff6fe2ee3349 100644
--- a/drivers/staging/rtl8192u/r819xU_phy.c
+++ b/drivers/staging/rtl8192u/r819xU_phy.c
@@ -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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic
  2021-08-22 14:28 [PATCH 0/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
  2021-08-22 14:28 ` [PATCH 1/2] staging/rtl8192u: Avoid CamelCase in names of variables Len Baker
@ 2021-08-22 14:28 ` Len Baker
  2021-08-22 14:59   ` Kees Cook
  1 sibling, 1 reply; 7+ messages in thread
From: Len Baker @ 2021-08-22 14:28 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Len Baker, Kees Cook, Michael Straube, Lee Jones, linux-staging,
	linux-kernel, linux-hardening

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 <len.baker@gmx.com>
---
 drivers/staging/rtl8192u/r819xU_phy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8192u/r819xU_phy.c b/drivers/staging/rtl8192u/r819xU_phy.c
index ff6fe2ee3349..97f4d89500ae 100644
--- a/drivers/staging/rtl8192u/r819xU_phy.c
+++ b/drivers/staging/rtl8192u/r819xU_phy.c
@@ -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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic
  2021-08-22 14:28 ` [PATCH 2/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
@ 2021-08-22 14:59   ` Kees Cook
  2021-08-22 15:41     ` Len Baker
  0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-08-22 14:59 UTC (permalink / raw)
  To: Len Baker
  Cc: Greg Kroah-Hartman, Michael Straube, Lee Jones, linux-staging,
	linux-kernel, linux-hardening

On Sun, Aug 22, 2021 at 04:28:20PM +0200, Len Baker wrote:
> 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.

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 <keescook@chromium.org>

> 
> Signed-off-by: Len Baker <len.baker@gmx.com>
> ---
>  drivers/staging/rtl8192u/r819xU_phy.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/r819xU_phy.c b/drivers/staging/rtl8192u/r819xU_phy.c
> index ff6fe2ee3349..97f4d89500ae 100644
> --- a/drivers/staging/rtl8192u/r819xU_phy.c
> +++ b/drivers/staging/rtl8192u/r819xU_phy.c
> @@ -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
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] staging/rtl8192u: Avoid CamelCase in names of variables
  2021-08-22 14:28 ` [PATCH 1/2] staging/rtl8192u: Avoid CamelCase in names of variables Len Baker
@ 2021-08-22 15:02   ` Kees Cook
  2021-08-23  8:54     ` Dan Carpenter
  0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-08-22 15:02 UTC (permalink / raw)
  To: Len Baker
  Cc: Greg Kroah-Hartman, Michael Straube, Lee Jones, linux-staging,
	linux-kernel, linux-hardening

On Sun, Aug 22, 2021 at 04:28:19PM +0200, Len Baker wrote:
> 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?

-Kees

> 
> Signed-off-by: Len Baker <len.baker@gmx.com>
> ---
>  drivers/staging/rtl8192u/r819xU_phy.c | 92 +++++++++++++--------------
>  1 file changed, 44 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/r819xU_phy.c b/drivers/staging/rtl8192u/r819xU_phy.c
> index 37b82553412e..ff6fe2ee3349 100644
> --- a/drivers/staging/rtl8192u/r819xU_phy.c
> +++ b/drivers/staging/rtl8192u/r819xU_phy.c
> @@ -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
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic
  2021-08-22 14:59   ` Kees Cook
@ 2021-08-22 15:41     ` Len Baker
  0 siblings, 0 replies; 7+ messages in thread
From: Len Baker @ 2021-08-22 15:41 UTC (permalink / raw)
  To: Kees Cook
  Cc: Len Baker, Greg Kroah-Hartman, Michael Straube, Lee Jones,
	linux-staging, linux-kernel, linux-hardening

Hi Kees,

On Sun, Aug 22, 2021 at 07:59:51AM -0700, Kees Cook wrote:
> On Sun, Aug 22, 2021 at 04:28:20PM +0200, Len Baker wrote:
> > 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.
>
> 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

Ok, I will add this info to the next version. Thanks for the advise.

> 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.

Ok, I will change the commit message to note this. Also I will send
a patch to add this info to the documentation.

> 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

I think my script is even worst ;) but find some arithmetic to improve :)
I will take a look. Thanks for the info.

> Reviewed-by: Kees Cook <keescook@chromium.org>
>

Regards,
Len

> >
> > Signed-off-by: Len Baker <len.baker@gmx.com>
> > ---
> >  drivers/staging/rtl8192u/r819xU_phy.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192u/r819xU_phy.c b/drivers/staging/rtl8192u/r819xU_phy.c
> > index ff6fe2ee3349..97f4d89500ae 100644
> > --- a/drivers/staging/rtl8192u/r819xU_phy.c
> > +++ b/drivers/staging/rtl8192u/r819xU_phy.c
> > @@ -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
> >
>
> --
> Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] staging/rtl8192u: Avoid CamelCase in names of variables
  2021-08-22 15:02   ` Kees Cook
@ 2021-08-23  8:54     ` Dan Carpenter
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-08-23  8:54 UTC (permalink / raw)
  To: Kees Cook
  Cc: Len Baker, Greg Kroah-Hartman, Michael Straube, Lee Jones,
	linux-staging, linux-kernel, linux-hardening

On Sun, Aug 22, 2021 at 08:02:14AM -0700, Kees Cook wrote:
> On Sun, Aug 22, 2021 at 04:28:19PM +0200, Len Baker wrote:
> > 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.

Yep.  Absolutely verboten on staging.

> 
> 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?

It doesn't matter.  One function at a time is fine.  So long as it's not
too long to review.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-23  8:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-22 14:28 [PATCH 0/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
2021-08-22 14:28 ` [PATCH 1/2] staging/rtl8192u: Avoid CamelCase in names of variables Len Baker
2021-08-22 15:02   ` Kees Cook
2021-08-23  8:54     ` Dan Carpenter
2021-08-22 14:28 ` [PATCH 2/2] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
2021-08-22 14:59   ` Kees Cook
2021-08-22 15:41     ` Len Baker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).