* [PATCH v2 0/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic
@ 2021-08-24 7:25 Len Baker
2021-08-24 7:25 ` [PATCH v2 1/3] staging/rtl8192u: Avoid CamelCase in names of variables Len Baker
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Len Baker @ 2021-08-24 7:25 UTC (permalink / raw)
To: Greg Kroah-Hartman, Kees Cook
Cc: Len Baker, Dan Carpenter, Michael Straube, Lee Jones,
linux-staging, linux-kernel, linux-hardening
The main reason of this patch serie is to avoid size calculations
(especially multiplication) in memory allocator function arguments due
to the risk of them overflowing [1]. 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.
At the same time, take the opportunity to refactor the function avoiding
CamelCase in the name of variables and moving some initializations to
the variables definition block.
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Changelog v1 -> v2
- Split the CamelCase refactoring and the moving of initializations in
two separate commits (Kees Cook).
- Link to the documentation to clarify the change in the 3/3 patch (Kees
Cook).
- Modify the commit message to clarify in the 3/3 patch that these
changes are not dynamic sizes but it is best to do the change to keep
the open-coded math idiom out of code (Kees Cook).
Len Baker (3):
staging/rtl8192u: Avoid CamelCase in names of variables
staging/rtl8192u: Initialize variables in the definition block
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] 4+ messages in thread
* [PATCH v2 1/3] staging/rtl8192u: Avoid CamelCase in names of variables
2021-08-24 7:25 [PATCH v2 0/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
@ 2021-08-24 7:25 ` Len Baker
2021-08-24 8:59 ` [PATCH v2 2/3] staging/rtl8192u: Initialize variables in the definition block Len Baker
2021-08-24 9:00 ` [PATCH v2 3/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
2 siblings, 0 replies; 4+ messages in thread
From: Len Baker @ 2021-08-24 7:25 UTC (permalink / raw)
To: Greg Kroah-Hartman, Kees Cook
Cc: Len Baker, Dan Carpenter, 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 | 95 +++++++++++++--------------
1 file changed, 47 insertions(+), 48 deletions(-)
diff --git a/drivers/staging/rtl8192u/r819xU_phy.c b/drivers/staging/rtl8192u/r819xU_phy.c
index 37b82553412e..6a67708cdd89 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;
+ struct sw_chnl_cmd *post_cmd;
+ u32 post_cmd_cnt;
+ struct sw_chnl_cmd *rf_cmd;
+ u32 rf_cmd_cnt;
+ 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,20 @@ 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++,
+ pre_cmd_cnt = 0;
+ 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++,
+ post_cmd_cnt = 0;
+ 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;
+ rf_cmd_cnt = 0;
switch (priv->rf_chip) {
case RF_8225:
if (!(channel >= 1 && channel <= 14)) {
@@ -1249,13 +1248,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 +1268,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 +1289,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 +1310,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 +1344,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 related [flat|nested] 4+ messages in thread
* [PATCH v2 2/3] staging/rtl8192u: Initialize variables in the definition block
2021-08-24 7:25 [PATCH v2 0/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
2021-08-24 7:25 ` [PATCH v2 1/3] staging/rtl8192u: Avoid CamelCase in names of variables Len Baker
@ 2021-08-24 8:59 ` Len Baker
2021-08-24 9:00 ` [PATCH v2 3/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
2 siblings, 0 replies; 4+ messages in thread
From: Len Baker @ 2021-08-24 8:59 UTC (permalink / raw)
To: Greg Kroah-Hartman, Kees Cook
Cc: Len Baker, Dan Carpenter, Michael Straube, Lee Jones,
linux-staging, linux-kernel, linux-hardening
Initialize the pre_cmd_cnt, post_cmd_cnt and rf_cmd_cnt variables in the
definition block as it is not necessary to do this in the middle of the
function.
Signed-off-by: Len Baker <len.baker@gmx.com>
---
drivers/staging/rtl8192u/r819xU_phy.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/rtl8192u/r819xU_phy.c b/drivers/staging/rtl8192u/r819xU_phy.c
index 6a67708cdd89..ff6fe2ee3349 100644
--- a/drivers/staging/rtl8192u/r819xU_phy.c
+++ b/drivers/staging/rtl8192u/r819xU_phy.c
@@ -1186,11 +1186,11 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
{
struct r8192_priv *priv = ieee80211_priv(dev);
struct sw_chnl_cmd *pre_cmd;
- u32 pre_cmd_cnt;
+ u32 pre_cmd_cnt = 0;
struct sw_chnl_cmd *post_cmd;
- u32 post_cmd_cnt;
+ u32 post_cmd_cnt = 0;
struct sw_chnl_cmd *rf_cmd;
- u32 rf_cmd_cnt;
+ u32 rf_cmd_cnt = 0;
struct sw_chnl_cmd *current_cmd = NULL;
u8 e_rfpath;
bool ret;
@@ -1225,7 +1225,6 @@ 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. */
- pre_cmd_cnt = 0;
rtl8192_phy_SetSwChnlCmdArray(pre_cmd, pre_cmd_cnt++,
MAX_PRECMD_CNT, CMD_ID_SET_TX_PWR_LEVEL,
0, 0, 0);
@@ -1233,12 +1232,10 @@ static u8 rtl8192_phy_SwChnlStepByStep(struct net_device *dev, u8 channel,
MAX_PRECMD_CNT, CMD_ID_END, 0, 0, 0);
/* <2> Fill up post common command. */
- post_cmd_cnt = 0;
rtl8192_phy_SetSwChnlCmdArray(post_cmd, post_cmd_cnt++,
MAX_POSTCMD_CNT, CMD_ID_END, 0, 0, 0);
/* <3> Fill up RF dependent command. */
- rf_cmd_cnt = 0;
switch (priv->rf_chip) {
case RF_8225:
if (!(channel >= 1 && channel <= 14)) {
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 3/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic
2021-08-24 7:25 [PATCH v2 0/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
2021-08-24 7:25 ` [PATCH v2 1/3] staging/rtl8192u: Avoid CamelCase in names of variables Len Baker
2021-08-24 8:59 ` [PATCH v2 2/3] staging/rtl8192u: Initialize variables in the definition block Len Baker
@ 2021-08-24 9:00 ` Len Baker
2 siblings, 0 replies; 4+ messages in thread
From: Len Baker @ 2021-08-24 9:00 UTC (permalink / raw)
To: Greg Kroah-Hartman, Kees Cook
Cc: Len Baker, Dan Carpenter, Michael Straube, Lee Jones,
linux-staging, linux-kernel, linux-hardening
As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], 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.
In this case these aren't actually dynamic sizes: both sides of the
multiplication are constant values. However it is best to refactor these
anyway, just to keep the open-coded math idiom out of code.
So, use the purpose specific kcalloc() function instead of the argument
size * count in the kzalloc() function.
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-24 9:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 7:25 [PATCH v2 0/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic Len Baker
2021-08-24 7:25 ` [PATCH v2 1/3] staging/rtl8192u: Avoid CamelCase in names of variables Len Baker
2021-08-24 8:59 ` [PATCH v2 2/3] staging/rtl8192u: Initialize variables in the definition block Len Baker
2021-08-24 9:00 ` [PATCH v2 3/3] staging/rtl8192u: Prefer kcalloc over open coded arithmetic 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).