[PATCH] PM / QOS: Use kcalloc() instead of kzalloc()
Use 2-factor multiplication argument form kcalloc() instead
of kzalloc().
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
drivers/base/power/qos.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 8e93167f1783..bd77f6734f14 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -201,7 +201,7 @@ static int dev_pm_qos_constraints_allocate(struct device
*dev)
if (!qos)
return -ENOMEM;
- n = kzalloc(3 * sizeof(*n), GFP_KERNEL);
+ n = kcalloc(3, sizeof(*n), GFP_KERNEL);
if (!n) {
kfree(qos);
return -ENOMEM;
--
2.42.0
[PATCH] ASoC: ti: Use devm_kcalloc() instead of devm_kzalloc()
Use 2-factor multiplication argument form devm_kcalloc() instead
of devm_kzalloc().
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
sound/soc/ti/j721e-evm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/ti/j721e-evm.c b/sound/soc/ti/j721e-evm.c
index b4b158dc736e..d9d1e021f5b2 100644
--- a/sound/soc/ti/j721e-evm.c
+++ b/sound/soc/ti/j721e-evm.c
@@ -649,7 +649,7 @@ static int j721e_soc_probe_cpb(struct j721e_priv *priv, int
*link_idx,
* Link 2: McASP10 <- pcm3168a_1 ADC
*/
comp_count = 6;
- compnent = devm_kzalloc(priv->dev, comp_count * sizeof(*compnent),
+ compnent = devm_kcalloc(priv->dev, comp_count, sizeof(*compnent),
GFP_KERNEL);
if (!compnent) {
ret = -ENOMEM;
@@ -763,7 +763,7 @@ static int j721e_soc_probe_ivi(struct j721e_priv *priv, int
*link_idx,
* \ pcm3168a_b ADC
*/
comp_count = 8;
- compnent = devm_kzalloc(priv->dev, comp_count * sizeof(*compnent),
+ compnent = devm_kcalloc(priv->dev, comp_count, sizeof(*compnent),
GFP_KERNEL);
if (!compnent) {
ret = -ENOMEM;
--
2.25.1
[PATCH] ASoC: qcom: Use devm_kcalloc() instead of devm_kzalloc()
Use 2-factor multiplication argument form devm_kcalloc() instead
of devm_kzalloc().
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
sound/soc/qcom/common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/qcom/common.c b/sound/soc/qcom/common.c
index 756706d5b493..747041fa7866 100644
--- a/sound/soc/qcom/common.c
+++ b/sound/soc/qcom/common.c
@@ -73,7 +73,7 @@ int qcom_snd_parse_of(struct snd_soc_card *card)
link = card->dai_link;
for_each_available_child_of_node(dev->of_node, np) {
- dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
+ dlc = devm_kcalloc(dev, 2, sizeof(*dlc), GFP_KERNEL);
if (!dlc) {
ret = -ENOMEM;
goto err_put_np;
--
2.25.1
[PATCH v2] ASoC: ti: j721e-evm: Use devm_kcalloc() instead of devm_kzalloc()
Use 2-factor multiplication argument form devm_kcalloc() instead
of devm_kzalloc().
Link: https://github.com/KSPP/linux/issues/162
Reviewed-by: Gustavo A. R. Silva
Acked-by: Peter Ujfalusi
Signed-off-by: Erick Archer
---
Changes in v2:
- Fix the commit title adding "j721e-evm"
---
sound/soc/ti/j721e-evm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/ti/j721e-evm.c b/sound/soc/ti/j721e-evm.c
index b4b158dc736e..d9d1e021f5b2 100644
--- a/sound/soc/ti/j721e-evm.c
+++ b/sound/soc/ti/j721e-evm.c
@@ -649,7 +649,7 @@ static int j721e_soc_probe_cpb(struct j721e_priv *priv, int
*link_idx,
* Link 2: McASP10 <- pcm3168a_1 ADC
*/
comp_count = 6;
- compnent = devm_kzalloc(priv->dev, comp_count * sizeof(*compnent),
+ compnent = devm_kcalloc(priv->dev, comp_count, sizeof(*compnent),
GFP_KERNEL);
if (!compnent) {
ret = -ENOMEM;
@@ -763,7 +763,7 @@ static int j721e_soc_probe_ivi(struct j721e_priv *priv, int
*link_idx,
* \ pcm3168a_b ADC
*/
comp_count = 8;
- compnent = devm_kzalloc(priv->dev, comp_count * sizeof(*compnent),
+ compnent = devm_kcalloc(priv->dev, comp_count, sizeof(*compnent),
GFP_KERNEL);
if (!compnent) {
ret = -ENOMEM;
--
2.25.1
[PATCH] scsi: csiostor: Use kcalloc() instead of kzalloc()
Use 2-factor multiplication argument form kcalloc() instead
of kzalloc().
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
drivers/scsi/csiostor/csio_init.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/csiostor/csio_init.c
b/drivers/scsi/csiostor/csio_init.c
index d649b7a2a879..d72892e44fd1 100644
--- a/drivers/scsi/csiostor/csio_init.c
+++ b/drivers/scsi/csiostor/csio_init.c
@@ -698,8 +698,7 @@ csio_lnodes_block_request(struct csio_hw *hw)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -737,8 +736,7 @@ csio_lnodes_unblock_request(struct csio_hw *hw)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -775,8 +773,7 @@ csio_lnodes_block_by_port(struct csio_hw *hw, uint8_t
portid)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -816,8 +813,7 @@ csio_lnodes_unblock_by_port(struct csio_hw *hw, uint8_t
portid)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -855,8 +851,7 @@ csio_lnodes_exit(struct csio_hw *hw, bool npiv)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "lnodes_exit: Failed to allocate lnodes_list.\n");
return;
--
2.25.1
[PATCH v2] scsi: csiostor: Use kcalloc() instead of kzalloc()
Use 2-factor multiplication argument form kcalloc() instead
of kzalloc().
Also, it is preferred to use sizeof(*pointer) instead of
sizeof(type) due to the type of the variable can change and
one needs not change the former (unlike the latter).
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
Changes in v2:
- Update the changelog text describing the sizeof()
changes (Gustavo A. R. Silva)
Version 1:
Link:
https://lore.kernel.org/linux-hardening/20240112182603.11048-1-erick.arc...@gmx.com/
---
drivers/scsi/csiostor/csio_init.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/csiostor/csio_init.c
b/drivers/scsi/csiostor/csio_init.c
index d649b7a2a879..d72892e44fd1 100644
--- a/drivers/scsi/csiostor/csio_init.c
+++ b/drivers/scsi/csiostor/csio_init.c
@@ -698,8 +698,7 @@ csio_lnodes_block_request(struct csio_hw *hw)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -737,8 +736,7 @@ csio_lnodes_unblock_request(struct csio_hw *hw)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -775,8 +773,7 @@ csio_lnodes_block_by_port(struct csio_hw *hw, uint8_t
portid)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -816,8 +813,7 @@ csio_lnodes_unblock_by_port(struct csio_hw *hw, uint8_t
portid)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -855,8 +851,7 @@ csio_lnodes_exit(struct csio_hw *hw, bool npiv)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "lnodes_exit: Failed to allocate lnodes_list.\n");
return;
--
2.25.1
[PATCH] eventfs: Use kcalloc() instead of kzalloc()
Use 2-factor multiplication argument form kcalloc() instead
of kzalloc().
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
fs/tracefs/event_inode.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index fdff53d5a1f8..f8196289692c 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -93,7 +93,7 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct
dentry *dentry,
/* Preallocate the children mode array if necessary */
if (!(dentry->d_inode->i_mode & S_IFDIR)) {
if (!ei->entry_attrs) {
- ei->entry_attrs = kzalloc(sizeof(*ei->entry_attrs) *
ei->nr_entries,
+ ei->entry_attrs = kcalloc(ei->nr_entries,
sizeof(*ei->entry_attrs),
GFP_NOFS);
if (!ei->entry_attrs) {
ret = -ENOMEM;
@@ -874,7 +874,7 @@ struct eventfs_inode *eventfs_create_dir(const char *name,
struct eventfs_inode
}
if (size) {
- ei->d_children = kzalloc(sizeof(*ei->d_children) * size,
GFP_KERNEL);
+ ei->d_children = kcalloc(size, sizeof(*ei->d_children),
GFP_KERNEL);
if (!ei->d_children) {
kfree_const(ei->name);
kfree(ei);
@@ -941,7 +941,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char
*name, struct dentry
goto fail;
if (size) {
- ei->d_children = kzalloc(sizeof(*ei->d_children) * size,
GFP_KERNEL);
+ ei->d_children = kcalloc(size, sizeof(*ei->d_children),
GFP_KERNEL);
if (!ei->d_children)
goto fail;
}
--
2.25.1
[PATCH v2] eventfs: Use kcalloc() instead of kzalloc()
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.
So, use the purpose specific kcalloc() function instead of the argument
size * count in the kzalloc() function.
[1]
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
Changes in v2:
- Update the commit message to better explain the changes (Mark Rutland)
Previous versions:
v1:
https://lore.kernel.org/linux-hardening/20240114105340.5746-1-erick.arc...@gmx.com/
---
fs/tracefs/event_inode.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index fdff53d5a1f8..f8196289692c 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -93,7 +93,7 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct
dentry *dentry,
/* Preallocate the children mode array if necessary */
if (!(dentry->d_inode->i_mode & S_IFDIR)) {
if (!ei->entry_attrs) {
- ei->entry_attrs = kzalloc(sizeof(*ei->entry_attrs) *
ei->nr_entries,
+ ei->entry_attrs = kcalloc(ei->nr_entries,
sizeof(*ei->entry_attrs),
GFP_NOFS);
if (!ei->entry_attrs) {
ret = -ENOMEM;
@@ -874,7 +874,7 @@ struct eventfs_inode *eventfs_create_dir(const char *name,
struct eventfs_inode
}
if (size) {
- ei->d_children = kzalloc(sizeof(*ei->d_children) * size,
GFP_KERNEL);
+ ei->d_children = kcalloc(size, sizeof(*ei->d_children),
GFP_KERNEL);
if (!ei->d_children) {
kfree_const(ei->name);
kfree(ei);
@@ -941,7 +941,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char
*name, struct dentry
goto fail;
if (size) {
- ei->d_children = kzalloc(sizeof(*ei->d_children) * size,
GFP_KERNEL);
+ ei->d_children = kcalloc(size, sizeof(*ei->d_children),
GFP_KERNEL);
if (!ei->d_children)
goto fail;
}
--
2.25.1
[PATCH] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*()
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.
So, use the purpose specific kcalloc*() function instead of the argument
size * count in the kzalloc*() function.
[1]
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
arch/x86/events/amd/uncore.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 5bf03c575812..9073eb0613cf 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -479,8 +479,8 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
goto fail;
curr->cpu = cpu;
- curr->events = kzalloc_node(sizeof(*curr->events) *
- pmu->num_counters,
+ curr->events = kcalloc_node(pmu->num_counters,
+ sizeof(*curr->events),
GFP_KERNEL, node);
if (!curr->events) {
kfree(curr);
@@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
uncore->num_pmus += group_num_pmus[gid];
}
- uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus,
+ uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus),
GFP_KERNEL);
if (!uncore->pmus) {
uncore->num_pmus = 0;
--
2.25.1
[PATCH] wifi: iwlegacy: Use kcalloc() instead of kzalloc()
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.
So, use the purpose specific kcalloc() function instead of the argument
size * count in the kzalloc() function.
Also, it is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not change the
former (unlike the latter).
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
drivers/net/wireless/intel/iwlegacy/common.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/common.c
b/drivers/net/wireless/intel/iwlegacy/common.c
index 17570d62c896..9d33a66a49b5 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -3438,9 +3438,7 @@ il_init_geos(struct il_priv *il)
if (!channels)
return -ENOMEM;
- rates =
- kzalloc((sizeof(struct ieee80211_rate) * RATE_COUNT_LEGACY),
- GFP_KERNEL);
+ rates = kcalloc(RATE_COUNT_LEGACY, sizeof(*rates), GFP_KERNEL);
if (!rates) {
kfree(channels);
return -ENOMEM;
--
2.25.1
[PATCH] staging: rtl8723bs: Use kcalloc() instead of kzalloc()
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.
So, use the purpose specific kcalloc() function instead of the argument
count * size in the kzalloc() function.
Also, it is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not change the
former (unlike the latter).
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 1ff763c10064..65a450fcdce7 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -1259,8 +1259,7 @@ static int cfg80211_rtw_scan(struct wiphy *wiphy
goto check_need_indicate_scan_done;
}
- ssid = kzalloc(RTW_SSID_SCAN_AMOUNT * sizeof(struct ndis_802_11_ssid),
- GFP_KERNEL);
+ ssid = kcalloc(RTW_SSID_SCAN_AMOUNT, sizeof(*ssid), GFP_KERNEL);
if (!ssid) {
ret = -ENOMEM;
goto check_need_indicate_scan_done;
--
2.25.1
[PATCH] pinctrl: pinctrl-zynqmp: Use devm_kcalloc() instead of devm_kzalloc()
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. So, use the purpose specific devm_kcalloc() function instead of the argument size * count in the devm_kzalloc() function. Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/162 Signed-off-by: Erick Archer --- drivers/pinctrl/pinctrl-zynqmp.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/pinctrl/pinctrl-zynqmp.c b/drivers/pinctrl/pinctrl-zynqmp.c index f2be341f73e1..5c46b7d7ebcb 100644 --- a/drivers/pinctrl/pinctrl-zynqmp.c +++ b/drivers/pinctrl/pinctrl-zynqmp.c @@ -562,7 +562,7 @@ static int zynqmp_pinctrl_prepare_func_groups(struct device *dev, u32 fid, const char **fgroups; int ret, index, i; - fgroups = devm_kzalloc(dev, sizeof(*fgroups) * func->ngroups, GFP_KERNEL); + fgroups = devm_kcalloc(dev, func->ngroups, sizeof(*fgroups), GFP_KERNEL); if (!fgroups) return -ENOMEM; @@ -754,7 +754,7 @@ static int zynqmp_pinctrl_prepare_function_info(struct device *dev, if (ret) return ret; - funcs = devm_kzalloc(dev, sizeof(*funcs) * pctrl->nfuncs, GFP_KERNEL); + funcs = devm_kcalloc(dev, pctrl->nfuncs, sizeof(*funcs), GFP_KERNEL); if (!funcs) return -ENOMEM; @@ -768,7 +768,7 @@ static int zynqmp_pinctrl_prepare_function_info(struct device *dev, pctrl->ngroups += funcs[i].ngroups; } - groups = devm_kzalloc(dev, sizeof(*groups) * pctrl->ngroups, GFP_KERNEL); + groups = devm_kcalloc(dev, pctrl->ngroups, sizeof(*groups), GFP_KERNEL); if (!groups) return -ENOMEM; @@ -830,7 +830,7 @@ static int zynqmp_pinctrl_prepare_pin_desc(struct device *dev, if (ret) return ret; - pins = devm_kzalloc(dev, sizeof(*pins) * *npins, GFP_KERNEL); + pins = devm_kcalloc(dev, *npins, sizeof(*pins), GFP_KERNEL); if (!pins) return -ENOMEM; -- 2.25.1
[PATCH] Documentation: power: Use kcalloc() instead of kzalloc()
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.
So, in the example code use the purpose specific kcalloc() function
instead of the argument size * count in the kzalloc() function.
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
Documentation/power/opp.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/power/opp.rst b/Documentation/power/opp.rst
index a7c03c470980..1b7f1d854f14 100644
--- a/Documentation/power/opp.rst
+++ b/Documentation/power/opp.rst
@@ -305,7 +305,7 @@ dev_pm_opp_get_opp_count
{
/* Do things */
num_available = dev_pm_opp_get_opp_count(dev);
- speeds = kzalloc(sizeof(u32) * num_available, GFP_KERNEL);
+ speeds = kcalloc(num_available, sizeof(u32), GFP_KERNEL);
/* populate the table in increasing order */
freq = 0;
while (!IS_ERR(opp = dev_pm_opp_find_freq_ceil(dev, &freq))) {
--
2.25.1
[PATCH] MIPS: Alchemy: Use kcalloc() instead of kzalloc()
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. So, use the purpose specific kcalloc() function instead of the argument size * count in the kzalloc() function. Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/162 Signed-off-by: Erick Archer --- arch/mips/alchemy/common/clock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/mips/alchemy/common/clock.c b/arch/mips/alchemy/common/clock.c index c01be8c45271..6c8996e20a7d 100644 --- a/arch/mips/alchemy/common/clock.c +++ b/arch/mips/alchemy/common/clock.c @@ -771,7 +771,7 @@ static int __init alchemy_clk_init_fgens(int ctype) } id.flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE; - a = kzalloc((sizeof(*a)) * 6, GFP_KERNEL); + a = kcalloc(6, sizeof(*a), GFP_KERNEL); if (!a) return -ENOMEM; -- 2.25.1
[PATCH] riscv: Use kcalloc() instead of kzalloc()
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.
So, use the purpose specific kcalloc() function instead of the argument
count * size in the kzalloc() function.
Also, it is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not change the
former (unlike the latter).
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer
---
arch/riscv/kernel/cpufeature.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 89920f84d0a3..549a76e34c4e 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -901,8 +901,7 @@ static int check_unaligned_access_all_cpus(void)
{
unsigned int cpu;
unsigned int cpu_count = num_possible_cpus();
- struct page **bufs = kzalloc(cpu_count * sizeof(struct page *),
-GFP_KERNEL);
+ struct page **bufs = kcalloc(cpu_count, sizeof(*bufs), GFP_KERNEL);
if (!bufs) {
pr_warn("Allocation failure, not measuring misaligned
performance\n");
--
2.25.1
[PATCH] bus: mhi: ep: Use kcalloc() instead of kzalloc()
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. So, use the purpose specific kcalloc() function instead of the argument count * size in the kzalloc() function. Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/162 Signed-off-by: Erick Archer --- drivers/bus/mhi/ep/main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c index 65fc1d738bec..8d7a4102bdb7 100644 --- a/drivers/bus/mhi/ep/main.c +++ b/drivers/bus/mhi/ep/main.c @@ -1149,8 +1149,9 @@ int mhi_ep_power_up(struct mhi_ep_cntrl *mhi_cntrl) mhi_ep_mmio_mask_interrupts(mhi_cntrl); mhi_ep_mmio_init(mhi_cntrl); - mhi_cntrl->mhi_event = kzalloc(mhi_cntrl->event_rings * (sizeof(*mhi_cntrl->mhi_event)), - GFP_KERNEL); + mhi_cntrl->mhi_event = kcalloc(mhi_cntrl->event_rings, + sizeof(*mhi_cntrl->mhi_event), + GFP_KERNEL); if (!mhi_cntrl->mhi_event) return -ENOMEM; -- 2.25.1
Re: [PATCH] Documentation: power: Use kcalloc() instead of kzalloc()
On Sat, Jan 20, 2024 at 01:05:27PM +0100, Erick Archer wrote:
> 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.
>
> So, in the example code use the purpose specific kcalloc() function
> instead of the argument size * count in the kzalloc() function.
>
> Link:
> https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> [1]
> Link: https://github.com/KSPP/linux/issues/162
> Signed-off-by: Erick Archer
Hi,
Drop this patch as a new version has been sent to change at the
same time the translations.
Thanks,
Erick
> ---
> Documentation/power/opp.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/power/opp.rst b/Documentation/power/opp.rst
> index a7c03c470980..1b7f1d854f14 100644
> --- a/Documentation/power/opp.rst
> +++ b/Documentation/power/opp.rst
> @@ -305,7 +305,7 @@ dev_pm_opp_get_opp_count
>{
> /* Do things */
> num_available = dev_pm_opp_get_opp_count(dev);
> - speeds = kzalloc(sizeof(u32) * num_available, GFP_KERNEL);
> + speeds = kcalloc(num_available, sizeof(u32), GFP_KERNEL);
> /* populate the table in increasing order */
> freq = 0;
> while (!IS_ERR(opp = dev_pm_opp_find_freq_ceil(dev, &freq))) {
> --
> 2.25.1
>
[PATCH] clk: hisilicon: Use devm_kcalloc() instead of devm_kzalloc()
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. So, use the purpose specific devm_kcalloc() function instead of the argument size * count in the devm_kzalloc() function. Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/162 Signed-off-by: Erick Archer --- drivers/clk/hisilicon/clk-hi3559a.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/clk/hisilicon/clk-hi3559a.c b/drivers/clk/hisilicon/clk-hi3559a.c index ff4ca0edce06..da476f940326 100644 --- a/drivers/clk/hisilicon/clk-hi3559a.c +++ b/drivers/clk/hisilicon/clk-hi3559a.c @@ -461,8 +461,7 @@ static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks, struct clk_init_data init; int i; - p_clk = devm_kzalloc(dev, sizeof(*p_clk) * nums, GFP_KERNEL); - + p_clk = devm_kcalloc(dev, nums, sizeof(*p_clk), GFP_KERNEL); if (!p_clk) return; -- 2.25.1
Re: [PATCH] staging: rtl8723bs: Use kcalloc() instead of kzalloc()
Hi Dan, On Mon, Jan 22, 2024 at 09:55:11AM +0300, Dan Carpenter wrote: > On Fri, Jan 19, 2024 at 06:39:00PM +0100, Erick Archer wrote: > > 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. > > > > So, use the purpose specific kcalloc() function instead of the argument > > count * size in the kzalloc() function. > > > > Also, it is preferred to use sizeof(*pointer) instead of sizeof(type) > > due to the type of the variable can change and one needs not change the > > former (unlike the latter). > > > > Link: > > https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > [1] > > Link: https://github.com/KSPP/linux/issues/162 > > Signed-off-by: Erick Archer > > I quite often write responses to patches and then never send them. I > wrote this response and debated sending it but in the end I decided to > send it because you have sent multiple patches. If you had only sent > one patch then I wouldn't have bothered. My intention is not to bother anyone. I'm a linux kernel developer newbie and I try to do my best. > Generally, commit messages should say what the user visible effects of > a patch are. Sometimes with these sorts of commits, it's hard to > determine the effect. For example, Kees went through and changed dozens > or hundreds of these allocations to use safer constructs and we don't > necessarily expect him to audit all the code. They should already have > been fine, but it's better to be safe. > > However in this case obviously the patch is small and just by glancing > at it we can see that it has no effect on rutime. > > But if someone is reviewing patches with "git log" instead of > "git log -p" they aren't going to see the patch. I can almost always > figure out what a commit does without looking at the commit message, > that doesn't mean that the commit messages are unnecessary. > > So I really prefer if commit message say, "This commit is just to make > static checkers happy and to make the code more readable. It has no > effect on runtime." The commit message you wrote is way more scary than > is warranted. Here is my proposed commit message: > > "We are trying to get rid of all multiplications from allocation > functions to prevent integer overflows. Here the multiplication is > obviously safe, but using kcalloc() is more appropriate and improves > readability. This patch has no effect on runtime behavior." > Understood. Thank you very much for your guidance and advices. I will change the commit message and I will send a more appropiate v2 patch. Best regards, Erick > regards, > dan carpenter >
Re: [PATCH] bus: mhi: ep: Use kcalloc() instead of kzalloc()
Hi Dan,
On Mon, Jan 22, 2024 at 10:15:20AM +0300, Dan Carpenter wrote:
> This code does not have an integer overflow, but it might have a
> different memory corruption bug.
I don't see this possible memory corruption bug. More info below.
> On Sat, Jan 20, 2024 at 04:25:18PM +0100, Erick Archer wrote:
> > 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.
> >
> > So, use the purpose specific kcalloc() function instead of the argument
> > count * size in the kzalloc() function.
> >
>
> This one is more complicated to analyze. I have built a Smatch cross
> function database so it's easy for me and I will help you.
>
> $ smbd.py where mhi_ep_cntrl event_rings
> drivers/pci/endpoint/functions/pci-epf-mhi.c | pci_epf_mhi_probe
> | (struct mhi_ep_cntrl)->event_rings | 0
> drivers/bus/mhi/ep/main.c | mhi_ep_irq | (struct
> mhi_ep_cntrl)->event_rings | min-max
> drivers/bus/mhi/ep/mmio.c | mhi_ep_mmio_init | (struct
> mhi_ep_cntrl)->event_rings | 0-255
> drivers/bus/mhi/ep/mmio.c | mhi_ep_mmio_update_ner | (struct
> mhi_ep_cntrl)->event_rings | 0-255
>
> The other way to figure this stuff out would be to do:
>
> $ grep -Rn "event_rings = " drivers/bus/mhi/ep/
> drivers/bus/mhi/ep/mmio.c:260: mhi_cntrl->event_rings =
> FIELD_GET(MHICFG_NER_MASK, regval);
> drivers/bus/mhi/ep/mmio.c:261: mhi_cntrl->hw_event_rings =
> FIELD_GET(MHICFG_NHWER_MASK, regval);
> drivers/bus/mhi/ep/mmio.c:271: mhi_cntrl->event_rings =
> FIELD_GET(MHICFG_NER_MASK, regval);
> drivers/bus/mhi/ep/mmio.c:272: mhi_cntrl->hw_event_rings =
> FIELD_GET(MHICFG_NHWER_MASK, regval);
>
> That means that this multiplication can never overflow so the patch
> has no effect on runtime. The patch is still useful because we don't
> want every single person to have to do this analysis. The kcalloc()
> function is just safer and more obviously correct.
Ok, I will send a v2 patch with more info in the commit message.
> It's a bit concerning that ->event_rings is set multiple times, but only
> allocated one time. It's either unnecessary or there is a potential
> memory corruption bug. If it's really necessary then there should be a
> check that the new size is <= the size of the original buffer that we
> allocated.
The ->event_rings is set twice. In the mhi_ep_mmio_init function and in
the mhi_ep_mmio_update_ner function.
void mhi_ep_mmio_init(struct mhi_ep_cntrl *mhi_cntrl)
{
[...]
mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval);
[...]
}
void mhi_ep_mmio_update_ner(struct mhi_ep_cntrl *mhi_cntrl)
{
[...]
mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval);
[...]
}
But ->event_rings does not need to be allocated because the type is a u32.
struct mhi_ep_cntrl {
[...]
u32 event_rings;
[...]
};
So, I don't know what you are trying to explain to me. I'm sorry.
> I work in static analysis and I understand the struggle of trying to
> understand code to see if static checker warnings are a real bug or not.
> I'm not going to insist that you figure everything out, but I am asking
> that you at least try. If after spending ten minutes reading the code
> you can't figure it out, then it's fine to write something like, "I
> don't know whether this multiply can really overflow or not, but let's
> make it safer by using kcalloc()." You can put that sort of "I don't
> know information" under the --- cut off line inf you want.
Thanks a lot for the advices.
Regards,
Erick
> regards,
> dan carpenter
[PATCH v2] bus: mhi: ep: Use kcalloc() instead of kzalloc()
This is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1]. Here the multiplication is obviously safe because the "event_rings" member never can have a value greater than 255 (8 bits). This member is set twice using always FIELD_GET: mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval); mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval); And the MHICFG_NER_MASK macro defines the 8 bits mask that guarantees a maximum value of 255. However, using kcalloc() is more appropriate [1] and improves readability. This patch has no effect on runtime behavior. Link: https://github.com/KSPP/linux/issues/162 [1] Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Reviewed-by: Gustavo A. R. Silva Signed-off-by: Erick Archer --- Changes in v2: - Add more info in the commit message to better explain the change. (Dan Carpenter) - Add the "Reviewed-by:" tag. Previous versions: v1 - https://lore.kernel.org/linux-hardening/20240120152518.13006-1-erick.arc...@gmx.com/ --- drivers/bus/mhi/ep/main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c index 65fc1d738bec..8d7a4102bdb7 100644 --- a/drivers/bus/mhi/ep/main.c +++ b/drivers/bus/mhi/ep/main.c @@ -1149,8 +1149,9 @@ int mhi_ep_power_up(struct mhi_ep_cntrl *mhi_cntrl) mhi_ep_mmio_mask_interrupts(mhi_cntrl); mhi_ep_mmio_init(mhi_cntrl); - mhi_cntrl->mhi_event = kzalloc(mhi_cntrl->event_rings * (sizeof(*mhi_cntrl->mhi_event)), - GFP_KERNEL); + mhi_cntrl->mhi_event = kcalloc(mhi_cntrl->event_rings, + sizeof(*mhi_cntrl->mhi_event), + GFP_KERNEL); if (!mhi_cntrl->mhi_event) return -ENOMEM; -- 2.25.1
[PATCH] dmaengine: pl08x: Use kcalloc() instead of kzalloc()
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
Here the multiplication is obviously safe because the "channels"
member can only be 8 or 2. This value is set when the "vendor_data"
structs are initialized.
static struct vendor_data vendor_pl080 = {
[...]
.channels = 8,
[...]
};
static struct vendor_data vendor_nomadik = {
[...]
.channels = 8,
[...]
};
static struct vendor_data vendor_pl080s = {
[...]
.channels = 8,
[...]
};
static struct vendor_data vendor_pl081 = {
[...]
.channels = 2,
[...]
};
However, using kcalloc() is more appropriate [1] and improves
readability. This patch has no effect on runtime behavior.
Link: https://github.com/KSPP/linux/issues/162 [1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Signed-off-by: Erick Archer
---
drivers/dma/amba-pl08x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index eea8bd33b4b7..e40766108787 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -2855,8 +2855,8 @@ static int pl08x_probe(struct amba_device *adev, const
struct amba_id *id)
}
/* Initialize physical channels */
- pl08x->phy_chans = kzalloc((vd->channels * sizeof(*pl08x->phy_chans)),
- GFP_KERNEL);
+ pl08x->phy_chans = kcalloc(vd->channels, sizeof(*pl08x->phy_chans),
+ GFP_KERNEL);
if (!pl08x->phy_chans) {
ret = -ENOMEM;
goto out_no_phychans;
--
2.25.1
Re: [PATCH] bus: mhi: ep: Use kcalloc() instead of kzalloc()
Hi Dan, On Mon, Jan 29, 2024 at 08:20:26AM +0300, Dan Carpenter wrote: > On Sun, Jan 28, 2024 at 11:29:33AM +0100, Erick Archer wrote: > > > It's a bit concerning that ->event_rings is set multiple times, but only > > > allocated one time. It's either unnecessary or there is a potential > > > memory corruption bug. If it's really necessary then there should be a > > > check that the new size is <= the size of the original buffer that we > > > allocated. > > > > The ->event_rings is set twice. In the mhi_ep_mmio_init function and in > > the mhi_ep_mmio_update_ner function. > > > > It's not about the type. > > The event_rings struct member is the number of elements in the > mhi_cntrl->mhi_event array. However, we ->event_rings without > re-allocating mhi_cntrl->mhi_event so those are not in sync any more. > So since we don't know the number of elements in the mhi_cntrl->mhi_event > array leading to memory corruption. Thanks for this clarification. Now I understand what you are explaining to me. Regards, Erick
[PATCH] irqchip/bcm-6345-l1: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
As the cpu variable is a pointer to "struct bcm6345_l1_cpu" and this
structure ends in a flexible array:
struct bcm6345_l1_cpu {
[...]
u32 enable_cache[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the argument "size + count * size" in the
kzalloc() function.
This way, the code is more readable and more safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/162 [2]
Signed-off-by: Erick Archer
---
drivers/irqchip/irq-bcm6345-l1.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c
index 9745a119d0e6..eb02d203c963 100644
--- a/drivers/irqchip/irq-bcm6345-l1.c
+++ b/drivers/irqchip/irq-bcm6345-l1.c
@@ -242,7 +242,7 @@ static int __init bcm6345_l1_init_one(struct device_node
*dn,
else if (intc->n_words != n_words)
return -EINVAL;
- cpu = intc->cpus[idx] = kzalloc(sizeof(*cpu) + n_words * sizeof(u32),
+ cpu = intc->cpus[idx] = kzalloc(struct_size(cpu, enable_cache, n_words),
GFP_KERNEL);
if (!cpu)
return -ENOMEM;
--
2.25.1
[PATCH] irqchip/irq-bcm7038-l1: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
As the cpu variable is a pointer to "struct bcm7038_l1_cpu" and this
structure ends in a flexible array:
struct bcm7038_l1_cpu {
void __iomem*map_base;
u32 mask_cache[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the argument "size + count * size" in the
kzalloc() function.
This way, the code is more readable and more safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/162 [2]
Signed-off-by: Erick Archer
---
drivers/irqchip/irq-bcm7038-l1.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c
index 24ca1d656adc..36e71af054e9 100644
--- a/drivers/irqchip/irq-bcm7038-l1.c
+++ b/drivers/irqchip/irq-bcm7038-l1.c
@@ -249,7 +249,7 @@ static int __init bcm7038_l1_init_one(struct device_node
*dn,
return -EINVAL;
}
- cpu = intc->cpus[idx] = kzalloc(sizeof(*cpu) + n_words * sizeof(u32),
+ cpu = intc->cpus[idx] = kzalloc(struct_size(cpu, mask_cache, n_words),
GFP_KERNEL);
if (!cpu)
return -ENOMEM;
--
2.25.1
[PATCH] mtd: rawnand: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
As the "chip" variable is a pointer to "struct mtk_nfc_nand_chip" and
this structure ends in a flexible array:
struct mtk_nfc_nand_chip {
[...]
u8 sels[] __counted_by(nsels);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the argument "size + count * size" in the
devm_kzalloc() function.
This way, the code is more readable and more safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
drivers/mtd/nand/raw/mtk_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/mtk_nand.c b/drivers/mtd/nand/raw/mtk_nand.c
index 60198e33d2d5..17477bb2d48f 100644
--- a/drivers/mtd/nand/raw/mtk_nand.c
+++ b/drivers/mtd/nand/raw/mtk_nand.c
@@ -1356,7 +1356,7 @@ static int mtk_nfc_nand_chip_init(struct device *dev,
struct mtk_nfc *nfc,
return -EINVAL;
}
- chip = devm_kzalloc(dev, sizeof(*chip) + nsels * sizeof(u8),
+ chip = devm_kzalloc(dev, struct_size(chip, sels, nsels),
GFP_KERNEL);
if (!chip)
return -ENOMEM;
--
2.25.1
[PATCH v2] mtd: rawnand: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
As the "chip" variable is a pointer to "struct mtk_nfc_nand_chip" and
this structure ends in a flexible array:
struct mtk_nfc_nand_chip {
[...]
u8 sels[] __counted_by(nsels);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the argument "size + count * size" in the
devm_kzalloc() function.
This way, the code is more readable and safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Reviewed-by: Gustavo A. R. Silva
Signed-off-by: Erick Archer
---
Changes in v2:
- Add the "Reviewed-by:" tag.
- Fix a spelling error in the commit message. Change "more safer" for
"safer" (Uwe Kleine-König)
---
drivers/mtd/nand/raw/mtk_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/mtk_nand.c b/drivers/mtd/nand/raw/mtk_nand.c
index 60198e33d2d5..17477bb2d48f 100644
--- a/drivers/mtd/nand/raw/mtk_nand.c
+++ b/drivers/mtd/nand/raw/mtk_nand.c
@@ -1356,7 +1356,7 @@ static int mtk_nfc_nand_chip_init(struct device *dev,
struct mtk_nfc *nfc,
return -EINVAL;
}
- chip = devm_kzalloc(dev, sizeof(*chip) + nsels * sizeof(u8),
+ chip = devm_kzalloc(dev, struct_size(chip, sels, nsels),
GFP_KERNEL);
if (!chip)
return -ENOMEM;
--
2.25.1
[PATCH] RDMA/uverbs: Remove flexible arrays from struct *_filter
When a struct containing a flexible array is included in another struct,
and there is a member after the struct-with-flex-array, there is a
possibility of memory overlap. These cases must be audited [1]. See:
struct inner {
...
int flex[];
};
struct outer {
...
struct inner header;
int overlap;
...
};
This is the scenario for all the "struct *_filter" structures that are
included in the following "struct ib_flow_spec_*" structures:
struct ib_flow_spec_eth
struct ib_flow_spec_ib
struct ib_flow_spec_ipv4
struct ib_flow_spec_ipv6
struct ib_flow_spec_tcp_udp
struct ib_flow_spec_tunnel
struct ib_flow_spec_esp
struct ib_flow_spec_gre
struct ib_flow_spec_mpls
The pattern is like the one shown below:
struct *_filter {
...
u8 real_sz[];
};
struct ib_flow_spec_mpls {
...
struct *_filter val;
struct *_filter mask;
};
In this case, the trailing flexible array "real_sz" is never allocated
and is only used to calculate the size of the structures. Here the use
of the "offsetof" helper can be changed by the "sizeof" operator because
the goal is to get the size of these structures. Therefore, the trailing
flexible arrays can also be removed.
Link: https://github.com/KSPP/linux/issues/202 [1]
Signed-off-by: Erick Archer
---
Hi everyone,
This patch has not been tested. This has only been built-tested.
Regards,
Erick
---
drivers/infiniband/core/uverbs_cmd.c | 16
include/rdma/ib_verbs.h | 17 -
2 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c
b/drivers/infiniband/core/uverbs_cmd.c
index 6de05ade2ba9..3d3ee3eca983 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2737,7 +2737,7 @@ int ib_uverbs_kern_spec_to_ib_spec_filter(enum
ib_flow_spec_type type,
switch (ib_spec->type & ~IB_FLOW_SPEC_INNER) {
case IB_FLOW_SPEC_ETH:
- ib_filter_sz = offsetof(struct ib_flow_eth_filter, real_sz);
+ ib_filter_sz = sizeof(struct ib_flow_eth_filter);
actual_filter_sz = spec_filter_size(kern_spec_mask,
kern_filter_sz,
ib_filter_sz);
@@ -2748,7 +2748,7 @@ int ib_uverbs_kern_spec_to_ib_spec_filter(enum
ib_flow_spec_type type,
memcpy(&ib_spec->eth.mask, kern_spec_mask, actual_filter_sz);
break;
case IB_FLOW_SPEC_IPV4:
- ib_filter_sz = offsetof(struct ib_flow_ipv4_filter, real_sz);
+ ib_filter_sz = sizeof(struct ib_flow_ipv4_filter);
actual_filter_sz = spec_filter_size(kern_spec_mask,
kern_filter_sz,
ib_filter_sz);
@@ -2759,7 +2759,7 @@ int ib_uverbs_kern_spec_to_ib_spec_filter(enum
ib_flow_spec_type type,
memcpy(&ib_spec->ipv4.mask, kern_spec_mask, actual_filter_sz);
break;
case IB_FLOW_SPEC_IPV6:
- ib_filter_sz = offsetof(struct ib_flow_ipv6_filter, real_sz);
+ ib_filter_sz = sizeof(struct ib_flow_ipv6_filter);
actual_filter_sz = spec_filter_size(kern_spec_mask,
kern_filter_sz,
ib_filter_sz);
@@ -2775,7 +2775,7 @@ int ib_uverbs_kern_spec_to_ib_spec_filter(enum
ib_flow_spec_type type,
break;
case IB_FLOW_SPEC_TCP:
case IB_FLOW_SPEC_UDP:
- ib_filter_sz = offsetof(struct ib_flow_tcp_udp_filter, real_sz);
+ ib_filter_sz = sizeof(struct ib_flow_tcp_udp_filter);
actual_filter_sz = spec_filter_size(kern_spec_mask,
kern_filter_sz,
ib_filter_sz);
@@ -2786,7 +2786,7 @@ int ib_uverbs_kern_spec_to_ib_spec_filter(enum
ib_flow_spec_type type,
memcpy(&ib_spec->tcp_udp.mask, kern_spec_mask,
actual_filter_sz);
break;
case IB_FLOW_SPEC_VXLAN_TUNNEL:
- ib_filter_sz = offsetof(struct ib_flow_tunnel_filter, real_sz);
+ ib_filter_sz = sizeof(struct ib_flow_tunnel_filter);
actual_filter_sz = spec_filter_size(kern_spec_mask,
kern_filter_sz,
ib_filter_sz);
@@ -2801,7 +2801,7 @@ int ib_uverbs_kern_spec_to_ib_spec_filter(enum
ib_flow_spec_type type,
return -EINVAL;
break;
case IB_FLOW_SPEC_ESP:
- ib_filter_sz = offsetof(struct ib_flow_esp_filter, real_sz);
+ ib_filter_sz = sizeof(s
[PATCH] iommu/vt-d: Use kcalloc() instead of kzalloc()
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
Here the multiplication is obviously safe because DMAR_LATENCY_NUM
is the number of latency types defined in the "latency_type" enum.
enum latency_type {
DMAR_LATENCY_INV_IOTLB = 0,
DMAR_LATENCY_INV_DEVTLB,
DMAR_LATENCY_INV_IEC,
DMAR_LATENCY_PRQ,
DMAR_LATENCY_NUM
};
However, using kcalloc() is more appropriate [2] and improves
readability. This patch has no effect on runtime behavior.
Link: https://github.com/KSPP/linux/issues/162 [1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[2]
Signed-off-by: Erick Archer
---
drivers/iommu/intel/perf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/perf.c b/drivers/iommu/intel/perf.c
index 94ee70ac38e3..adc4de6bbd88 100644
--- a/drivers/iommu/intel/perf.c
+++ b/drivers/iommu/intel/perf.c
@@ -33,7 +33,7 @@ int dmar_latency_enable(struct intel_iommu *iommu, enum
latency_type type)
spin_lock_irqsave(&latency_lock, flags);
if (!iommu->perf_statistic) {
- iommu->perf_statistic = kzalloc(sizeof(*lstat) *
DMAR_LATENCY_NUM,
+ iommu->perf_statistic = kcalloc(DMAR_LATENCY_NUM,
sizeof(*lstat),
GFP_ATOMIC);
if (!iommu->perf_statistic) {
ret = -ENOMEM;
--
2.25.1
[PATCH] iommu/mtk_iommu: Use devm_kcalloc() instead of devm_kzalloc()
This is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1]. Here the multiplication is obviously safe because MTK_PROTECT_PA_ALIGN is defined as a literal value of 256 or 128. For the "mtk_iommu.c" file: 256 For the "mtk_iommu_v1.c" file: 128 However, using devm_kcalloc() is more appropriate [2] and improves readability. This patch has no effect on runtime behavior. Link: https://github.com/KSPP/linux/issues/162 [1] Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2] Signed-off-by: Erick Archer --- drivers/iommu/mtk_iommu.c| 2 +- drivers/iommu/mtk_iommu_v1.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c index 7abe9e85a570..9aae6eb604b1 100644 --- a/drivers/iommu/mtk_iommu.c +++ b/drivers/iommu/mtk_iommu.c @@ -1264,7 +1264,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) data->plat_data = of_device_get_match_data(dev); /* Protect memory. HW will access here while translation fault.*/ - protect = devm_kzalloc(dev, MTK_PROTECT_PA_ALIGN * 2, GFP_KERNEL); + protect = devm_kcalloc(dev, 2, MTK_PROTECT_PA_ALIGN, GFP_KERNEL); if (!protect) return -ENOMEM; data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN); diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c index 25b41222abae..45cd845d153f 100644 --- a/drivers/iommu/mtk_iommu_v1.c +++ b/drivers/iommu/mtk_iommu_v1.c @@ -621,8 +621,8 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev) data->dev = dev; /* Protect memory. HW will access here while translation fault.*/ - protect = devm_kzalloc(dev, MTK_PROTECT_PA_ALIGN * 2, - GFP_KERNEL | GFP_DMA); + protect = devm_kcalloc(dev, 2, MTK_PROTECT_PA_ALIGN, + GFP_KERNEL | GFP_DMA); if (!protect) return -ENOMEM; data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN); -- 2.25.1
[PATCH v2] RDMA/uverbs: Remove flexible arrays from struct *_filter
When a struct containing a flexible array is included in another struct,
and there is a member after the struct-with-flex-array, there is a
possibility of memory overlap. These cases must be audited [1]. See:
struct inner {
...
int flex[];
};
struct outer {
...
struct inner header;
int overlap;
...
};
This is the scenario for all the "struct *_filter" structures that are
included in the following "struct ib_flow_spec_*" structures:
struct ib_flow_spec_eth
struct ib_flow_spec_ib
struct ib_flow_spec_ipv4
struct ib_flow_spec_ipv6
struct ib_flow_spec_tcp_udp
struct ib_flow_spec_tunnel
struct ib_flow_spec_esp
struct ib_flow_spec_gre
struct ib_flow_spec_mpls
The pattern is like the one shown below:
struct *_filter {
...
u8 real_sz[];
};
struct ib_flow_spec_* {
...
struct *_filter val;
struct *_filter mask;
};
In this case, the trailing flexible array "real_sz" is never allocated
and is only used to calculate the size of the structures. Here the use
of the "offsetof" helper can be changed by the "sizeof" operator because
the goal is to get the size of these structures. Therefore, the trailing
flexible arrays can also be removed.
However, due to the trailing padding that can be induced in structs it
is possible that the:
offsetof(struct *_filter, real_sz) != sizeof(struct *_filter)
This situation happens with the "struct ib_flow_ipv6_filter" and to
avoid it the "__packed" macro is used in this structure. But now, the
"sizeof(struct ib_flow_ipv6_filter)" has changed. This is not a problem
since this size is not used in the code.
The situation now is that "sizeof(struct ib_flow_spec_ipv6)" has also
changed (this struct contains the struct ib_flow_ipv6_filter). This is
also not a problem since it is only used to set the size of the "union
ib_flow_spec", which can store all the "ib_flow_spec_*" structures.
Link: https://github.com/KSPP/linux/issues/202 [1]
Signed-off-by: Erick Archer
---
Changes in v2:
- Add the "__packed" macro to the "struct ib_flow_ipv6_filter".
- Update the commit message to explain why the "__packed" macro is used.
Previous versions:
v1:
https://lore.kernel.org/linux-hardening/2024025856.9788-1-erick.arc...@gmx.com/
Hi Kees and Jason,
First of all, thanks for yours explanations and tips. These were used to get
the final patch.
The steps that I have followed to only add the "__packed" macro to the
"struct ib_flow_ipv6_filter" are the followings:
Step 1: Create new "struct *_filter_new" structures based on "struct *_filter"
structures but with the trailing flexible arrays removed. Create new
"struct ib_flow_spec_*_new" structures based on "struct ib_flow_spec_*"
structures but containing the new "struct *_filter_new" structures.
Also add a set of assertions to check all the "offsetof" changes made
in the patch sent. And a set of assertions to verify if the size of
all the "struct ib_flow_spec_*" structures " has changed.
drivers/infiniband/core/uverbs_cmd.c | 19 +
include/rdma/ib_verbs.h | 117 +++
2 files changed, 136 insertions(+)
diff --git a/drivers/infiniband/core/uverbs_cmd.c
b/drivers/infiniband/core/uverbs_cmd.c
index 6de05ade2ba9..a7ee65791900 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2726,6 +2726,25 @@ int ib_uverbs_kern_spec_to_ib_spec_filter(enum
ib_flow_spec_type type,
ssize_t actual_filter_sz;
ssize_t ib_filter_sz;
+ static_assert(offsetof(struct ib_flow_eth_filter, real_sz) ==
sizeof(struct ib_flow_eth_filter_new));
+ static_assert(offsetof(struct ib_flow_ipv4_filter, real_sz) ==
sizeof(struct ib_flow_ipv4_filter_new));
+ static_assert(offsetof(struct ib_flow_ipv6_filter, real_sz) ==
sizeof(struct ib_flow_ipv6_filter_new));
+ static_assert(offsetof(struct ib_flow_tcp_udp_filter, real_sz) ==
sizeof(struct ib_flow_tcp_udp_filter_new));
+ static_assert(offsetof(struct ib_flow_tunnel_filter, real_sz) ==
sizeof(struct ib_flow_tunnel_filter_new));
+ static_assert(offsetof(struct ib_flow_esp_filter, real_sz) ==
sizeof(struct ib_flow_esp_filter_new));
+ static_assert(offsetof(struct ib_flow_gre_filter, real_sz) ==
sizeof(struct ib_flow_gre_filter_new));
+ static_assert(offsetof(struct ib_flow_mpls_filter, real_sz) ==
sizeof(struct ib_flow_mpls_filter_new));
+
+ static_assert(sizeof(struct ib_flow_spec_eth) == sizeof(struct
ib_flow_spec_eth_new));
+ static_assert(sizeof(struct ib_flow_spec_ib) == sizeof(struct
ib_flow_spec_ib_new));
+ static_assert(sizeof(struct ib_flow_spec_ipv4) == siz
[PATCH] greybus: audio: apbridgea: Remove flexible array from struct audio_apbridgea_hdr
When a struct containing a flexible array is included in another struct,
and there is a member after the struct-with-flex-array, there is a
possibility of memory overlap. These cases must be audited [1]. See:
struct inner {
...
int flex[];
};
struct outer {
...
struct inner header;
int overlap;
...
};
This is the scenario for the "struct audio_apbridgea_hdr" structure
that is included in the following "struct audio_apbridgea_*_request"
structures:
struct audio_apbridgea_set_config_request
struct audio_apbridgea_register_cport_request
struct audio_apbridgea_unregister_cport_request
struct audio_apbridgea_set_tx_data_size_request
struct audio_apbridgea_prepare_tx_request
struct audio_apbridgea_start_tx_request
struct audio_apbridgea_stop_tx_request
struct audio_apbridgea_shutdown_tx_request
struct audio_apbridgea_set_rx_data_size_request
struct audio_apbridgea_prepare_rx_request
struct audio_apbridgea_start_rx_request
struct audio_apbridgea_stop_rx_request
struct audio_apbridgea_shutdown_rx_request
The pattern is like the one shown below:
struct audio_apbridgea_hdr {
...
__u8 data[];
} __packed;
struct audio_apbridgea_*_request {
struct audio_apbridgea_hdr hdr;
...
} __packed;
In this case, the trailing flexible array can be removed because it is
never used.
Link: https://github.com/KSPP/linux/issues/202 [1]
Signed-off-by: Erick Archer
---
Hi everyone,
I'm not sure this patch is correct. My concerns are:
The "struct audio_apbridgea_hdr" structure is used as a first member in
all the "struct audio_apbridgea_*_request" structures. And these last
structures are used in the "gb_audio_apbridgea_*(...)" functions. These
functions fill the "request" structure and always use:
gb_hd_output(connection->hd, &req, sizeof(req),
GB_APB_REQUEST_AUDIO_CONTROL, true);
Then, the "gb_hd_output(struct gb_host_device *hd, ...)" function calls:
hd->driver->output(hd, req, size, cmd, async);
The first parameter to this function is of type:
struct gb_host_device {
...
const struct gb_hd_driver *driver;
...
};
And the "gb_hd_driver" structure is defined as:
struct gb_hd_driver {
...
int (*output)(struct gb_host_device *hd, void *req, u16 size,
u8 cmd,
bool async);
};
Therefore, my question is:
Where is the "output" function pointer set? I think I'm missing something.
I have search for another greybus drivers and I have found that, for
example, the "es2_ap_driver" (drivers/greybus/es2.c) sets the "output"
member in:
static struct gb_hd_driver es2_driver = {
...
.output = output,
};
I think that the flexible array that I have removed should be used in
the function assigned to the "output" function pointer. But I am not
able to find this function.
A bit of light on this would be greatly appreciated.
Thanks,
Erick
---
drivers/staging/greybus/audio_apbridgea.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/staging/greybus/audio_apbridgea.h
b/drivers/staging/greybus/audio_apbridgea.h
index efec0f815efd..ab707d310129 100644
--- a/drivers/staging/greybus/audio_apbridgea.h
+++ b/drivers/staging/greybus/audio_apbridgea.h
@@ -65,7 +65,6 @@
struct audio_apbridgea_hdr {
__u8type;
__le16 i2s_port;
- __u8data[];
} __packed;
struct audio_apbridgea_set_config_request {
--
2.25.1
[PATCH] net: wwan: t7xx: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "port_prox" variable is a pointer to "struct port_proxy" and
this structure ends in a flexible array:
struct port_proxy {
[...]
struct t7xx_port ports[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the argument "size + size * count" in the
devm_kzalloc() function.
This way, the code is more readable and safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
drivers/net/wwan/t7xx/t7xx_port_proxy.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
index 8f5e01705af2..7d6388bf1d7c 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
@@ -543,8 +543,10 @@ static int t7xx_proxy_alloc(struct t7xx_modem *md)
struct device *dev = &md->t7xx_dev->pdev->dev;
struct port_proxy *port_prox;
- port_prox = devm_kzalloc(dev, sizeof(*port_prox) +
-sizeof(struct t7xx_port) *
T7XX_MAX_POSSIBLE_PORTS_NUM,
+ port_prox = devm_kzalloc(dev,
+struct_size(port_prox,
+ports,
+T7XX_MAX_POSSIBLE_PORTS_NUM),
GFP_KERNEL);
if (!port_prox)
return -ENOMEM;
--
2.25.1
[PATCH] mwl8k: Avoid overlapping composite structs that contain flex-arrays
When a struct containing a flexible array is included in another struct,
and there is a member after the struct-with-flex-array, there is a
possibility of memory overlap. These cases must be audited [1]. See:
struct inner {
...
int flex[];
};
struct outer {
...
struct inner header;
int overlap;
...
};
This is the scenario for the "struct mwl8k_cmd_pkt" structure that is
included in the following "struct mwl8k_cmd_*" structures:
struct mwl8k_cmd_get_hw_spec_sta
struct mwl8k_cmd_get_hw_spec_ap
struct mwl8k_cmd_set_hw_spec
struct mwl8k_cmd_mac_multicast_adr
struct mwl8k_cmd_get_stat
struct mwl8k_cmd_radio_control
struct mwl8k_cmd_rf_tx_power
struct mwl8k_cmd_tx_power
struct mwl8k_cmd_rf_antenna
struct mwl8k_cmd_set_beacon
struct mwl8k_cmd_bbp_reg_access
struct mwl8k_cmd_set_post_scan
struct mwl8k_cmd_set_rf_channel
struct mwl8k_cmd_update_set_aid
struct mwl8k_cmd_set_rate
struct mwl8k_cmd_finalize_join
struct mwl8k_cmd_set_rts_threshold
struct mwl8k_cmd_set_slot
struct mwl8k_cmd_set_edca_params
struct mwl8k_cmd_set_wmm_mode
struct mwl8k_cmd_mimo_config
struct mwl8k_cmd_use_fixed_rate_sta
struct mwl8k_cmd_use_fixed_rate_ap
struct mwl8k_cmd_enable_sniffer
struct mwl8k_cmd_update_mac_addr
struct mwl8k_cmd_set_rate_adapt_mode
struct mwl8k_cmd_get_watchdog_bitmap
struct mwl8k_cmd_bss_start
struct mwl8k_cmd_bastream
struct mwl8k_cmd_set_new_stn
struct mwl8k_cmd_update_encryption
struct mwl8k_cmd_set_key
struct mwl8k_cmd_update_stadb
The pattern is like the one shown below:
struct mwl8k_cmd_pkt {
...
char payload[];
} __packed;
struct mwl8k_cmd_* {
struct mwl8k_cmd_pkt header;
...
};
In this case, because the flexible array "payload" is only used in the
"mwl8k_load_fw_image" function, it is best to define a new structure for
the packet header called "struct mwl8k_cmd_pkt_hdr". This way, the
"struct mwl8k_cmd_pkt" and all the affected "struct mwl8k_cmd_*" used
for commands can now be defined using this new header structure.
For consistency, although the "struct mwl8k_cmd_set_pre_scan" does not
suffer from the overlapping scenario, also use the new header structure
to define it.
Moreover, change the prototype of the "mwl8k_post_cmd" function and the
"mwl8k_post_pervif_cmd" function because it is not necessary to pass the
whole packet structure. It is enough to use only the header structure.
Also, change the return type of the "__mwl8k_cmd_mac_multicast_adr"
function for the same reason.
As a final point, refactor the necessary calls, use the new members of
the structures and change some variable names and types to achieve the
desired goal.
Link: https://github.com/KSPP/linux/issues/202 [1]
Signed-off-by: Erick Archer
---
Hi everyone,
This patch is based on my understanding of the code. Any comments would
be greatly appreciated.
Also, I have verified that the old and new packet structure are the same
size:
struct mwl8k_cmd_pkt_old {
__le16 code;
__le16 length;
__u8seq_num;
__u8macid;
__le16 result;
charpayload[];
} __packed;
struct mwl8k_cmd_pkt_hdr {
__le16 code;
__le16 length;
__u8seq_num;
__u8macid;
__le16 result;
} __packed;
struct mwl8k_cmd_pkt_new {
struct mwl8k_cmd_pkt_hdr header;
char payload[];
} __packed;
static_assert(sizeof(struct mwl8k_cmd_pkt_old) == sizeof(struct
mwl8k_cmd_pkt_new));
Best regards,
Erick
---
drivers/net/wireless/marvell/mwl8k.c | 145 ++-
1 file changed, 75 insertions(+), 70 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwl8k.c
b/drivers/net/wireless/marvell/mwl8k.c
index ce8fea76dbb2..57de32ba4efc 100644
--- a/drivers/net/wireless/marvell/mwl8k.c
+++ b/drivers/net/wireless/marvell/mwl8k.c
@@ -586,13 +586,17 @@ static int mwl8k_request_firmware(struct mwl8k_priv
*priv, char *fw_image,
return 0;
}
-struct mwl8k_cmd_pkt {
+struct mwl8k_cmd_pkt_hdr {
__le16 code;
__le16 length;
__u8seq_num;
__u8macid;
__le16 result;
- charpayload[];
+} __packed;
+
+struct mwl8k_cmd_pkt {
+ struct mwl8k_cmd_pkt_hdr header;
+ char payload[];
} __packed;
/*
@@ -652,17 +656,17 @@ static int mwl8k_load_fw_image(struct mwl8k_priv *priv,
if (cmd == NULL)
return -ENOMEM;
- cmd->code = cpu_to_le16(MWL8K_CMD_CODE_DNLD);
- cmd->seq_num = 0;
- cmd->macid = 0;
- cmd->result = 0;
+ cmd->header.code = cpu_to_le16(MWL8K_CMD_CODE_DNLD);
+ cmd->header.seq_num = 0;
+ cmd->header.macid = 0;
+ cmd->header.result = 0;
done = 0;
while (length) {
int block_size = length > 256 ? 256 : length;
memcpy(cmd->payload,
Re: [PATCH] mwl8k: Avoid overlapping composite structs that contain flex-arrays
Hi Gustavo,
On Sat, Mar 16, 2024 at 12:59:11PM -0600, Gustavo A. R. Silva wrote:
>
> [..]
>
> >
> > Link: https://github.com/KSPP/linux/issues/202 [1]
> > Signed-off-by: Erick Archer
> > ---
> > Hi everyone,
> >
> > This patch is based on my understanding of the code. Any comments would
> > be greatly appreciated.
>
> Thanks for looking into this. :)
>
> I'm currently in the process of trying a general solution for all these
> composite structures without having to use two separate structs, avoid too
> much code churn, and continue allowing for __counted_by() annotations at
> the same time.
I searched the mailing list and found several of your patches:
Link: https://lore.kernel.org/linux-hardening/ZfCXBykRw5XqBvf0@neat/
Link:
https://lore.kernel.org/linux-hardening/cover.1709658886.git.gustavo...@kernel.org/
Link: https://lore.kernel.org/linux-hardening/ZeeaRuTpuxInH6ZB@neat/
In all of them you use the `struct_group_tagged()` helper to solve the
overlapping scenario. Great proposal ;)
> I'll be sending a bunch of patches once the merge window closes. So, for
> now, I think it's wise to wait for those patches.
So, are you working in a patch for the "mwl8k"? Or do you prefer
a v2 of this patch based on your proposal?
>
> More comments below.
>
> [..]
>
> > diff --git a/drivers/net/wireless/marvell/mwl8k.c
> > b/drivers/net/wireless/marvell/mwl8k.c
> > index ce8fea76dbb2..57de32ba4efc 100644
> > --- a/drivers/net/wireless/marvell/mwl8k.c
> > +++ b/drivers/net/wireless/marvell/mwl8k.c
> > @@ -586,13 +586,17 @@ static int mwl8k_request_firmware(struct mwl8k_priv
> > *priv, char *fw_image,
> > return 0;
> > }
> >
> > -struct mwl8k_cmd_pkt {
> > +struct mwl8k_cmd_pkt_hdr {
> > __le16 code;
> > __le16 length;
> > __u8seq_num;
> > __u8macid;
> > __le16 result;
> > - charpayload[];
> > +} __packed;
> > +
> > +struct mwl8k_cmd_pkt {
> > + struct mwl8k_cmd_pkt_hdr header;
> > + char payload[];
> > } __packed;
>
> One of the problems with this is that `struct mwl8k_cmd_pkt` is candidate for
> a
> `__counted_by()` annotation:
>
> @@ -592,7 +592,7 @@ struct mwl8k_cmd_pkt {
> __u8seq_num;
> __u8macid;
> __le16 result;
> - charpayload[];
> + charpayload[] __counted_by(length);
> } __packed;
>
> and with the changes you propose, that is not possible anymore because the
> counter
> member must be at the same level or in an anonymous struct also at the same
> level
> as `payload`.
Ok, I understand the problem you raise and I agree.
Anyway, thanks for your comments.
Best regards,
Erick
> Thanks
> --
> Gustavo
>
[PATCH] perf/x86/rapl: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "rapl_pmus" variable is a pointer to "struct rapl_pmus" and
this structure ends in a flexible array:
struct rapl_pmus {
[...]
struct rapl_pmu *pmus[] __counted_by(maxdie);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() function.
This way, the code is more readable and safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
arch/x86/events/rapl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index fb2b1961e5a3..8ef08b5d55a7 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -675,10 +675,8 @@ static const struct attribute_group *rapl_attr_update[] = {
static int __init init_rapl_pmus(void)
{
int maxdie = topology_max_packages() * topology_max_dies_per_package();
- size_t size;
- size = sizeof(*rapl_pmus) + maxdie * sizeof(struct rapl_pmu *);
- rapl_pmus = kzalloc(size, GFP_KERNEL);
+ rapl_pmus = kzalloc(struct_size(rapl_pmus, pmus, maxdie), GFP_KERNEL);
if (!rapl_pmus)
return -ENOMEM;
--
2.25.1
[PATCH] perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "box" variable is a pointer to "struct intel_uncore_box" and
this structure ends in a flexible array:
struct intel_uncore_box {
[...]
struct intel_uncore_extra_reg shared_regs[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc_node() function.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Hi,
The Coccinelle script used to detect this code pattern is the following:
virtual report
@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~
"kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@
i0 = sizeof(t1) + sizeof(t2) * i1;
...
i2 = ALLOC@p1(..., i0, ...);
@script:python depends on report@
p1 << rule1.p1;
@@
msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)
Regards,
Erick
---
arch/x86/events/intel/uncore.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 258e2cdf28fa..ce756d24c370 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -350,12 +350,11 @@ static void uncore_pmu_init_hrtimer(struct
intel_uncore_box *box)
static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type
*type,
int node)
{
- int i, size, numshared = type->num_shared_regs ;
+ int i, numshared = type->num_shared_regs;
struct intel_uncore_box *box;
- size = sizeof(*box) + numshared * sizeof(struct intel_uncore_extra_reg);
-
- box = kzalloc_node(size, GFP_KERNEL, node);
+ box = kzalloc_node(struct_size(box, shared_regs, numshared), GFP_KERNEL,
+ node);
if (!box)
return NULL;
--
2.25.1
[PATCH v2] dmaengine: pl08x: Use kcalloc() instead of kzalloc()
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
Here the multiplication is obviously safe because the "channels"
member can only be 8 or 2. This value is set when the "vendor_data"
structs are initialized.
static struct vendor_data vendor_pl080 = {
[...]
.channels = 8,
[...]
};
static struct vendor_data vendor_nomadik = {
[...]
.channels = 8,
[...]
};
static struct vendor_data vendor_pl080s = {
[...]
.channels = 8,
[...]
};
static struct vendor_data vendor_pl081 = {
[...]
.channels = 2,
[...]
};
However, using kcalloc() is more appropriate [1] and improves
readability. This patch has no effect on runtime behavior.
Link: https://github.com/KSPP/linux/issues/162 [1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Reviewed-by: Gustavo A. R. Silva
Signed-off-by: Erick Archer
---
Changes in v2:
- Add the "Reviewed-by:" tag.
- Rebase against linux-next.
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/20240128115236.4791-1-erick.arc...@gmx.com/
Hi everyone,
This patch seems to be lost. Gustavo reviewed it on January 30, 2024
but the patch has not been applied since.
Thanks,
Erick
---
drivers/dma/amba-pl08x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index fbf048f432bf..73a5cfb4da8a 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -2855,8 +2855,8 @@ static int pl08x_probe(struct amba_device *adev, const
struct amba_id *id)
}
/* Initialize physical channels */
- pl08x->phy_chans = kzalloc((vd->channels * sizeof(*pl08x->phy_chans)),
- GFP_KERNEL);
+ pl08x->phy_chans = kcalloc(vd->channels, sizeof(*pl08x->phy_chans),
+ GFP_KERNEL);
if (!pl08x->phy_chans) {
ret = -ENOMEM;
goto out_no_phychans;
--
2.25.1
[PATCH v2] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*()
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.
So, use the purpose specific kcalloc*() function instead of the argument
size * count in the kzalloc*() function.
[1]
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Link: https://github.com/KSPP/linux/issues/162
Reviewed-by: Gustavo A. R. Silva
Signed-off-by: Erick Archer
---
Changes in v2:
- Add the "Reviewed-by:" tag.
- Rebase against linux-next.
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/20240116125813.3754-1-erick.arc...@gmx.com/
Hi everyone,
This patch seems to be lost. Gustavo reviewed it on January 16, 2024
but the patch has not been applied since.
Thanks,
Erick
---
arch/x86/events/amd/uncore.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 4ccb8fa483e6..61c0a2114183 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -479,8 +479,8 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
goto fail;
curr->cpu = cpu;
- curr->events = kzalloc_node(sizeof(*curr->events) *
- pmu->num_counters,
+ curr->events = kcalloc_node(pmu->num_counters,
+ sizeof(*curr->events),
GFP_KERNEL, node);
if (!curr->events) {
kfree(curr);
@@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
uncore->num_pmus += group_num_pmus[gid];
}
- uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus,
+ uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus),
GFP_KERNEL);
if (!uncore->pmus) {
uncore->num_pmus = 0;
--
2.25.1
[PATCH v3] scsi: csiostor: Use kcalloc() instead of kzalloc()
Use 2-factor multiplication argument form kcalloc() instead
of kzalloc().
Also, it is preferred to use sizeof(*pointer) instead of
sizeof(type) due to the type of the variable can change and
one needs not change the former (unlike the latter).
Link: https://github.com/KSPP/linux/issues/162
Reviewed-by: Gustavo A. R. Silva
Signed-off-by: Erick Archer
---
Changes in v2:
- Update the changelog text describing the sizeof()
changes (Gustavo A. R. Silva)
Changes in v3:
- Add the "Reviewed-by:" tag.
- Rebase against linux-next.
Version 1:
Link:
https://lore.kernel.org/linux-hardening/20240112182603.11048-1-erick.arc...@gmx.com/
Version 2:
Link:
https://lore.kernel.org/linux-hardening/20240114102400.3816-1-erick.arc...@gmx.com/
Hi everyone,
This patch seems to be lost. Gustavo reviewed it on January 15, 2024
but the patch has not been applied since.
Thanks,
Erick
---
drivers/scsi/csiostor/csio_init.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/csiostor/csio_init.c
b/drivers/scsi/csiostor/csio_init.c
index d649b7a2a879..d72892e44fd1 100644
--- a/drivers/scsi/csiostor/csio_init.c
+++ b/drivers/scsi/csiostor/csio_init.c
@@ -698,8 +698,7 @@ csio_lnodes_block_request(struct csio_hw *hw)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -737,8 +736,7 @@ csio_lnodes_unblock_request(struct csio_hw *hw)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -775,8 +773,7 @@ csio_lnodes_block_by_port(struct csio_hw *hw, uint8_t
portid)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -816,8 +813,7 @@ csio_lnodes_unblock_by_port(struct csio_hw *hw, uint8_t
portid)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "Failed to allocate lnodes_list");
return;
@@ -855,8 +851,7 @@ csio_lnodes_exit(struct csio_hw *hw, bool npiv)
struct csio_lnode **lnode_list;
int cur_cnt = 0, ii;
- lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
- GFP_KERNEL);
+ lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
if (!lnode_list) {
csio_err(hw, "lnodes_exit: Failed to allocate lnodes_list.\n");
return;
--
2.25.1
[PATCH] mtd: maps: sa1100-flash: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "info" variable is a pointer to "struct sa_info" and this
structure ends in a flexible array:
struct sa_info {
[...]
struct sa_subdev_info subdev[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + size * count" in
the kzalloc() function.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Hi,
The Coccinelle script used to detect this code pattern is the following:
virtual report
@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~
"kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@
i0 = sizeof(t1) + sizeof(t2) * i1;
...
i2 = ALLOC@p1(..., i0, ...);
@script:python depends on report@
p1 << rule1.p1;
@@
msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)
Regards,
Erick
---
drivers/mtd/maps/sa1100-flash.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/maps/sa1100-flash.c b/drivers/mtd/maps/sa1100-flash.c
index d4ce2376d33f..ac8a0a19a021 100644
--- a/drivers/mtd/maps/sa1100-flash.c
+++ b/drivers/mtd/maps/sa1100-flash.c
@@ -153,7 +153,7 @@ static struct sa_info *sa1100_setup_mtd(struct
platform_device *pdev,
struct flash_platform_data *plat)
{
struct sa_info *info;
- int nr, size, i, ret = 0;
+ int nr, i, ret = 0;
/*
* Count number of devices.
@@ -167,12 +167,10 @@ static struct sa_info *sa1100_setup_mtd(struct
platform_device *pdev,
goto out;
}
- size = sizeof(struct sa_info) + sizeof(struct sa_subdev_info) * nr;
-
/*
* Allocate the map_info structs in one go.
*/
- info = kzalloc(size, GFP_KERNEL);
+ info = kzalloc(struct_size(info, subdev, nr), GFP_KERNEL);
if (!info) {
ret = -ENOMEM;
goto out;
--
2.25.1
Re: [PATCH v2] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*()
Hi Ingo,
On Sat, Mar 30, 2024 at 10:11:23PM +0100, Ingo Molnar wrote:
>
> * Erick Archer wrote:
>
> > 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.
> >
> > So, use the purpose specific kcalloc*() function instead of the argument
> > size * count in the kzalloc*() function.
> >
> > [1]
> > https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> >
> > Link: https://github.com/KSPP/linux/issues/162
> > Reviewed-by: Gustavo A. R. Silva
> > Signed-off-by: Erick Archer
> > ---
> > Changes in v2:
> > - Add the "Reviewed-by:" tag.
> > - Rebase against linux-next.
> >
> > Previous versions:
> > v1 ->
> > https://lore.kernel.org/linux-hardening/20240116125813.3754-1-erick.arc...@gmx.com/
> >
> > Hi everyone,
> >
> > This patch seems to be lost. Gustavo reviewed it on January 16, 2024
> > but the patch has not been applied since.
> >
> > Thanks,
> > Erick
> > ---
> > arch/x86/events/amd/uncore.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> > index 4ccb8fa483e6..61c0a2114183 100644
> > --- a/arch/x86/events/amd/uncore.c
> > +++ b/arch/x86/events/amd/uncore.c
> > @@ -479,8 +479,8 @@ static int amd_uncore_ctx_init(struct amd_uncore
> > *uncore, unsigned int cpu)
> > goto fail;
> >
> > curr->cpu = cpu;
> > - curr->events = kzalloc_node(sizeof(*curr->events) *
> > - pmu->num_counters,
> > + curr->events = kcalloc_node(pmu->num_counters,
> > + sizeof(*curr->events),
> > GFP_KERNEL, node);
> > if (!curr->events) {
> > kfree(curr);
> > @@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore,
> > unsigned int cpu)
> > uncore->num_pmus += group_num_pmus[gid];
> > }
> >
> > - uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus,
> > + uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus),
> >GFP_KERNEL);
> > if (!uncore->pmus) {
> > uncore->num_pmus = 0;
>
> This change is nonsense, kzalloc() is a perfectly usable interface, and
> none of the arguments are user-controlled, so I don't see how there
> could be a real overflow bug here.
Yes, you are right. This scenario is obviously safe, but this is an
effort to get rid of all multiplications from allocations functions
in the kernel. Surely, the commit message is scarier than it really
is in reality.
If you prefer I can send a v3 with the following commit message in
order to better explain the change.
[start of commit message] -
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
Here the multiplication is obviously safe. However, using kcalloc*()
is more appropriate [2] and improves readability. This patch has no
effect on runtime behavior.
Link: https://github.com/KSPP/linux/issues/162 [1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[2]
[end of commit message] ---
Best regards,
Erick
> Thanks,
>
> Ingo
[PATCH] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of
trailing elements. Specifically, it uses a "mana_handle_t" array. So,
use the preferred way in the kernel declaring a flexible array [1].
Also, avoid the open-coded arithmetic in the memory allocator functions
[2] using the "struct_size" macro.
Moreover, use the "offsetof" helper to get the indirect table offset
instead of the "sizeof" operator and avoid the open-coded arithmetic in
pointers using the new flex member.
Now, it is also possible to use the "flex_array_size" helper to compute
the size of these trailing elements in the "memcpy" function.
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays
[1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[2]
Signed-off-by: Erick Archer
---
drivers/infiniband/hw/mana/qp.c | 8
drivers/net/ethernet/microsoft/mana/mana_en.c | 9 +
include/net/mana/mana.h | 1 +
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
index 6e7627745c95..c2a39db8ef92 100644
--- a/drivers/infiniband/hw/mana/qp.c
+++ b/drivers/infiniband/hw/mana/qp.c
@@ -22,8 +22,7 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
gc = mdev_to_gc(dev);
- req_buf_size =
- sizeof(*req) + sizeof(mana_handle_t) * MANA_INDIRECT_TABLE_SIZE;
+ req_buf_size = struct_size(req, indir_tab, MANA_INDIRECT_TABLE_SIZE);
req = kzalloc(req_buf_size, GFP_KERNEL);
if (!req)
return -ENOMEM;
@@ -44,11 +43,12 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev
*dev,
req->rss_enable = true;
req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
- req->indir_tab_offset = sizeof(*req);
+ req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
+indir_tab);
req->update_indir_tab = true;
req->cqe_coalescing_enable = 1;
- req_indir_tab = (mana_handle_t *)(req + 1);
+ req_indir_tab = req->indir_tab;
/* The ind table passed to the hardware must have
* MANA_INDIRECT_TABLE_SIZE entries. Adjust the verb
* ind_table to MANA_INDIRECT_TABLE_SIZE if required
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 59287c6e6cee..04aa096c6cc4 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1062,7 +1062,7 @@ static int mana_cfg_vport_steering(struct
mana_port_context *apc,
u32 req_buf_size;
int err;
- req_buf_size = sizeof(*req) + sizeof(mana_handle_t) * num_entries;
+ req_buf_size = struct_size(req, indir_tab, num_entries);
req = kzalloc(req_buf_size, GFP_KERNEL);
if (!req)
return -ENOMEM;
@@ -1074,7 +1074,8 @@ static int mana_cfg_vport_steering(struct
mana_port_context *apc,
req->vport = apc->port_handle;
req->num_indir_entries = num_entries;
- req->indir_tab_offset = sizeof(*req);
+ req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
+indir_tab);
req->rx_enable = rx;
req->rss_enable = apc->rss_state;
req->update_default_rxobj = update_default_rxobj;
@@ -1087,9 +1088,9 @@ static int mana_cfg_vport_steering(struct
mana_port_context *apc,
memcpy(&req->hashkey, apc->hashkey, MANA_HASH_KEY_SIZE);
if (update_tab) {
- req_indir_tab = (mana_handle_t *)(req + 1);
+ req_indir_tab = req->indir_tab;
memcpy(req_indir_tab, apc->rxobj_table,
- req->num_indir_entries * sizeof(mana_handle_t));
+ flex_array_size(req, indir_tab, req->num_indir_entries));
}
err = mana_send_request(apc->ac, req, req_buf_size, &resp,
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index 76147feb0d10..20ffcae29e1e 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -671,6 +671,7 @@ struct mana_cfg_rx_steer_req_v2 {
u8 hashkey[MANA_HASH_KEY_SIZE];
u8 cqe_coalescing_enable;
u8 reserved2[7];
+ mana_handle_t indir_tab[];
}; /* HW DATA */
struct mana_cfg_rx_steer_resp {
--
2.25.1
Re: [PATCH] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
Hi Gustavo,
On Sun, Mar 31, 2024 at 08:53:07PM -0600, Gustavo A. R. Silva wrote:
>
>
> On 31/03/24 09:04, Erick Archer wrote:
> > The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of
> > trailing elements. Specifically, it uses a "mana_handle_t" array. So,
> > use the preferred way in the kernel declaring a flexible array [1].
> >
> > Also, avoid the open-coded arithmetic in the memory allocator functions
> > [2] using the "struct_size" macro.
> >
> > Moreover, use the "offsetof" helper to get the indirect table offset
> > instead of the "sizeof" operator and avoid the open-coded arithmetic in
> > pointers using the new flex member.
> >
> > Now, it is also possible to use the "flex_array_size" helper to compute
> > the size of these trailing elements in the "memcpy" function.
> >
> > Link:
> > https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays
> > [1]
> > Link:
> > https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> > [2]
> > Signed-off-by: Erick Archer
> > ---
> > drivers/infiniband/hw/mana/qp.c | 8
> > drivers/net/ethernet/microsoft/mana/mana_en.c | 9 +
> > include/net/mana/mana.h | 1 +
> > 3 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mana/qp.c
> > b/drivers/infiniband/hw/mana/qp.c
> > index 6e7627745c95..c2a39db8ef92 100644
> > --- a/drivers/infiniband/hw/mana/qp.c
> > +++ b/drivers/infiniband/hw/mana/qp.c
> > @@ -22,8 +22,7 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev
> > *dev,
> > gc = mdev_to_gc(dev);
> > - req_buf_size =
> > - sizeof(*req) + sizeof(mana_handle_t) * MANA_INDIRECT_TABLE_SIZE;
> > + req_buf_size = struct_size(req, indir_tab, MANA_INDIRECT_TABLE_SIZE);
> > req = kzalloc(req_buf_size, GFP_KERNEL);
> > if (!req)
> > return -ENOMEM;
> > @@ -44,11 +43,12 @@ static int mana_ib_cfg_vport_steering(struct
> > mana_ib_dev *dev,
> > req->rss_enable = true;
> > req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
> > - req->indir_tab_offset = sizeof(*req);
> > + req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
> > +indir_tab);
> > req->update_indir_tab = true;
> > req->cqe_coalescing_enable = 1;
> > - req_indir_tab = (mana_handle_t *)(req + 1);
> > + req_indir_tab = req->indir_tab;
>
> It seems that `req_indir_tab` can be removed, and `req->indir_tab` be
> directly used.
It seems reasonable to me. Thanks for looking at this.
Regards,
Erick
>
> > /* The ind table passed to the hardware must have
> > * MANA_INDIRECT_TABLE_SIZE entries. Adjust the verb
> > * ind_table to MANA_INDIRECT_TABLE_SIZE if required
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 59287c6e6cee..04aa096c6cc4 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -1062,7 +1062,7 @@ static int mana_cfg_vport_steering(struct
> > mana_port_context *apc,
> > u32 req_buf_size;
> > int err;
> > - req_buf_size = sizeof(*req) + sizeof(mana_handle_t) * num_entries;
> > + req_buf_size = struct_size(req, indir_tab, num_entries);
> > req = kzalloc(req_buf_size, GFP_KERNEL);
> > if (!req)
> > return -ENOMEM;
> > @@ -1074,7 +1074,8 @@ static int mana_cfg_vport_steering(struct
> > mana_port_context *apc,
> > req->vport = apc->port_handle;
> > req->num_indir_entries = num_entries;
> > - req->indir_tab_offset = sizeof(*req);
> > + req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
> > +indir_tab);
> > req->rx_enable = rx;
> > req->rss_enable = apc->rss_state;
> > req->update_default_rxobj = update_default_rxobj;
> > @@ -1087,9 +1088,9 @@ static int mana_cfg_vport_steering(struct
> > mana_port_context *apc,
> > memcpy(&req->hashkey, apc->hashkey, MANA_HASH_KEY_SIZE);
> > if (update_tab) {
> > - req_indir_tab = (mana_handle_t *)(req + 1);
> > + req_indir_tab = req->indir_tab;
>
> Ditto.
>
> T
[PATCH v2] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of
trailing elements. Specifically, it uses a "mana_handle_t" array. So,
use the preferred way in the kernel declaring a flexible array [1].
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
Also, avoid the open-coded arithmetic in the memory allocator functions
[2] using the "struct_size" macro.
Moreover, use the "offsetof" helper to get the indirect table offset
instead of the "sizeof" operator and avoid the open-coded arithmetic in
pointers using the new flex member. This new structure member also allow
us to remove the "req_indir_tab" variable since it is no longer needed.
Now, it is also possible to use the "flex_array_size" helper to compute
the size of these trailing elements in the "memcpy" function.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays
[1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[2]
Signed-off-by: Erick Archer
---
Changes in v2:
- Remove the "req_indir_tab" variable (Gustavo A. R. Silva).
- Update the commit message.
- Add the "__counted_by" attribute.
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237974ef1b9bafa618166c38b...@as8pr02mb7237.eurprd02.prod.outlook.com/
Hi,
The Coccinelle script used to detect this code pattern is the following:
virtual report
@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~
"kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@
i0 = sizeof(t1) + sizeof(t2) * i1;
...
i2 = ALLOC@p1(..., i0, ...);
@script:python depends on report@
p1 << rule1.p1;
@@
msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)
Regards,
Erick
---
drivers/infiniband/hw/mana/qp.c | 12 +---
drivers/net/ethernet/microsoft/mana/mana_en.c | 14 ++
include/net/mana/mana.h | 1 +
3 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
index 6e7627745c95..258f89464c10 100644
--- a/drivers/infiniband/hw/mana/qp.c
+++ b/drivers/infiniband/hw/mana/qp.c
@@ -15,15 +15,13 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev
*dev,
struct mana_port_context *mpc = netdev_priv(ndev);
struct mana_cfg_rx_steer_req_v2 *req;
struct mana_cfg_rx_steer_resp resp = {};
- mana_handle_t *req_indir_tab;
struct gdma_context *gc;
u32 req_buf_size;
int i, err;
gc = mdev_to_gc(dev);
- req_buf_size =
- sizeof(*req) + sizeof(mana_handle_t) * MANA_INDIRECT_TABLE_SIZE;
+ req_buf_size = struct_size(req, indir_tab, MANA_INDIRECT_TABLE_SIZE);
req = kzalloc(req_buf_size, GFP_KERNEL);
if (!req)
return -ENOMEM;
@@ -44,20 +42,20 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev
*dev,
req->rss_enable = true;
req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
- req->indir_tab_offset = sizeof(*req);
+ req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
+indir_tab);
req->update_indir_tab = true;
req->cqe_coalescing_enable = 1;
- req_indir_tab = (mana_handle_t *)(req + 1);
/* The ind table passed to the hardware must have
* MANA_INDIRECT_TABLE_SIZE entries. Adjust the verb
* ind_table to MANA_INDIRECT_TABLE_SIZE if required
*/
ibdev_dbg(&dev->ib_dev, "ind table size %u\n", 1 << log_ind_tbl_size);
for (i = 0; i < MANA_INDIRECT_TABLE_SIZE; i++) {
- req_indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
+ req->indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
ibdev_dbg(&dev->ib_dev, "index %u handle 0x%llx\n", i,
- req_indir_tab[i]);
+ req->indir_tab[i]);
}
req->update_hashkey = true;
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 59287c6e6cee..62bf3e5661a6 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1058,11 +1058,
[PATCH v3 0/3] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of trailing elements. Specifically, it uses a "mana_handle_t" array. So, use the preferred way in the kernel declaring a flexible array [1]. At the same time, prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). Also, avoid the open-coded arithmetic in the memory allocator functions [2] using the "struct_size" macro. Moreover, use the "offsetof" helper to get the indirect table offset instead of the "sizeof" operator and avoid the open-coded arithmetic in pointers using the new flex member. This new structure member also allow us to remove the "req_indir_tab" variable since it is no longer needed. Now, it is also possible to use the "flex_array_size" helper to compute the size of these trailing elements in the "memcpy" function. Specifically, the first commit adds the flex member and the patches 2 and 3 refactor the consumers of the "struct mana_cfg_rx_steer_req_v2". This code was detected with the help of Coccinelle, and audited and modified manually. The Coccinelle script used to detect this code pattern is the following: virtual report @rule1@ type t1; type t2; identifier i0; identifier i1; identifier i2; identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; position p1; @@ i0 = sizeof(t1) + sizeof(t2) * i1; ... i2 = ALLOC@p1(..., i0, ...); @script:python depends on report@ p1 << rule1.p1; @@ msg = "WARNING: verify allocation on line %s" % (p1[0].line) coccilib.report.print_report(p1[0],msg) Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1] Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2] Signed-off-by: Erick Archer --- Changes in v3: - Split the changes in various commits to simplify the acceptance process (Leon Romanovsky). Changes in v2: - Remove the "req_indir_tab" variable (Gustavo A. R. Silva). - Update the commit message. - Add the "__counted_by" attribute. Previous versions: v1 -> https://lore.kernel.org/linux-hardening/as8pr02mb7237974ef1b9bafa618166c38b...@as8pr02mb7237.eurprd02.prod.outlook.com/ v2 -> https://lore.kernel.org/linux-hardening/as8pr02mb723729c5a63f24c312fc9cd18b...@as8pr02mb7237.eurprd02.prod.outlook.com/ --- Erick Archer (3): net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 RDMA/mana_ib: Prefer struct_size over open coded arithmetic net: mana: Avoid open coded arithmetic drivers/infiniband/hw/mana/qp.c | 12 +--- drivers/net/ethernet/microsoft/mana/mana_en.c | 14 ++ include/net/mana/mana.h | 1 + 3 files changed, 12 insertions(+), 15 deletions(-) -- 2.25.1
[PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2
The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of
trailing elements. Specifically, it uses a "mana_handle_t" array. So,
use the preferred way in the kernel declaring a flexible array [1].
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
This is a previous step to refactor the two consumers of this structure.
drivers/infiniband/hw/mana/qp.c
drivers/net/ethernet/microsoft/mana/mana_en.c
The ultimate goal is to avoid the open-coded arithmetic in the memory
allocator functions [2] using the "struct_size" macro.
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays
[1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[2]
Signed-off-by: Erick Archer
---
include/net/mana/mana.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index 4eeedf14711b..561f6719fb4e 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -670,6 +670,7 @@ struct mana_cfg_rx_steer_req_v2 {
u8 hashkey[MANA_HASH_KEY_SIZE];
u8 cqe_coalescing_enable;
u8 reserved2[7];
+ mana_handle_t indir_tab[] __counted_by(num_indir_entries);
}; /* HW DATA */
struct mana_cfg_rx_steer_resp {
--
2.25.1
[PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "req" variable is a pointer to "struct mana_cfg_rx_steer_req_v2"
and this structure ends in a flexible array:
struct mana_cfg_rx_steer_req_v2 {
[...]
mana_handle_t indir_tab[] __counted_by(num_indir_entries);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + size * count" in
the kzalloc() function.
Moreover, use the "offsetof" helper to get the indirect table offset
instead of the "sizeof" operator and avoid the open-coded arithmetic in
pointers using the new flex member. This new structure member also allow
us to remove the "req_indir_tab" variable since it is no longer needed.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
drivers/infiniband/hw/mana/qp.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
index ef0a6dc664d0..4cd8f8afe80d 100644
--- a/drivers/infiniband/hw/mana/qp.c
+++ b/drivers/infiniband/hw/mana/qp.c
@@ -15,15 +15,13 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev
*dev,
struct mana_port_context *mpc = netdev_priv(ndev);
struct mana_cfg_rx_steer_req_v2 *req;
struct mana_cfg_rx_steer_resp resp = {};
- mana_handle_t *req_indir_tab;
struct gdma_context *gc;
u32 req_buf_size;
int i, err;
gc = mdev_to_gc(dev);
- req_buf_size =
- sizeof(*req) + sizeof(mana_handle_t) * MANA_INDIRECT_TABLE_SIZE;
+ req_buf_size = struct_size(req, indir_tab, MANA_INDIRECT_TABLE_SIZE);
req = kzalloc(req_buf_size, GFP_KERNEL);
if (!req)
return -ENOMEM;
@@ -44,20 +42,20 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev
*dev,
req->rss_enable = true;
req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
- req->indir_tab_offset = sizeof(*req);
+ req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
+indir_tab);
req->update_indir_tab = true;
req->cqe_coalescing_enable = 1;
- req_indir_tab = (mana_handle_t *)(req + 1);
/* The ind table passed to the hardware must have
* MANA_INDIRECT_TABLE_SIZE entries. Adjust the verb
* ind_table to MANA_INDIRECT_TABLE_SIZE if required
*/
ibdev_dbg(&dev->ib_dev, "ind table size %u\n", 1 << log_ind_tbl_size);
for (i = 0; i < MANA_INDIRECT_TABLE_SIZE; i++) {
- req_indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
+ req->indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
ibdev_dbg(&dev->ib_dev, "index %u handle 0x%llx\n", i,
- req_indir_tab[i]);
+ req->indir_tab[i]);
}
req->update_hashkey = true;
--
2.25.1
[PATCH v3 3/3] net: mana: Avoid open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "req" variable is a pointer to "struct mana_cfg_rx_steer_req_v2"
and this structure ends in a flexible array:
struct mana_cfg_rx_steer_req_v2 {
[...]
mana_handle_t indir_tab[] __counted_by(num_indir_entries);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + size * count" in
the kzalloc() function.
Moreover, use the "offsetof" helper to get the indirect table offset
instead of the "sizeof" operator and avoid the open-coded arithmetic in
pointers using the new flex member. This new structure member also allow
us to remove the "req_indir_tab" variable since it is no longer needed.
Now, it is also possible to use the "flex_array_size" helper to compute
the size of these trailing elements in the "memcpy" function.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
drivers/net/ethernet/microsoft/mana/mana_en.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
b/drivers/net/ethernet/microsoft/mana/mana_en.c
index d8af5e7e15b4..f2fae659bf3b 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1058,11 +1058,10 @@ static int mana_cfg_vport_steering(struct
mana_port_context *apc,
struct mana_cfg_rx_steer_req_v2 *req;
struct mana_cfg_rx_steer_resp resp = {};
struct net_device *ndev = apc->ndev;
- mana_handle_t *req_indir_tab;
u32 req_buf_size;
int err;
- req_buf_size = sizeof(*req) + sizeof(mana_handle_t) * num_entries;
+ req_buf_size = struct_size(req, indir_tab, num_entries);
req = kzalloc(req_buf_size, GFP_KERNEL);
if (!req)
return -ENOMEM;
@@ -1074,7 +1073,8 @@ static int mana_cfg_vport_steering(struct
mana_port_context *apc,
req->vport = apc->port_handle;
req->num_indir_entries = num_entries;
- req->indir_tab_offset = sizeof(*req);
+ req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
+indir_tab);
req->rx_enable = rx;
req->rss_enable = apc->rss_state;
req->update_default_rxobj = update_default_rxobj;
@@ -1086,11 +1086,9 @@ static int mana_cfg_vport_steering(struct
mana_port_context *apc,
if (update_key)
memcpy(&req->hashkey, apc->hashkey, MANA_HASH_KEY_SIZE);
- if (update_tab) {
- req_indir_tab = (mana_handle_t *)(req + 1);
- memcpy(req_indir_tab, apc->rxobj_table,
- req->num_indir_entries * sizeof(mana_handle_t));
- }
+ if (update_tab)
+ memcpy(req->indir_tab, apc->rxobj_table,
+ flex_array_size(req, indir_tab, req->num_indir_entries));
err = mana_send_request(apc->ac, req, req_buf_size, &resp,
sizeof(resp));
--
2.25.1
Re: [PATCH v2] dmaengine: pl08x: Use kcalloc() instead of kzalloc()
Hi Vinod,
On Sun, Apr 07, 2024 at 05:09:39PM +0530, Vinod Koul wrote:
> On 30-03-24, 16:23, Erick Archer wrote:
> > This is an effort to get rid of all multiplications from allocation
> > functions in order to prevent integer overflows [1].
> >
> > Here the multiplication is obviously safe because the "channels"
> > member can only be 8 or 2. This value is set when the "vendor_data"
> > structs are initialized.
> >
> > static struct vendor_data vendor_pl080 = {
> > [...]
> > .channels = 8,
> > [...]
> > };
> >
> > static struct vendor_data vendor_nomadik = {
> > [...]
> > .channels = 8,
> > [...]
> > };
> >
> > static struct vendor_data vendor_pl080s = {
> > [...]
> > .channels = 8,
> > [...]
> > };
> >
> > static struct vendor_data vendor_pl081 = {
> > [...]
> > .channels = 2,
> > [...]
> > };
> >
> > However, using kcalloc() is more appropriate [1] and improves
> > readability. This patch has no effect on runtime behavior.
> >
> > Link: https://github.com/KSPP/linux/issues/162 [1]
> > Link:
> > https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> > [1]
> > Reviewed-by: Gustavo A. R. Silva
> > Signed-off-by: Erick Archer
> > ---
> > Changes in v2:
> > - Add the "Reviewed-by:" tag.
> > - Rebase against linux-next.
> >
> > Previous versions:
> > v1 ->
> > https://lore.kernel.org/linux-hardening/20240128115236.4791-1-erick.arc...@gmx.com/
> >
> > Hi everyone,
> >
> > This patch seems to be lost. Gustavo reviewed it on January 30, 2024
> > but the patch has not been applied since.
> >
> > Thanks,
> > Erick
> > ---
> > drivers/dma/amba-pl08x.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> > index fbf048f432bf..73a5cfb4da8a 100644
> > --- a/drivers/dma/amba-pl08x.c
> > +++ b/drivers/dma/amba-pl08x.c
> > @@ -2855,8 +2855,8 @@ static int pl08x_probe(struct amba_device *adev,
> > const struct amba_id *id)
> > }
> >
> > /* Initialize physical channels */
> > - pl08x->phy_chans = kzalloc((vd->channels * sizeof(*pl08x->phy_chans)),
> > - GFP_KERNEL);
> > + pl08x->phy_chans = kcalloc(vd->channels, sizeof(*pl08x->phy_chans),
> > + GFP_KERNEL);
>
> How is one better than the other?
>
The advantage of kcalloc is that it will prevent integer overflows that
could result from the multiplication of number of elements and size, and
it is also a bit nicer to read.
However, in this case the multiplication is obviously safe but the best
practice is to use the two factor form if available (as explained in the
section "open-coded arithmetic in allocator arguments" of the "Deprecated
Interfaces, Language Features, Attributes, and Conventions [1]"
[1]
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Regards,
Erick
>
> > if (!pl08x->phy_chans) {
> > ret = -ENOMEM;
> > goto out_no_phychans;
> > --
> > 2.25.1
>
> --
> ~Vinod
Re: [PATCH] perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
On Sat, Mar 30, 2024 at 03:32:59PM +0100, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1][2].
>
> As the "box" variable is a pointer to "struct intel_uncore_box" and
> this structure ends in a flexible array:
>
> struct intel_uncore_box {
> [...]
> struct intel_uncore_extra_reg shared_regs[];
> };
>
> the preferred way in the kernel is to use the struct_size() helper to
> do the arithmetic instead of the calculation "size + count * size" in
> the kzalloc_node() function.
>
> This way, the code is more readable and safer.
>
> This code was detected with the help of Coccinelle, and audited and
> modified manually.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> [1]
> Link: https://github.com/KSPP/linux/issues/160 [2]
> Signed-off-by: Erick Archer
Friendly ping. Any comments?
Regards,
Erick
Re: [PATCH v3] scsi: csiostor: Use kcalloc() instead of kzalloc()
On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote: > Use 2-factor multiplication argument form kcalloc() instead > of kzalloc(). > > Also, it is preferred to use sizeof(*pointer) instead of > sizeof(type) due to the type of the variable can change and > one needs not change the former (unlike the latter). > > Link: https://github.com/KSPP/linux/issues/162 > Reviewed-by: Gustavo A. R. Silva > Signed-off-by: Erick Archer Friendly ping. Any comments? Regards, Erick
[PATCH] Input: ff-core - prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "ff" variable is a pointer to "struct ff_device" and this
structure ends in a flexible array:
struct ff_device {
[...]
struct file *effect_owners[] __counted_by(max_effects);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() function.
The struct_size() helper returns SIZE_MAX on overflow. So, refactor
the comparison to take advantage of this.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Hi,
The Coccinelle script used to detect this code pattern is the following:
virtual report
@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~
"kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@
i0 = sizeof(t1) + sizeof(t2) * i1;
...
i2 = ALLOC@p1(..., i0, ...);
@script:python depends on report@
p1 << rule1.p1;
@@
msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)
Regards,
Erick
---
drivers/input/ff-core.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 16231fe080b0..609a5f01761b 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -9,8 +9,10 @@
/* #define DEBUG */
#include
+#include
#include
#include
+#include
#include
#include
@@ -315,9 +317,8 @@ int input_ff_create(struct input_dev *dev, unsigned int
max_effects)
return -EINVAL;
}
- ff_dev_size = sizeof(struct ff_device) +
- max_effects * sizeof(struct file *);
- if (ff_dev_size < max_effects) /* overflow */
+ ff_dev_size = struct_size(ff, effect_owners, max_effects);
+ if (ff_dev_size == SIZE_MAX) /* overflow */
return -EINVAL;
ff = kzalloc(ff_dev_size, GFP_KERNEL);
--
2.25.1
[PATCH v3] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*()
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
Here the multiplication is obviously safe. However, using kcalloc*()
is more appropriate [2] and improves readability. This patch has no
effect on runtime behavior.
Link: https://github.com/KSPP/linux/issues/162 [1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[2]
Reviewed-by: Gustavo A. R. Silva
Signed-off-by: Erick Archer
---
Changes in v3:
- Update the commit message to better explain the changes.
- Rebase against linux-next.
Changes in v2:
- Add the "Reviewed-by:" tag.
- Rebase against linux-next.
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/20240116125813.3754-1-erick.arc...@gmx.com
v2 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237a07d73d6d15ebf72fd8d8b...@as8pr02mb7237.eurprd02.prod.outlook.com
Hi,
This is a new try. In the v2 version Ingo explained that this change
is nonsense since kzalloc() is a perfectly usable interface and there
is no real overflow here.
Anyway, if we have the 2-factor form of the allocator, I think it is
a good practice to use it.
In this version I have updated the commit message to explain that
the code is obviusly safe in contrast with the last version where the
impression was given that there was a real overlow bug.
I hope this patch can be applied this time.
Regards,
Erick
---
arch/x86/events/amd/uncore.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 4ccb8fa483e6..61c0a2114183 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -479,8 +479,8 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
goto fail;
curr->cpu = cpu;
- curr->events = kzalloc_node(sizeof(*curr->events) *
- pmu->num_counters,
+ curr->events = kcalloc_node(pmu->num_counters,
+ sizeof(*curr->events),
GFP_KERNEL, node);
if (!curr->events) {
kfree(curr);
@@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
uncore->num_pmus += group_num_pmus[gid];
}
- uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus,
+ uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus),
GFP_KERNEL);
if (!uncore->pmus) {
uncore->num_pmus = 0;
--
2.25.1
[PATCH] sctp: prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "ids" variable is a pointer to "struct sctp_assoc_ids" and this
structure ends in a flexible array:
struct sctp_assoc_ids {
[...]
sctp_assoc_tgaids_assoc_id[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + size * count" in
the kmalloc() function.
Also, refactor the code adding the "ids_size" variable to avoid sizing
twice.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Hi,
The Coccinelle script used to detect this code pattern is the following:
virtual report
@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~
"kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@
i0 = sizeof(t1) + sizeof(t2) * i1;
...
i2 = ALLOC@p1(..., i0, ...);
@script:python depends on report@
p1 << rule1.p1;
@@
msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)
Regards,
Erick
---
net/sctp/socket.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index e416b6d3d270..64196b1dce1d 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -7119,6 +7119,7 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int
len,
struct sctp_sock *sp = sctp_sk(sk);
struct sctp_association *asoc;
struct sctp_assoc_ids *ids;
+ size_t ids_size;
u32 num = 0;
if (sctp_style(sk, TCP))
@@ -7131,11 +7132,11 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk,
int len,
num++;
}
- if (len < sizeof(struct sctp_assoc_ids) + sizeof(sctp_assoc_t) * num)
+ ids_size = struct_size(ids, gaids_assoc_id, num);
+ if (len < ids_size)
return -EINVAL;
- len = sizeof(struct sctp_assoc_ids) + sizeof(sctp_assoc_t) * num;
-
+ len = ids_size;
ids = kmalloc(len, GFP_USER | __GFP_NOWARN);
if (unlikely(!ids))
return -ENOMEM;
--
2.25.1
[PATCH] tty: rfcomm: prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "dl" variable is a pointer to "struct rfcomm_dev_list_req" and
this structure ends in a flexible array:
struct rfcomm_dev_list_req {
[...]
struct rfcomm_dev_info dev_info[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() and copy_to_user() functions.
At the same time remove the "size" variable as it is no longer needed.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Hi,
The Coccinelle script used to detect this code pattern is the following:
virtual report
@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~
"kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@
i0 = sizeof(t1) + sizeof(t2) * i1;
...
i2 = ALLOC@p1(..., i0, ...);
@script:python depends on report@
p1 << rule1.p1;
@@
msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)
Regards,
Erick
---
net/bluetooth/rfcomm/tty.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 69c75c041fe1..bdc64c8fb85b 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -504,7 +504,7 @@ static int rfcomm_get_dev_list(void __user *arg)
struct rfcomm_dev *dev;
struct rfcomm_dev_list_req *dl;
struct rfcomm_dev_info *di;
- int n = 0, size, err;
+ int n = 0, err;
u16 dev_num;
BT_DBG("");
@@ -515,9 +515,7 @@ static int rfcomm_get_dev_list(void __user *arg)
if (!dev_num || dev_num > (PAGE_SIZE * 4) / sizeof(*di))
return -EINVAL;
- size = sizeof(*dl) + dev_num * sizeof(*di);
-
- dl = kzalloc(size, GFP_KERNEL);
+ dl = kzalloc(struct_size(dl, dev_info, dev_num), GFP_KERNEL);
if (!dl)
return -ENOMEM;
@@ -542,9 +540,7 @@ static int rfcomm_get_dev_list(void __user *arg)
mutex_unlock(&rfcomm_dev_lock);
dl->dev_num = n;
- size = sizeof(*dl) + n * sizeof(*di);
-
- err = copy_to_user(arg, dl, size);
+ err = copy_to_user(arg, dl, struct_size(dl, dev_info, n));
kfree(dl);
return err ? -EFAULT : 0;
--
2.25.1
[PATCH] perf/ring_buffer: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "rb" variable is a pointer to "struct perf_buffer" and this
structure ends in a flexible array:
struct perf_buffer {
[...]
void*data_pages[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc_node() functions.
In the "rb_alloc" function defined in the else branch of the macro
#ifndef CONFIG_PERF_USE_VMALLOC
the count in the struct_size helper is the literal "1" since only one
pointer to void is allocated. Also, remove the "size" variable as it
is no longer needed.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Hi,
The Coccinelle script used to detect this code pattern is the following:
virtual report
@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~
"kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@
i0 = sizeof(t1)
...
i0 += sizeof(t2) * i1;
...
i2 = ALLOC@p1(..., i0, ...);
@script:python depends on report@
p1 << rule1.p1;
@@
msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)
Regards,
Erick
---
kernel/events/ring_buffer.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 4013408ce012..e68b02a56382 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
unsigned long size;
int i, node;
- size = sizeof(struct perf_buffer);
- size += nr_pages * sizeof(void *);
-
+ size = struct_size(rb, data_pages, nr_pages);
if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER)
goto fail;
@@ -916,15 +914,11 @@ void rb_free(struct perf_buffer *rb)
struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
{
struct perf_buffer *rb;
- unsigned long size;
void *all_buf;
int node;
- size = sizeof(struct perf_buffer);
- size += sizeof(void *);
-
node = (cpu == -1) ? cpu : cpu_to_node(cpu);
- rb = kzalloc_node(size, GFP_KERNEL, node);
+ rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
if (!rb)
goto fail;
--
2.25.1
Re: [PATCH] sctp: prefer struct_size over open coded arithmetic
Hi Kees and Xin,
On Mon, Apr 29, 2024 at 10:45:20AM -0700, Kees Cook wrote:
> On Sat, Apr 27, 2024 at 07:23:36PM +0200, Erick Archer wrote:
> > This is an effort to get rid of all multiplications from allocation
> > functions in order to prevent integer overflows [1][2].
> >
> > As the "ids" variable is a pointer to "struct sctp_assoc_ids" and this
> > structure ends in a flexible array:
> >
> > struct sctp_assoc_ids {
> __u32 gaids_number_of_ids;
> > sctp_assoc_tgaids_assoc_id[];
> > };
>
> This could gain __counted_by:
>
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index b7d91d4cf0db..836173e73401 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -1007,7 +1007,7 @@ enum sctp_sstat_state {
> */
> struct sctp_assoc_ids {
> __u32 gaids_number_of_ids;
> - sctp_assoc_tgaids_assoc_id[];
> + sctp_assoc_tgaids_assoc_id[] __counted_by(gaids_number_of_ids);
> };
>
Since this patch has been applied to the linux-next tree, I will send an
incremental one.
Thanks Kees and Xin for the review.
Regards,
Erick
[PATCH] sctp: annotate struct sctp_assoc_ids with __counted_by()
Prepare for the coming implementation by GCC and Clang of the
__counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
(for strcpy/memcpy-family functions).
Suggested-by: Kees Cook
Signed-off-by: Erick Archer
---
include/uapi/linux/sctp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index b7d91d4cf0db..836173e73401 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -1007,7 +1007,7 @@ enum sctp_sstat_state {
*/
struct sctp_assoc_ids {
__u32 gaids_number_of_ids;
- sctp_assoc_tgaids_assoc_id[];
+ sctp_assoc_tgaids_assoc_id[] __counted_by(gaids_number_of_ids);
};
/*
--
2.25.1
Re: [PATCH] perf/ring_buffer: Prefer struct_size over open coded arithmetic
On Tue, Apr 30, 2024 at 11:15:04AM +0200, Peter Zijlstra wrote: > On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > > This is an effort to get rid of all multiplications from allocation > > functions in order to prevent integer overflows [1][2]. > > So personally I detest struct_size() because I can never remember wtf it > does, whereas the code it replaces is simple and straight forward :/ Ok, I accept what you say. Anyway, I think it's a matter of preference ;) If I prepare a patch with the changes suggested by Kees to gain coverage of the __counted_by attribute, will it have a chance of being accepted? Are there any guidelines related to this attribute, such as in the case of the struct_size() helper? Regards, Erick
Re: [PATCH] perf/ring_buffer: Prefer struct_size over open coded arithmetic
On Thu, May 02, 2024 at 03:55:36PM -0700, Kees Cook wrote: > On Thu, May 02, 2024 at 11:18:37AM +0200, Peter Zijlstra wrote: > > On Wed, May 01, 2024 at 01:21:42PM -0700, Kees Cook wrote: > > > On Tue, Apr 30, 2024 at 11:15:04AM +0200, Peter Zijlstra wrote: > > > > On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > > > > > This is an effort to get rid of all multiplications from allocation > > > > > functions in order to prevent integer overflows [1][2]. > > > > > > > > So personally I detest struct_size() because I can never remember wtf it > > > > does, whereas the code it replaces is simple and straight forward :/ > > > > > > Sure, new APIs can involved a learning curve. If we can all handle > > > container_of(), we can deal with struct_size(). :) > > > > containre_of() is actually *much* shorter than typing it all out. Which > > is a benefit. > > > > struct_size() not so much. That's just obfuscation for obfuscation's > > sake. I do not agree with this. > > It's really not -- it's making sure that the calculation is semantically > sane: all the right things are being used for the struct size calculation > and things can't "drift", if types change, flex array changes, etc. It's > both a code robustness improvement and a wrap-around stopping improvement. > Also, in the "Deprecated Interfaces, Language Features, Attributes, and Conventions" [1] it says verbatim: Another common case to avoid is calculating the size of a structure with a trailing array of others structures, as in: header = kzalloc(sizeof(*header) + count * sizeof(*header->item), GFP_KERNEL); Instead, use the helper: header = kzalloc(struct_size(header, item, count), GFP_KERNEL); Therefore, if there is a convention to follow, we should not make an exception. Moreover, struct_size is widely used in the kernel and widely accepted. Also makes the code safer. So, I will send a new patch with the changes Kees proposed and I hope that it will be the first step in the adoption of struct_size in the perf and sched subsystems ;) Regards, Erick [1] https://docs.kernel.org/process/deprecated.html > -- > Kees Cook
[PATCH v2] perf/ring_buffer: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "rb" variable is a pointer to "struct perf_buffer" and this
structure ends in a flexible array:
struct perf_buffer {
[...]
void*data_pages[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc_node() functions.
In the "rb_alloc" function defined in the else branch of the macro
#ifndef CONFIG_PERF_USE_VMALLOC
the count in the struct_size helper is the literal "1" since only one
pointer to void is allocated. Also, remove the "size" variable as it
is no longer needed.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions). In this case, it is important to note
that the logic needs a little refactoring to ensure that the "nr_pages"
member is initialized before the first access to the flex array.
In one case, it is only necessary to move the assignment before the
array-writing loop while in the other case the assignment needs to be
added.
This way, the code is more safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Changes in v2:
- Annotate "struct perf_buffer" with __counted_by() attribute (Kees Cook).
- Refactor the logic to gain __counted_by() coverage (Kees Cook).
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb72372ab065ea8340d960ccc48b...@as8pr02mb7237.eurprd02.prod.outlook.com/
Hi Peter,
I know that you detest the struct_size() helper, however, as Kees
explained in v1, this change improves the robustness of the code.
Also, we will gain __counted_by() coverage.
I hope this patch can be applied this time.
Regards,
Erick
---
kernel/events/internal.h| 2 +-
kernel/events/ring_buffer.c | 14 --
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 5150d5f84c03..dc8d39b01adb 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -55,7 +55,7 @@ struct perf_buffer {
void*aux_priv;
struct perf_event_mmap_page *user_page;
- void*data_pages[];
+ void*data_pages[] __counted_by(nr_pages);
};
extern void rb_free(struct perf_buffer *rb);
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 4013408ce012..080537eff69f 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
unsigned long size;
int i, node;
- size = sizeof(struct perf_buffer);
- size += nr_pages * sizeof(void *);
-
+ size = struct_size(rb, data_pages, nr_pages);
if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER)
goto fail;
@@ -833,6 +831,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
if (!rb)
goto fail;
+ rb->nr_pages = nr_pages;
rb->user_page = perf_mmap_alloc_page(cpu);
if (!rb->user_page)
goto fail_user_page;
@@ -843,8 +842,6 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
goto fail_data_pages;
}
- rb->nr_pages = nr_pages;
-
ring_buffer_init(rb, watermark, flags);
return rb;
@@ -916,18 +913,15 @@ void rb_free(struct perf_buffer *rb)
struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
{
struct perf_buffer *rb;
- unsigned long size;
void *all_buf;
int node;
- size = sizeof(struct perf_buffer);
- size += sizeof(void *);
-
node = (cpu == -1) ? cpu : cpu_to_node(cpu);
- rb = kzalloc_node(size, GFP_KERNEL, node);
+ rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
if (!rb)
goto fail;
+ rb->nr_pages = nr_pages;
INIT_WORK(&rb->work, rb_free_work);
all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
--
2.25.1
Re: [PATCH v2] perf/ring_buffer: Prefer struct_size over open coded arithmetic
On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote:
> Le 05/05/2024 à 16:15, Erick Archer a écrit :
> > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> > index 4013408ce012..080537eff69f 100644
> > --- a/kernel/events/ring_buffer.c
> > +++ b/kernel/events/ring_buffer.c
> > @@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long
> > watermark, int cpu, int flags)
> > unsigned long size;
>
> Hi,
>
> Should size be size_t?
I'm sorry, but I don't have enough knowledge to answer this question.
The "size" variable is used as a return value by struct_size and as
a parameter to the order_base_2() and kzalloc_node() functions.
The size type for the kzalloc_node function is "size_t" but for the
order_base_2() macro it is necessary an unsigned type (since this
is expanded to "__ilog2_u32(u32 n)" or "__ilog2_u64(u64 n)").
So, I don't know if it is correct to change the type to size_t.
Maybe someone can help with this.
>
> > int i, node;
> > - size = sizeof(struct perf_buffer);
> > - size += nr_pages * sizeof(void *);
> > -
> > + size = struct_size(rb, data_pages, nr_pages);
> > if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER)
> > goto fail;
> > @@ -833,6 +831,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long
> > watermark, int cpu, int flags)
> > if (!rb)
> > goto fail;
> > + rb->nr_pages = nr_pages;
> > rb->user_page = perf_mmap_alloc_page(cpu);
> > if (!rb->user_page)
> > goto fail_user_page;
> > @@ -843,8 +842,6 @@ struct perf_buffer *rb_alloc(int nr_pages, long
> > watermark, int cpu, int flags)
> > goto fail_data_pages;
> > }
> > - rb->nr_pages = nr_pages;
> > -
> > ring_buffer_init(rb, watermark, flags);
> > return rb;
> > @@ -916,18 +913,15 @@ void rb_free(struct perf_buffer *rb)
> > struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int
> > flags)
> > {
> > struct perf_buffer *rb;
> > - unsigned long size;
> > void *all_buf;
> > int node;
> > - size = sizeof(struct perf_buffer);
> > - size += sizeof(void *);
> > -
> > node = (cpu == -1) ? cpu : cpu_to_node(cpu);
> > - rb = kzalloc_node(size, GFP_KERNEL, node);
> > + rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
> > if (!rb)
> > goto fail;
> > + rb->nr_pages = nr_pages;
>
> I don't think this is correct.
I think you are right. My bad :(
> There is already a logic in place about it a few lines below:
>
> all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
> if (!all_buf)
> goto fail_all_buf;
>
> rb->user_page = all_buf;
> rb->data_pages[0] = all_buf + PAGE_SIZE;
> if (nr_pages) { <--- here
> rb->nr_pages = 1; <---
> rb->page_order = ilog2(nr_pages);
> }
>
> I think that what is needed is to move this block just 2 lines above,
> (before rb->data_pages[0] = ...)
>
>
> I'm also wondering what should be done if nr_pages = 0.
Perhaps this is enough since we only allocate memory for one
member of the array.
@@ -916,18 +913,15 @@ void rb_free(struct perf_buffer *rb)
struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
{
struct perf_buffer *rb;
- unsigned long size;
void *all_buf;
int node;
- size = sizeof(struct perf_buffer);
- size += sizeof(void *);
-
node = (cpu == -1) ? cpu : cpu_to_node(cpu);
- rb = kzalloc_node(size, GFP_KERNEL, node);
+ rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
if (!rb)
goto fail;
+ rb->nr_pages = 1;
INIT_WORK(&rb->work, rb_free_work);
all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
I think that we don't need to deal with the "nr_pages = 0" case
since the flex array will always have a length of one.
Kees, can you help us with this?
Regards,
Erick
> CJ
>
> > INIT_WORK(&rb->work, rb_free_work);
> > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
>
[PATCH] uapi: stddef.h: Provide UAPI macros for __counted_by_{le, be}
Provide UAPI macros for UAPI structs that will gain annotations for
__counted_by_{le, be} attributes.
Signed-off-by: Erick Archer
---
include/uapi/linux/stddef.h | 8
1 file changed, 8 insertions(+)
diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
index 2ec6f35cda32..58154117d9b0 100644
--- a/include/uapi/linux/stddef.h
+++ b/include/uapi/linux/stddef.h
@@ -55,4 +55,12 @@
#define __counted_by(m)
#endif
+#ifndef __counted_by_le
+#define __counted_by_le(m)
+#endif
+
+#ifndef __counted_by_be
+#define __counted_by_be(m)
+#endif
+
#endif /* _UAPI_LINUX_STDDEF_H */
--
2.25.1
[PATCH v2] uapi: stddef.h: Provide UAPI macros for __counted_by_{le, be}
This commit can be considered an addition to commit ca7e324e8ad3
("compiler_types: add Endianness-dependent __counted_by_{le,be}") [1].
In the commit referenced above the __counted_by_{le,be}() attributes
were defined based on platform's endianness with the goal to that the
structures contain flexible arrays at the end, and the counter for,
can be annotated with these attributes.
So, this commit only provide UAPI macros for UAPI structs that will
gain annotations for __counted_by_{le, be} attributes. And it is the
previous step to be able to use these attributes in UAPI.
Link:
https://lore.kernel.org/r/20240327142241.1745989-2-aleksander.loba...@intel.com
Suggested-by: Sven Eckelmann
Signed-off-by: Erick Archer
---
Changes in v2:
- Add the "Suggested_by:" tag.
- Update the commit message to better explain the goal (Miguel Ojeda).
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237cd9ecbf7907ad8b1cc938b...@as8pr02mb7237.eurprd02.prod.outlook.com/
Hi everyone,
The goal of this commit is to be able to accept this another one [1].
And as Kees suggested in this mail [2] we need to land this patch before
the other because we will need __counted_by_be() in UAPI.
[1]
https://lore.kernel.org/linux-hardening/as8pr02mb72371f89d188b047410b755e8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
[2] https://lore.kernel.org/linux-hardening/202405060924.4001F77D@keescook/
---
include/uapi/linux/stddef.h | 8
1 file changed, 8 insertions(+)
diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
index 2ec6f35cda32..58154117d9b0 100644
--- a/include/uapi/linux/stddef.h
+++ b/include/uapi/linux/stddef.h
@@ -55,4 +55,12 @@
#define __counted_by(m)
#endif
+#ifndef __counted_by_le
+#define __counted_by_le(m)
+#endif
+
+#ifndef __counted_by_be
+#define __counted_by_be(m)
+#endif
+
#endif /* _UAPI_LINUX_STDDEF_H */
--
2.25.1
Re: [PATCH v2] perf/ring_buffer: Prefer struct_size over open coded arithmetic
Hi Kees and Christophe,
First of all, thanks for the reviews, comments and advices.
On Mon, May 06, 2024 at 09:23:15AM -0700, Kees Cook wrote:
> On Sun, May 05, 2024 at 07:31:24PM +0200, Erick Archer wrote:
> > On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote:
> > > Le 05/05/2024 à 16:15, Erick Archer a écrit :
> > > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> > > > index 4013408ce012..080537eff69f 100644
> > > > --- a/kernel/events/ring_buffer.c
> > > > +++ b/kernel/events/ring_buffer.c
> > > > @@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long
> > > > watermark, int cpu, int flags)
> > > > unsigned long size;
> > >
> > > Hi,
> > >
> > > Should size be size_t?
> >
> > I'm sorry, but I don't have enough knowledge to answer this question.
> > The "size" variable is used as a return value by struct_size and as
> > a parameter to the order_base_2() and kzalloc_node() functions.
>
> For Linux, size_t and unsigned long are the same (currently).
> Pedantically, yes, this should be size_t, but it's the same.
Thanks for this clarification. I will change the type for the next
version.
>
> > [...]
> > > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
> > > if (!all_buf)
> > > goto fail_all_buf;
> > >
> > > rb->user_page = all_buf;
> > > rb->data_pages[0] = all_buf + PAGE_SIZE;
> > > if (nr_pages) { <--- here
> > > rb->nr_pages = 1; <---
> > > rb->page_order = ilog2(nr_pages);
> > > }
> > [...]
> > I think that we don't need to deal with the "nr_pages = 0" case
> > since the flex array will always have a length of one.
> >
> > Kees, can you help us with this?
>
> Agh, this code hurt my head for a while.
>
> all_buf contains "nr_pages + 1" pages. all_buf gets attached to
> rb->user_page, and then rb->data_pages[0] points to the second page in
> all_buf... which means, I guess, that rb->data_pages does only have 1
> entry.
>
> However, the nr_pages == 0 case is weird. Currently, data_pages[0] will
> still get set (which points ... off the end of all_buf). If we
> unconditionally set rb->nr_pages to 1, we're changing the behavior. If
> we _don't_ set rb->data_pages[0], we're changing the behavior, but I
> think it's an invalid pointer anyway, so this is the safer change to
> make.
Thanks for explain things well.
> I suspect the right replacement is:
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index 4013408ce012..7d638ce76799 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -916,15 +916,11 @@ void rb_free(struct perf_buffer *rb)
> struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int
> flags)
> {
> struct perf_buffer *rb;
> - unsigned long size;
> void *all_buf;
> int node;
>
> - size = sizeof(struct perf_buffer);
> - size += sizeof(void *);
> -
> node = (cpu == -1) ? cpu : cpu_to_node(cpu);
> - rb = kzalloc_node(size, GFP_KERNEL, node);
> + rb = kzalloc_node(struct_size(rb, nr_pages, 1), GFP_KERNEL, node);
> if (!rb)
> goto fail;
>
> @@ -935,9 +931,9 @@ struct perf_buffer *rb_alloc(int nr_pages, long
> watermark, int cpu, int flags)
> goto fail_all_buf;
>
> rb->user_page = all_buf;
> - rb->data_pages[0] = all_buf + PAGE_SIZE;
> if (nr_pages) {
> rb->nr_pages = 1;
> + rb->data_pages[0] = all_buf + PAGE_SIZE;
> rb->page_order = ilog2(nr_pages);
> }
>
Ok, I'll do it like this for the next version.
>
>
> Also, why does rb_alloc() take an "int" nr_pages? The only caller has an
> unsigned long argument for nr_pages. Nothing checks for >INT_MAX that I
> can find.
Thanks for letting me know. I will take a look.
>
> --
Again, thanks,
Erick
> Kees Cook
Re: [PATCH] uapi: stddef.h: Provide UAPI macros for __counted_by_{le, be}
Hi Alexander,
On Tue, May 07, 2024 at 02:58:15PM +0200, Alexander Lobakin wrote:
> From: Erick Archer
> Date: Mon, 6 May 2024 19:42:08 +0200
>
> > Provide UAPI macros for UAPI structs that will gain annotations for
> > __counted_by_{le, be} attributes.
>
> Pls add me to Cc next time.
Ok.
> Why is this change needed? __counted_by_{le, be}() aren't used anywhere
> in the uAPI headers.
The goal of this commit is to be able to accept this another one [1].
In the "batman-adv" we annotate "struct batadv_tvlv_tt_data" with the
"__counted_by_be()" attribute.
[1]
https://lore.kernel.org/linux-hardening/as8pr02mb72371f89d188b047410b755e8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
Thanks,
Erick
>
> >
> > Signed-off-by: Erick Archer
> > ---
> > include/uapi/linux/stddef.h | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
> > index 2ec6f35cda32..58154117d9b0 100644
> > --- a/include/uapi/linux/stddef.h
> > +++ b/include/uapi/linux/stddef.h
> > @@ -55,4 +55,12 @@
> > #define __counted_by(m)
> > #endif
> >
> > +#ifndef __counted_by_le
> > +#define __counted_by_le(m)
> > +#endif
> > +
> > +#ifndef __counted_by_be
> > +#define __counted_by_be(m)
> > +#endif
> > +
> > #endif /* _UAPI_LINUX_STDDEF_H */
>
> Thanks,
> Olek
Re: [PATCH v3] scsi: csiostor: Use kcalloc() instead of kzalloc()
Hi Martin, Kees and Finn, On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote: > Use 2-factor multiplication argument form kcalloc() instead > of kzalloc(). > > Also, it is preferred to use sizeof(*pointer) instead of > sizeof(type) due to the type of the variable can change and > one needs not change the former (unlike the latter). > > Link: https://github.com/KSPP/linux/issues/162 > Reviewed-by: Gustavo A. R. Silva > Signed-off-by: Erick Archer > --- > Thank you very much for the reviews and comments. Regards, Erick
[PATCH v3] perf/ring_buffer: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "rb" variable is a pointer to "struct perf_buffer" and this
structure ends in a flexible array:
struct perf_buffer {
[...]
void*data_pages[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc_node() functions.
In the "rb_alloc" function defined in the else branch of the macro
#ifndef CONFIG_PERF_USE_VMALLOC
the count in the struct_size helper is the literal "1" since only one
pointer to void is allocated. Also, remove the "size" variable as it
is no longer needed.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions). In this case, it is important to note
that the logic needs a little refactoring to ensure that the "nr_pages"
member is initialized before the first access to the flex array.
In one case, it is only necessary to move the "nr_pages" assignment
before the array-writing loop while in the other case the access to the
flex array needs to be moved inside the if branch and after the
"nr_pages" assignment.
This way, the code is more safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Changes in v3:
- Refactor the logic, compared to the previous version, of the second
rb_alloc() function to gain __counted_by() coverage (Kees Cook and
Christophe JAILLET).
Changes in v2:
- Annotate "struct perf_buffer" with __counted_by() attribute (Kees Cook).
- Refactor the logic to gain __counted_by() coverage (Kees Cook).
Previous versions:
v2 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237569e4fbe0b26f62fdfdb8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb72372ab065ea8340d960ccc48b...@as8pr02mb7237.eurprd02.prod.outlook.com/
---
kernel/events/internal.h| 2 +-
kernel/events/ring_buffer.c | 15 ---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 5150d5f84c03..dc8d39b01adb 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -55,7 +55,7 @@ struct perf_buffer {
void*aux_priv;
struct perf_event_mmap_page *user_page;
- void*data_pages[];
+ void*data_pages[] __counted_by(nr_pages);
};
extern void rb_free(struct perf_buffer *rb);
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 4013408ce012..d123fa2096cf 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
unsigned long size;
int i, node;
- size = sizeof(struct perf_buffer);
- size += nr_pages * sizeof(void *);
-
+ size = struct_size(rb, data_pages, nr_pages);
if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER)
goto fail;
@@ -833,6 +831,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
if (!rb)
goto fail;
+ rb->nr_pages = nr_pages;
rb->user_page = perf_mmap_alloc_page(cpu);
if (!rb->user_page)
goto fail_user_page;
@@ -843,8 +842,6 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
goto fail_data_pages;
}
- rb->nr_pages = nr_pages;
-
ring_buffer_init(rb, watermark, flags);
return rb;
@@ -916,15 +913,11 @@ void rb_free(struct perf_buffer *rb)
struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
{
struct perf_buffer *rb;
- unsigned long size;
void *all_buf;
int node;
- size = sizeof(struct perf_buffer);
- size += sizeof(void *);
-
node = (cpu == -1) ? cpu : cpu_to_node(cpu);
- rb = kzalloc_node(size, GFP_KERNEL, node);
+ rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
if (!rb)
goto fail;
@@ -935,9 +928,9 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
goto fail_all_buf;
rb->user_page = all_buf;
-
Re: [PATCH] perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
Hi everyone,
On Sat, Mar 30, 2024 at 03:32:59PM +0100, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1][2].
>
> As the "box" variable is a pointer to "struct intel_uncore_box" and
> this structure ends in a flexible array:
>
> struct intel_uncore_box {
> [...]
> struct intel_uncore_extra_reg shared_regs[];
> };
>
> the preferred way in the kernel is to use the struct_size() helper to
> do the arithmetic instead of the calculation "size + count * size" in
> the kzalloc_node() function.
>
> This way, the code is more readable and safer.
>
> This code was detected with the help of Coccinelle, and audited and
> modified manually.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> [1]
> Link: https://github.com/KSPP/linux/issues/160 [2]
> Signed-off-by: Erick Archer
How could this patch be accepted?
Thanks,
Erick
[PATCH] perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
The "struct amd_uncore_ctx" can be refactored to use a flex array for
the "events" member. This way, the allocation/freeing of the memory can
be simplified.
Specifically, as the "curr" variable is a pointer to the amd_uncore_ctx
structure and it now ends up in a flexible array:
struct amd_uncore_ctx {
[...]
struct perf_event *events[];
};
the two-step allocation can be simplifief by using just one kzalloc_node
function and the struct_size() helper to do the arithmetic calculation
for the memory to be allocated.
This way, the code is more readable and safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Suggested-by: Christophe JAILLET
Signed-off-by: Erick Archer
---
Hi,
This patch can be considered v4 of this other one [1]. However, since
the patch has completely changed due to the addition of the flex array,
I have decided to make a new series and remove the "Reviewed-by:" tag
by Gustavo A. R. Silva and Kees cook.
[1]
https://lore.kernel.org/linux-hardening/paxpr02mb7248f46defa47e79677481b18b...@paxpr02mb7248.eurprd02.prod.outlook.com/
Thanks,
Erick
---
arch/x86/events/amd/uncore.c | 18 +-
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 4ccb8fa483e6..86755d16a3b2 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -37,8 +37,8 @@ static int pmu_version;
struct amd_uncore_ctx {
int refcnt;
int cpu;
- struct perf_event **events;
struct hlist_node node;
+ struct perf_event *events[];
};
struct amd_uncore_pmu {
@@ -426,10 +426,8 @@ static void amd_uncore_ctx_free(struct amd_uncore *uncore,
unsigned int cpu)
if (cpu == ctx->cpu)
cpumask_clear_cpu(cpu, &pmu->active_mask);
- if (!--ctx->refcnt) {
- kfree(ctx->events);
+ if (!--ctx->refcnt)
kfree(ctx);
- }
*per_cpu_ptr(pmu->ctx, cpu) = NULL;
}
@@ -474,19 +472,13 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
/* Allocate context if sibling does not exist */
if (!curr) {
node = cpu_to_node(cpu);
- curr = kzalloc_node(sizeof(*curr), GFP_KERNEL, node);
+ curr = kzalloc_node(struct_size(curr, events,
+ pmu->num_counters),
+ GFP_KERNEL, node);
if (!curr)
goto fail;
curr->cpu = cpu;
- curr->events = kzalloc_node(sizeof(*curr->events) *
- pmu->num_counters,
- GFP_KERNEL, node);
- if (!curr->events) {
- kfree(curr);
- goto fail;
- }
-
cpumask_set_cpu(cpu, &pmu->active_mask);
}
--
2.25.1
Re: [PATCH v3] scsi: csiostor: Use kcalloc() instead of kzalloc()
Hi James, On Sat, May 11, 2024 at 01:18:46PM +0200, Erick Archer wrote: > Hi Martin, Kees and Finn, > > On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote: > > Use 2-factor multiplication argument form kcalloc() instead > > of kzalloc(). > > > > Also, it is preferred to use sizeof(*pointer) instead of > > sizeof(type) due to the type of the variable can change and > > one needs not change the former (unlike the latter). > > > > Link: https://github.com/KSPP/linux/issues/162 > > Reviewed-by: Gustavo A. R. Silva > > Signed-off-by: Erick Archer > > --- > > > Thank you very much for the reviews and comments. Also, thanks for the comments and clarifications. Regards, Erick
[PATCH v2] tty: rfcomm: prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "dl" variable is a pointer to "struct rfcomm_dev_list_req" and
this structure ends in a flexible array:
struct rfcomm_dev_list_req {
[...]
struct rfcomm_dev_info dev_info[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() and copy_to_user() functions.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
In this case, it is important to note that the logic needs a little
refactoring to ensure that the "dev_num" member is initialized before
the first access to the flex array. Specifically, add the assignment
before the list_for_each_entry() loop.
Also remove the "size" variable as it is no longer needed and refactor
the list_for_each_entry() loop to use di[n] instead of (di + n).
This way, the code is more readable, idiomatic and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
Changes in v2:
- Add the __counted_by() attribute (Kees Cook).
- Refactor the list_for_each_entry() loop to use di[n] instead of
(di + n) (Kees Cook).
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb723725e0069f7ae8f64e876e8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
---
include/net/bluetooth/rfcomm.h | 2 +-
net/bluetooth/rfcomm/tty.c | 23 ++-
2 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/include/net/bluetooth/rfcomm.h b/include/net/bluetooth/rfcomm.h
index 99d26879b02a..c05882476900 100644
--- a/include/net/bluetooth/rfcomm.h
+++ b/include/net/bluetooth/rfcomm.h
@@ -355,7 +355,7 @@ struct rfcomm_dev_info {
struct rfcomm_dev_list_req {
u16 dev_num;
- struct rfcomm_dev_info dev_info[];
+ struct rfcomm_dev_info dev_info[] __counted_by(dev_num);
};
int rfcomm_dev_ioctl(struct sock *sk, unsigned int cmd, void __user *arg);
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 69c75c041fe1..af80d599c337 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -504,7 +504,7 @@ static int rfcomm_get_dev_list(void __user *arg)
struct rfcomm_dev *dev;
struct rfcomm_dev_list_req *dl;
struct rfcomm_dev_info *di;
- int n = 0, size, err;
+ int n = 0, err;
u16 dev_num;
BT_DBG("");
@@ -515,12 +515,11 @@ static int rfcomm_get_dev_list(void __user *arg)
if (!dev_num || dev_num > (PAGE_SIZE * 4) / sizeof(*di))
return -EINVAL;
- size = sizeof(*dl) + dev_num * sizeof(*di);
-
- dl = kzalloc(size, GFP_KERNEL);
+ dl = kzalloc(struct_size(dl, dev_info, dev_num), GFP_KERNEL);
if (!dl)
return -ENOMEM;
+ dl->dev_num = dev_num;
di = dl->dev_info;
mutex_lock(&rfcomm_dev_lock);
@@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg)
list_for_each_entry(dev, &rfcomm_dev_list, list) {
if (!tty_port_get(&dev->port))
continue;
- (di + n)->id = dev->id;
- (di + n)->flags = dev->flags;
- (di + n)->state = dev->dlc->state;
- (di + n)->channel = dev->channel;
- bacpy(&(di + n)->src, &dev->src);
- bacpy(&(di + n)->dst, &dev->dst);
+ di[n].id = dev->id;
+ di[n].flags = dev->flags;
+ di[n].state = dev->dlc->state;
+ di[n].channel = dev->channel;
+ bacpy(&di[n].src, &dev->src);
+ bacpy(&di[n].dst, &dev->dst);
tty_port_put(&dev->port);
if (++n >= dev_num)
break;
@@ -542,9 +541,7 @@ static int rfcomm_get_dev_list(void __user *arg)
mutex_unlock(&rfcomm_dev_lock);
dl->dev_num = n;
- size = sizeof(*dl) + n * sizeof(*di);
-
- err = copy_to_user(arg, dl, size);
+ err = copy_to_user(arg, dl, struct_size(dl, dev_info, n));
kfree(dl);
return err ? -EFAULT : 0;
--
2.25.1
[PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "dl" variable is a pointer to "struct hci_dev_list_req" and this
structure ends in a flexible array:
struct hci_dev_list_req {
[...]
struct hci_dev_req dev_req[]; /* hci_dev_req structures */
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() and copy_to_user() functions.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
In this case, it is important to note that the logic needs a little
refactoring to ensure that the "dev_num" member is initialized before
the first access to the flex array. Specifically, add the assignment
before the list_for_each_entry() loop.
Also remove the "size" variable as it is no longer needed and refactor
the list_for_each_entry() loop to use dr[n] instead of (dr + n).
This way, the code is more readable, idiomatic and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
include/net/bluetooth/hci_sock.h | 2 +-
net/bluetooth/hci_core.c | 15 ++-
2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/include/net/bluetooth/hci_sock.h b/include/net/bluetooth/hci_sock.h
index 9949870f7d78..13e8cd4414a1 100644
--- a/include/net/bluetooth/hci_sock.h
+++ b/include/net/bluetooth/hci_sock.h
@@ -144,7 +144,7 @@ struct hci_dev_req {
struct hci_dev_list_req {
__u16 dev_num;
- struct hci_dev_req dev_req[]; /* hci_dev_req structures */
+ struct hci_dev_req dev_req[] __counted_by(dev_num);
};
struct hci_conn_list_req {
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index adfd53a9fcd4..cae8a67bcd62 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -837,7 +837,7 @@ int hci_get_dev_list(void __user *arg)
struct hci_dev *hdev;
struct hci_dev_list_req *dl;
struct hci_dev_req *dr;
- int n = 0, size, err;
+ int n = 0, err;
__u16 dev_num;
if (get_user(dev_num, (__u16 __user *) arg))
@@ -846,12 +846,11 @@ int hci_get_dev_list(void __user *arg)
if (!dev_num || dev_num > (PAGE_SIZE * 2) / sizeof(*dr))
return -EINVAL;
- size = sizeof(*dl) + dev_num * sizeof(*dr);
-
- dl = kzalloc(size, GFP_KERNEL);
+ dl = kzalloc(struct_size(dl, dev_req, dev_num), GFP_KERNEL);
if (!dl)
return -ENOMEM;
+ dl->dev_num = dev_num;
dr = dl->dev_req;
read_lock(&hci_dev_list_lock);
@@ -865,8 +864,8 @@ int hci_get_dev_list(void __user *arg)
if (hci_dev_test_flag(hdev, HCI_AUTO_OFF))
flags &= ~BIT(HCI_UP);
- (dr + n)->dev_id = hdev->id;
- (dr + n)->dev_opt = flags;
+ dr[n].dev_id = hdev->id;
+ dr[n].dev_opt = flags;
if (++n >= dev_num)
break;
@@ -874,9 +873,7 @@ int hci_get_dev_list(void __user *arg)
read_unlock(&hci_dev_list_lock);
dl->dev_num = n;
- size = sizeof(*dl) + n * sizeof(*dr);
-
- err = copy_to_user(arg, dl, size);
+ err = copy_to_user(arg, dl, struct_size(dl, dev_req, n));
kfree(dl);
return err ? -EFAULT : 0;
--
2.25.1
[PATCH] net: prestera: Add flex arrays to some structs
The "struct prestera_msg_vtcam_rule_add_req" uses a dynamically sized
set of trailing elements. Specifically, it uses an array of structures
of type "prestera_msg_acl_action actions_msg".
The "struct prestera_msg_flood_domain_ports_set_req" also uses a
dynamically sized set of trailing elements. Specifically, it uses an
array of structures of type "prestera_msg_acl_action actions_msg".
So, use the preferred way in the kernel declaring flexible arrays [1].
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions). In this case, it is important to note
that the attribute used is specifically __counted_by_le since the
counters are of type __le32.
The logic does not need to change since the counters for the flexible
arrays are asigned before any access to the arrays.
The order in which the structure prestera_msg_vtcam_rule_add_req and the
structure prestera_msg_flood_domain_ports_set_req are defined must be
changed to avoid incomplete type errors.
Also, avoid the open-coded arithmetic in memory allocator functions [2]
using the "struct_size" macro.
Moreover, the new structure members also allow us to avoid the open-
coded arithmetic on pointers. So, take advantage of this refactoring
accordingly.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays
[1]
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[2]
Signed-off-by: Erick Archer
---
.../ethernet/marvell/prestera/prestera_hw.c | 83 +--
1 file changed, 37 insertions(+), 46 deletions(-)
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
index fc6f7d2746e8..197198ba61b1 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
@@ -419,15 +419,6 @@ struct prestera_msg_vtcam_destroy_req {
__le32 vtcam_id;
};
-struct prestera_msg_vtcam_rule_add_req {
- struct prestera_msg_cmd cmd;
- __le32 key[__PRESTERA_ACL_RULE_MATCH_TYPE_MAX];
- __le32 keymask[__PRESTERA_ACL_RULE_MATCH_TYPE_MAX];
- __le32 vtcam_id;
- __le32 prio;
- __le32 n_act;
-};
-
struct prestera_msg_vtcam_rule_del_req {
struct prestera_msg_cmd cmd;
__le32 vtcam_id;
@@ -471,6 +462,16 @@ struct prestera_msg_acl_action {
};
};
+struct prestera_msg_vtcam_rule_add_req {
+ struct prestera_msg_cmd cmd;
+ __le32 key[__PRESTERA_ACL_RULE_MATCH_TYPE_MAX];
+ __le32 keymask[__PRESTERA_ACL_RULE_MATCH_TYPE_MAX];
+ __le32 vtcam_id;
+ __le32 prio;
+ __le32 n_act;
+ struct prestera_msg_acl_action actions_msg[] __counted_by_le(n_act);
+};
+
struct prestera_msg_counter_req {
struct prestera_msg_cmd cmd;
__le32 client;
@@ -702,12 +703,6 @@ struct prestera_msg_flood_domain_destroy_req {
__le32 flood_domain_idx;
};
-struct prestera_msg_flood_domain_ports_set_req {
- struct prestera_msg_cmd cmd;
- __le32 flood_domain_idx;
- __le32 ports_num;
-};
-
struct prestera_msg_flood_domain_ports_reset_req {
struct prestera_msg_cmd cmd;
__le32 flood_domain_idx;
@@ -725,6 +720,13 @@ struct prestera_msg_flood_domain_port {
__le16 port_type;
};
+struct prestera_msg_flood_domain_ports_set_req {
+ struct prestera_msg_cmd cmd;
+ __le32 flood_domain_idx;
+ __le32 ports_num;
+ struct prestera_msg_flood_domain_port ports[]
__counted_by_le(ports_num);
+};
+
struct prestera_msg_mdb_create_req {
struct prestera_msg_cmd cmd;
__le32 flood_domain_idx;
@@ -1371,23 +1373,18 @@ int prestera_hw_vtcam_rule_add(struct prestera_switch
*sw,
struct prestera_acl_hw_action_info *act,
u8 n_act, u32 *rule_id)
{
- struct prestera_msg_acl_action *actions_msg;
struct prestera_msg_vtcam_rule_add_req *req;
struct prestera_msg_vtcam_resp resp;
- void *buff;
- u32 size;
+ size_t size;
int err;
u8 i;
- size = sizeof(*req) + sizeof(*actions_msg) * n_act;
-
- buff = kzalloc(size, GFP_KERNEL);
- if (!buff)
+ size = struct_size(req, actions_msg, n_act);
+ req = kzalloc(size, GFP_KERNEL);
+ if (!req)
return -ENOMEM;
- req = buff;
req->n_act = __cpu_to_le32(n_act);
- actions_msg = buff + sizeof(*req);
/* put acl matches into the message */
memcpy(req-&g
Re: [PATCH v2] tty: rfcomm: prefer struct_size over open coded arithmetic
Hi Kees, Jiri and Luiz,
First of all, thanks for the reviews.
On Mon, May 13, 2024 at 12:29:04PM -0400, Luiz Augusto von Dentz wrote:
> Hi Jiri, Eric,
>
> On Mon, May 13, 2024 at 1:07 AM Jiri Slaby wrote:
> >
> > On 12. 05. 24, 13:17, Erick Archer wrote:
> > > This is an effort to get rid of all multiplications from allocation
> > > functions in order to prevent integer overflows [1][2].
> > >
> > > As the "dl" variable is a pointer to "struct rfcomm_dev_list_req" and
> > > this structure ends in a flexible array:
> > ...
> > > --- a/include/net/bluetooth/rfcomm.h
> > > +++ b/include/net/bluetooth/rfcomm.h
> > ...
> > > @@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg)
> > > list_for_each_entry(dev, &rfcomm_dev_list, list) {
> > > if (!tty_port_get(&dev->port))
> > > continue;
> > > - (di + n)->id = dev->id;
> > > - (di + n)->flags = dev->flags;
> > > - (di + n)->state = dev->dlc->state;
> > > - (di + n)->channel = dev->channel;
> > > - bacpy(&(di + n)->src, &dev->src);
> > > - bacpy(&(di + n)->dst, &dev->dst);
> > > + di[n].id = dev->id;
> > > + di[n].flags = dev->flags;
> > > + di[n].state = dev->dlc->state;
> > > + di[n].channel = dev->channel;
> > > + bacpy(&di[n].src, &dev->src);
> > > + bacpy(&di[n].dst, &dev->dst);
> >
> > This does not relate much to "prefer struct_size over open coded
> > arithmetic". It should have been in a separate patch.
>
> +1, please split these changes into its own patch so we can apply it
> separately.
Ok, no problem. Also, I will simplify the "bacpy" lines with direct
assignments as Kees suggested:
di[n].src = dev->src;
di[n].dst = dev->dst;
instead of:
bacpy(&di[n].src, &dev->src);
bacpy(&di[n].dst, &dev->dst);
Regards,
Erick
> > Other than that, LGTM.
> >
> > thanks,
> > --
> > js
> > suse labs
> >
>
>
> --
> Luiz Augusto von Dentz
Re: [PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic
Hi Kees and Luiz, First of all, thanks for the reviews. On Mon, May 13, 2024 at 12:31:29PM -0400, Luiz Augusto von Dentz wrote: > Hi Eric, > > On Sun, May 12, 2024 at 11:08 PM Kees Cook wrote: > > > > On Sun, May 12, 2024 at 04:17:06PM +0200, Erick Archer wrote: > > > [...] > > > Also remove the "size" variable as it is no longer needed and refactor > > > the list_for_each_entry() loop to use dr[n] instead of (dr + n). > > Have the change above split on its own patch. Ok, no problem. I will send a new version. Regards, Erick
[PATCH v3 0/2] tty: rfcomm: refactor rfcomm_get_dev_list() function
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "dl" variable is a pointer to "struct rfcomm_dev_list_req" and
this structure ends in a flexible array:
struct rfcomm_dev_list_req {
[...]
struct rfcomm_dev_info dev_info[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() and copy_to_user() functions.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
In this case, it is important to note that the logic needs a little
refactoring to ensure that the "dev_num" member is initialized before
the first access to the flex array. Specifically, add the assignment
before the list_for_each_entry() loop.
Also remove the "size" variable as it is no longer needed and refactor
the list_for_each_entry() loop to use di[n] instead of (di + n).
This way, the code is more readable, idiomatic and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Specifically, the first patch is related to the struct_size() helper
and the second patch refactors the list_for_each_entry() loop to use
array indexing instead of pointer arithmetic.
Regards,
Erick
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
---
Changes in v3:
- Add the "Reviewed-by:" tags.
- Split the changes in two commits (Jiri Slaby, Luiz Augusto von Dentz).
Changes in v2:
- Add the __counted_by() attribute (Kees Cook).
- Refactor the list_for_each_entry() loop to use di[n] instead of
(di + n) (Kees Cook).
Previous versions:
v2 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237262c62b054fabd7229168b...@as8pr02mb7237.eurprd02.prod.outlook.com/
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb723725e0069f7ae8f64e876e8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
---
Erick Archer (2):
tty: rfcomm: prefer struct_size over open coded arithmetic
tty: rfcomm: prefer array indexing over pointer arithmetic
include/net/bluetooth/rfcomm.h | 2 +-
net/bluetooth/rfcomm/tty.c | 23 ++-
2 files changed, 11 insertions(+), 14 deletions(-)
--
2.25.1
[PATCH v3 1/2] tty: rfcomm: prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "dl" variable is a pointer to "struct rfcomm_dev_list_req" and
this structure ends in a flexible array:
struct rfcomm_dev_list_req {
[...]
struct rfcomm_dev_info dev_info[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() and copy_to_user() functions.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
In this case, it is important to note that the logic needs a little
refactoring to ensure that the "dev_num" member is initialized before
the first access to the flex array. Specifically, add the assignment
before the list_for_each_entry() loop.
Also remove the "size" variable as it is no longer needed.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
include/net/bluetooth/rfcomm.h | 2 +-
net/bluetooth/rfcomm/tty.c | 11 ---
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/include/net/bluetooth/rfcomm.h b/include/net/bluetooth/rfcomm.h
index 99d26879b02a..c05882476900 100644
--- a/include/net/bluetooth/rfcomm.h
+++ b/include/net/bluetooth/rfcomm.h
@@ -355,7 +355,7 @@ struct rfcomm_dev_info {
struct rfcomm_dev_list_req {
u16 dev_num;
- struct rfcomm_dev_info dev_info[];
+ struct rfcomm_dev_info dev_info[] __counted_by(dev_num);
};
int rfcomm_dev_ioctl(struct sock *sk, unsigned int cmd, void __user *arg);
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 69c75c041fe1..44b781e7569e 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -504,7 +504,7 @@ static int rfcomm_get_dev_list(void __user *arg)
struct rfcomm_dev *dev;
struct rfcomm_dev_list_req *dl;
struct rfcomm_dev_info *di;
- int n = 0, size, err;
+ int n = 0, err;
u16 dev_num;
BT_DBG("");
@@ -515,12 +515,11 @@ static int rfcomm_get_dev_list(void __user *arg)
if (!dev_num || dev_num > (PAGE_SIZE * 4) / sizeof(*di))
return -EINVAL;
- size = sizeof(*dl) + dev_num * sizeof(*di);
-
- dl = kzalloc(size, GFP_KERNEL);
+ dl = kzalloc(struct_size(dl, dev_info, dev_num), GFP_KERNEL);
if (!dl)
return -ENOMEM;
+ dl->dev_num = dev_num;
di = dl->dev_info;
mutex_lock(&rfcomm_dev_lock);
@@ -542,9 +541,7 @@ static int rfcomm_get_dev_list(void __user *arg)
mutex_unlock(&rfcomm_dev_lock);
dl->dev_num = n;
- size = sizeof(*dl) + n * sizeof(*di);
-
- err = copy_to_user(arg, dl, size);
+ err = copy_to_user(arg, dl, struct_size(dl, dev_info, n));
kfree(dl);
return err ? -EFAULT : 0;
--
2.25.1
[PATCH v3 2/2] tty: rfcomm: prefer array indexing over pointer arithmetic
Refactor the list_for_each_entry() loop of rfcomm_get_dev_list()
function to use array indexing instead of pointer arithmetic.
This way, the code is more readable and idiomatic.
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
net/bluetooth/rfcomm/tty.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 44b781e7569e..af80d599c337 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -527,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg)
list_for_each_entry(dev, &rfcomm_dev_list, list) {
if (!tty_port_get(&dev->port))
continue;
- (di + n)->id = dev->id;
- (di + n)->flags = dev->flags;
- (di + n)->state = dev->dlc->state;
- (di + n)->channel = dev->channel;
- bacpy(&(di + n)->src, &dev->src);
- bacpy(&(di + n)->dst, &dev->dst);
+ di[n].id = dev->id;
+ di[n].flags = dev->flags;
+ di[n].state = dev->dlc->state;
+ di[n].channel = dev->channel;
+ bacpy(&di[n].src, &dev->src);
+ bacpy(&di[n].dst, &dev->dst);
tty_port_put(&dev->port);
if (++n >= dev_num)
break;
--
2.25.1
[PATCH v2 0/2] Bluetooth: hci_core: Refactor hci_get_dev_list() function
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "dl" variable is a pointer to "struct hci_dev_list_req" and this
structure ends in a flexible array:
struct hci_dev_list_req {
[...]
struct hci_dev_req dev_req[]; /* hci_dev_req structures */
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() and copy_to_user() functions.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
In this case, it is important to note that the logic needs a little
refactoring to ensure that the "dev_num" member is initialized before
the first access to the flex array. Specifically, add the assignment
before the list_for_each_entry() loop.
Also remove the "size" variable as it is no longer needed and refactor
the list_for_each_entry() loop to use dr[n] instead of (dr + n).
This way, the code is more readable, idiomatic and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Specifically, the first patch is related to the struct_size() helper
and the second patch refactors the list_for_each_entry() loop to use
array indexing instead of pointer arithmetic.
Regards,
Erick
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
---
Changes in v2:
- Add the "Reviewed-by:" tags.
- Split the changes in two commits (Luiz Augusto von Dentz).
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237ecd397bdb7f529adc7468b...@as8pr02mb7237.eurprd02.prod.outlook.com/
---
Erick Archer (2):
Bluetooth: hci_core: Prefer struct_size over open coded arithmetic
Bluetooth: hci_core: Prefer array indexing over pointer arithmetic
include/net/bluetooth/hci_sock.h | 2 +-
net/bluetooth/hci_core.c | 15 ++-
2 files changed, 7 insertions(+), 10 deletions(-)
--
2.25.1
[PATCH v2 1/2] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "dl" variable is a pointer to "struct hci_dev_list_req" and this
structure ends in a flexible array:
struct hci_dev_list_req {
[...]
struct hci_dev_req dev_req[]; /* hci_dev_req structures */
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() and copy_to_user() functions.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
In this case, it is important to note that the logic needs a little
refactoring to ensure that the "dev_num" member is initialized before
the first access to the flex array. Specifically, add the assignment
before the list_for_each_entry() loop.
Also remove the "size" variable as it is no longer needed.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
include/net/bluetooth/hci_sock.h | 2 +-
net/bluetooth/hci_core.c | 11 ---
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/include/net/bluetooth/hci_sock.h b/include/net/bluetooth/hci_sock.h
index 9949870f7d78..13e8cd4414a1 100644
--- a/include/net/bluetooth/hci_sock.h
+++ b/include/net/bluetooth/hci_sock.h
@@ -144,7 +144,7 @@ struct hci_dev_req {
struct hci_dev_list_req {
__u16 dev_num;
- struct hci_dev_req dev_req[]; /* hci_dev_req structures */
+ struct hci_dev_req dev_req[] __counted_by(dev_num);
};
struct hci_conn_list_req {
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dd3b0f501018..81fe0958056d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -837,7 +837,7 @@ int hci_get_dev_list(void __user *arg)
struct hci_dev *hdev;
struct hci_dev_list_req *dl;
struct hci_dev_req *dr;
- int n = 0, size, err;
+ int n = 0, err;
__u16 dev_num;
if (get_user(dev_num, (__u16 __user *) arg))
@@ -846,12 +846,11 @@ int hci_get_dev_list(void __user *arg)
if (!dev_num || dev_num > (PAGE_SIZE * 2) / sizeof(*dr))
return -EINVAL;
- size = sizeof(*dl) + dev_num * sizeof(*dr);
-
- dl = kzalloc(size, GFP_KERNEL);
+ dl = kzalloc(struct_size(dl, dev_req, dev_num), GFP_KERNEL);
if (!dl)
return -ENOMEM;
+ dl->dev_num = dev_num;
dr = dl->dev_req;
read_lock(&hci_dev_list_lock);
@@ -874,9 +873,7 @@ int hci_get_dev_list(void __user *arg)
read_unlock(&hci_dev_list_lock);
dl->dev_num = n;
- size = sizeof(*dl) + n * sizeof(*dr);
-
- err = copy_to_user(arg, dl, size);
+ err = copy_to_user(arg, dl, struct_size(dl, dev_req, n));
kfree(dl);
return err ? -EFAULT : 0;
--
2.25.1
[PATCH v2 2/2] Bluetooth: hci_core: Prefer array indexing over pointer arithmetic
Refactor the list_for_each_entry() loop of hci_get_dev_list() function to use array indexing instead of pointer arithmetic. This way, the code is more readable and idiomatic. Reviewed-by: Kees Cook Signed-off-by: Erick Archer --- net/bluetooth/hci_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 81fe0958056d..b3ee9ff17624 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -864,8 +864,8 @@ int hci_get_dev_list(void __user *arg) if (hci_dev_test_flag(hdev, HCI_AUTO_OFF)) flags &= ~BIT(HCI_UP); - (dr + n)->dev_id = hdev->id; - (dr + n)->dev_opt = flags; + dr[n].dev_id = hdev->id; + dr[n].dev_opt = flags; if (++n >= dev_num) break; -- 2.25.1
[PATCH] Bluetooth: Use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not
change the former (unlike the latter). This patch has no effect
on runtime behavior.
Signed-off-by: Erick Archer
---
drivers/bluetooth/btrtl.c | 2 +-
drivers/bluetooth/hci_ldisc.c | 2 +-
drivers/bluetooth/hci_qca.c | 5 ++---
drivers/bluetooth/hci_vhci.c | 2 +-
4 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 4f1e37b4f780..f2f37143c454 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -811,7 +811,7 @@ static int rtl_download_firmware(struct hci_dev *hdev,
struct sk_buff *skb;
struct hci_rp_read_local_version *rp;
- dl_cmd = kmalloc(sizeof(struct rtl_download_cmd), GFP_KERNEL);
+ dl_cmd = kmalloc(sizeof(*dl_cmd), GFP_KERNEL);
if (!dl_cmd)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 17a2f158a0df..30192bb08354 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -488,7 +488,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
if (tty->ops->write == NULL)
return -EOPNOTSUPP;
- hu = kzalloc(sizeof(struct hci_uart), GFP_KERNEL);
+ hu = kzalloc(sizeof(*hu), GFP_KERNEL);
if (!hu) {
BT_ERR("Can't allocate control structure");
return -ENFILE;
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 0c9c9ee56592..384a2bbbf303 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -569,7 +569,7 @@ static int qca_open(struct hci_uart *hu)
if (!hci_uart_has_flow_control(hu))
return -EOPNOTSUPP;
- qca = kzalloc(sizeof(struct qca_data), GFP_KERNEL);
+ qca = kzalloc(sizeof(*qca), GFP_KERNEL);
if (!qca)
return -ENOMEM;
@@ -1040,8 +1040,7 @@ static void qca_controller_memdump(struct work_struct
*work)
}
if (!qca_memdump) {
- qca_memdump = kzalloc(sizeof(struct qca_memdump_info),
- GFP_ATOMIC);
+ qca_memdump = kzalloc(sizeof(*qca_memdump), GFP_ATOMIC);
if (!qca_memdump) {
mutex_unlock(&qca->hci_memdump_lock);
return;
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 28750a40f0ed..c4046f8f1985 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -633,7 +633,7 @@ static int vhci_open(struct inode *inode, struct file *file)
{
struct vhci_data *data;
- data = kzalloc(sizeof(struct vhci_data), GFP_KERNEL);
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
--
2.25.1
[PATCH] Input: keyboard - use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not
change the former (unlike the latter). This patch has no effect
on runtime behavior.
Signed-off-by: Erick Archer
---
drivers/input/keyboard/atkbd.c | 2 +-
drivers/input/keyboard/lkkbd.c | 2 +-
drivers/input/keyboard/locomokbd.c | 2 +-
drivers/input/keyboard/maple_keyb.c | 2 +-
drivers/input/keyboard/newtonkbd.c | 2 +-
drivers/input/keyboard/stowaway.c | 2 +-
drivers/input/keyboard/sunkbd.c | 2 +-
drivers/input/keyboard/xtkbd.c | 2 +-
8 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 7f67f9f2946b..f4f2078cf501 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -1279,7 +1279,7 @@ static int atkbd_connect(struct serio *serio, struct
serio_driver *drv)
struct input_dev *dev;
int err = -ENOMEM;
- atkbd = kzalloc(sizeof(struct atkbd), GFP_KERNEL);
+ atkbd = kzalloc(sizeof(*atkbd), GFP_KERNEL);
dev = input_allocate_device();
if (!atkbd || !dev)
goto fail1;
diff --git a/drivers/input/keyboard/lkkbd.c b/drivers/input/keyboard/lkkbd.c
index 047b654b3752..c035216dd27c 100644
--- a/drivers/input/keyboard/lkkbd.c
+++ b/drivers/input/keyboard/lkkbd.c
@@ -608,7 +608,7 @@ static int lkkbd_connect(struct serio *serio, struct
serio_driver *drv)
int i;
int err;
- lk = kzalloc(sizeof(struct lkkbd), GFP_KERNEL);
+ lk = kzalloc(sizeof(*lk), GFP_KERNEL);
input_dev = input_allocate_device();
if (!lk || !input_dev) {
err = -ENOMEM;
diff --git a/drivers/input/keyboard/locomokbd.c
b/drivers/input/keyboard/locomokbd.c
index f866c03b9d0e..4b0f8323c492 100644
--- a/drivers/input/keyboard/locomokbd.c
+++ b/drivers/input/keyboard/locomokbd.c
@@ -227,7 +227,7 @@ static int locomokbd_probe(struct locomo_dev *dev)
struct input_dev *input_dev;
int i, err;
- locomokbd = kzalloc(sizeof(struct locomokbd), GFP_KERNEL);
+ locomokbd = kzalloc(sizeof(*locomokbd), GFP_KERNEL);
input_dev = input_allocate_device();
if (!locomokbd || !input_dev) {
err = -ENOMEM;
diff --git a/drivers/input/keyboard/maple_keyb.c
b/drivers/input/keyboard/maple_keyb.c
index d08b565be24c..91a1d2958109 100644
--- a/drivers/input/keyboard/maple_keyb.c
+++ b/drivers/input/keyboard/maple_keyb.c
@@ -154,7 +154,7 @@ static int probe_maple_kbd(struct device *dev)
mdev = to_maple_dev(dev);
mdrv = to_maple_driver(dev->driver);
- kbd = kzalloc(sizeof(struct dc_kbd), GFP_KERNEL);
+ kbd = kzalloc(sizeof(*kbd), GFP_KERNEL);
if (!kbd) {
error = -ENOMEM;
goto fail;
diff --git a/drivers/input/keyboard/newtonkbd.c
b/drivers/input/keyboard/newtonkbd.c
index df00a119aa9a..71e0a3f830dd 100644
--- a/drivers/input/keyboard/newtonkbd.c
+++ b/drivers/input/keyboard/newtonkbd.c
@@ -68,7 +68,7 @@ static int nkbd_connect(struct serio *serio, struct
serio_driver *drv)
int err = -ENOMEM;
int i;
- nkbd = kzalloc(sizeof(struct nkbd), GFP_KERNEL);
+ nkbd = kzalloc(sizeof(*nkbd), GFP_KERNEL);
input_dev = input_allocate_device();
if (!nkbd || !input_dev)
goto fail1;
diff --git a/drivers/input/keyboard/stowaway.c
b/drivers/input/keyboard/stowaway.c
index 56e784936059..7ef0b3f4f549 100644
--- a/drivers/input/keyboard/stowaway.c
+++ b/drivers/input/keyboard/stowaway.c
@@ -72,7 +72,7 @@ static int skbd_connect(struct serio *serio, struct
serio_driver *drv)
int err = -ENOMEM;
int i;
- skbd = kzalloc(sizeof(struct skbd), GFP_KERNEL);
+ skbd = kzalloc(sizeof(*skbd), GFP_KERNEL);
input_dev = input_allocate_device();
if (!skbd || !input_dev)
goto fail1;
diff --git a/drivers/input/keyboard/sunkbd.c b/drivers/input/keyboard/sunkbd.c
index b123a208ef36..72fb46029710 100644
--- a/drivers/input/keyboard/sunkbd.c
+++ b/drivers/input/keyboard/sunkbd.c
@@ -263,7 +263,7 @@ static int sunkbd_connect(struct serio *serio, struct
serio_driver *drv)
int err = -ENOMEM;
int i;
- sunkbd = kzalloc(sizeof(struct sunkbd), GFP_KERNEL);
+ sunkbd = kzalloc(sizeof(*sunkbd), GFP_KERNEL);
input_dev = input_allocate_device();
if (!sunkbd || !input_dev)
goto fail1;
diff --git a/drivers/input/keyboard/xtkbd.c b/drivers/input/keyboard/xtkbd.c
index c9d7c2481726..befa713268ae 100644
--- a/drivers/input/keyboard/xtkbd.c
+++ b/drivers/input/keyboard/xtkbd.c
@@ -70,7 +70,7 @@ static int xtkbd_connect(struct serio *serio, struct
serio_driver *drv)
int err = -ENOMEM;
int i;
- xtkbd = kmalloc(sizeof(struct xtkbd), GFP_KERNEL);
+ xtkbd = kmalloc(sizeof(*xtkbd), GFP_KERNEL);
input_
[RFC] HID: ishtp-hid-client: replace fake-flex arrays with flex-array members
One-element arrays as fake flex arrays are deprecated [1] and we are
moving towards adopting C99 flexible-array members, instead. This case
also has more complexity because it is a flexible array of flexible
arrays and this patch needs to be ready to enable the new compiler flag
-Wflex-array-member-not-at-end (coming in GCC-14) globally.
So, define a new struct type for the single reports:
struct report {
uint16_t size;
struct hostif_msg_hdr msg;
} __packed;
but without the payload (flex array) in it. And add this payload to the
"hostif_msg" structure. This way, in the "report_list" structure we can
declare a flex array of single reports which now do not contain another
flex array.
struct report_list {
[...]
struct report reports[];
} __packed;
Also, use "container_of()" whenever we need to retrieve a pointer to
the flexible structure, through which we can access the flexible array
if needed.
Moreover, refactor the code accordingly to use the new structures and
take advantage of this avoiding some pointer arithmetic and using the
"struct_size" helper when possible.
This way, the code is more readable and safer.
Link:
https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays
[1]
Closes: https://github.com/KSPP/linux/issues/333
Signed-off-by: Erick Archer
---
Hi,
The idea behind this patch is extracted from the ones sent by Gustavo
A. R. Silva [1] but without the use of "struct_group_tagged()" helper
to separate the flexible array from the rest of the members in the
flexible structures.
Regarding adding the "__counted_by" attribute to the flexible arrays,
I can say that I have not dared. The reasons are:
1.- In both arrays there are a no direct assignment to the counter
member. Only exists a cast from a raw stream of bytes to a pointer
of a structure and this way the counter member has the value.
2.- The outer flexible array (in the struct report_list) has elements
of different size. I believe that every report can have a different
size, so I think the "__counted_by" will not work as expected.
Comments are welcome ;)
Regards,
Erick
[1] Here are some patches that use the same idea:
Link:
https://lore.kernel.org/linux-hardening/cover.1709658886.git.gustavo...@kernel.org/
Link: https://lore.kernel.org/linux-hardening/ZgYWlkxdrrieDYIu@neat/
Link: https://lore.kernel.org/linux-hardening/ZgG8bbEzhmX5nGRE@neat/
---
drivers/hid/intel-ish-hid/ishtp-hid-client.c | 27 ++--
drivers/hid/intel-ish-hid/ishtp-hid.h| 11 +---
2 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index fbd4f8ea1951..c0c8f4d7b0e3 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -70,10 +70,10 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl,
void *recv_buf,
unsigned char *payload;
struct device_info *dev_info;
int i, j;
- size_t payload_len, total_len, cur_pos, raw_len;
+ size_t payload_len, total_len, cur_pos, raw_len, msg_len;
int report_type;
struct report_list *reports_list;
- char *reports;
+ struct report *report;
size_t report_len;
struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
int curr_hid_dev = client_data->cur_hid_dev;
@@ -99,7 +99,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void
*recv_buf,
payload_len = recv_msg->hdr.size;
/* Sanity checks */
- if (cur_pos + payload_len + sizeof(struct hostif_msg) >
+ if (cur_pos + struct_size(recv_msg, payload, payload_len) >
total_len) {
++client_data->bad_recv_cnt;
report_bad_packet(hid_ishtp_cl, recv_msg, cur_pos,
@@ -280,14 +280,13 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl,
void *recv_buf,
case HOSTIF_PUBLISH_INPUT_REPORT_LIST:
report_type = HID_INPUT_REPORT;
reports_list = (struct report_list *)payload;
- reports = (char *)reports_list->reports;
+ report = reports_list->reports;
for (j = 0; j < reports_list->num_of_reports; j++) {
- recv_msg = (struct hostif_msg *)(reports +
- sizeof(uint16_t));
- report_len = *(uint16_t *)reports;
- payload = reports + sizeof(uint16_t) +
- sizeof(struct hostif_msg_hdr);
+ recv_msg = container_of(&report->msg,
+
[PATCH] wifi: brcm80211: use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not
change the former (unlike the latter). This patch has no effect
on runtime behavior.
Signed-off-by: Erick Archer
---
.../broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 ++--
.../broadcom/brcm80211/brcmfmac/btcoex.c | 2 +-
.../broadcom/brcm80211/brcmfmac/sdio.c| 2 +-
.../broadcom/brcm80211/brcmfmac/usb.c | 2 +-
.../broadcom/brcm80211/brcmsmac/aiutils.c | 2 +-
.../broadcom/brcm80211/brcmsmac/ampdu.c | 2 +-
.../broadcom/brcm80211/brcmsmac/antsel.c | 2 +-
.../broadcom/brcm80211/brcmsmac/channel.c | 2 +-
.../broadcom/brcm80211/brcmsmac/dma.c | 2 +-
.../broadcom/brcm80211/brcmsmac/mac80211_if.c | 2 +-
.../broadcom/brcm80211/brcmsmac/main.c| 23 +--
.../broadcom/brcm80211/brcmsmac/phy/phy_cmn.c | 4 ++--
.../broadcom/brcm80211/brcmsmac/phy/phy_lcn.c | 2 +-
.../broadcom/brcm80211/brcmsmac/phy_shim.c| 2 +-
14 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 13391c2d82aa..d35262335eaf 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1061,10 +1061,10 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
if (func->num != 2)
return -ENODEV;
- bus_if = kzalloc(sizeof(struct brcmf_bus), GFP_KERNEL);
+ bus_if = kzalloc(sizeof(*bus_if), GFP_KERNEL);
if (!bus_if)
return -ENOMEM;
- sdiodev = kzalloc(sizeof(struct brcmf_sdio_dev), GFP_KERNEL);
+ sdiodev = kzalloc(sizeof(*sdiodev), GFP_KERNEL);
if (!sdiodev) {
kfree(bus_if);
return -ENOMEM;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
index 7ea2631b8069..c199595a6a01 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
@@ -361,7 +361,7 @@ int brcmf_btcoex_attach(struct brcmf_cfg80211_info *cfg)
struct brcmf_btcoex_info *btci = NULL;
brcmf_dbg(TRACE, "enter\n");
- btci = kmalloc(sizeof(struct brcmf_btcoex_info), GFP_KERNEL);
+ btci = kmalloc(sizeof(*btci), GFP_KERNEL);
if (!btci)
return -ENOMEM;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 6b38d9de71af..1461dc453ac2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4450,7 +4450,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev
*sdiodev)
brcmf_dbg(TRACE, "Enter\n");
/* Allocate private bus interface state */
- bus = kzalloc(sizeof(struct brcmf_sdio), GFP_ATOMIC);
+ bus = kzalloc(sizeof(*bus), GFP_ATOMIC);
if (!bus)
goto fail;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 9a105e6debe1..40b818e63ce4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1247,7 +1247,7 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info
*devinfo,
if (!bus_pub)
return -ENODEV;
- bus = kzalloc(sizeof(struct brcmf_bus), GFP_ATOMIC);
+ bus = kzalloc(sizeof(*bus), GFP_ATOMIC);
if (!bus) {
ret = -ENOMEM;
goto fail;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/aiutils.c
b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/aiutils.c
index 2084b506a450..50d817485cf9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/aiutils.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/aiutils.c
@@ -512,7 +512,7 @@ ai_attach(struct bcma_bus *pbus)
struct si_info *sii;
/* alloc struct si_info */
- sii = kzalloc(sizeof(struct si_info), GFP_ATOMIC);
+ sii = kzalloc(sizeof(*sii), GFP_ATOMIC);
if (sii == NULL)
return NULL;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/ampdu.c
b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/ampdu.c
index c3376f887114..33d17b779201 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/ampdu.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/ampdu.c
@@ -219,7 +219,7 @@ struct ampdu_info *brcms_c_ampdu_attach(struct brcms_c_info
*wlc)
struct ampdu_info *ampdu;
int i;
- ampdu = kzalloc(sizeof(struct ampdu_info), GFP_ATOMIC);
+ ampdu = kzalloc(sizeof(*ampdu), GFP_ATOMIC);
if (!ampdu)
[PATCH v2] wifi: brcm80211: use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not
change the former (unlike the latter). This patch has no effect
on runtime behavior.
At the same time remove some redundant NULL initializations.
Acked-by: Arend van Spriel
Signed-off-by: Erick Archer
---
Changes in v2:
- Remove some redundant NULL initializations (Arend van Spriel).
- Refactor a bit the logic of "phy_lcn.c" (Arend van Spriel).
- Add the "Acked-by:" tag.
Previous versions:
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237c3696881f79eaee8d02e8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
---
.../broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 ++--
.../broadcom/brcm80211/brcmfmac/btcoex.c | 4 ++--
.../broadcom/brcm80211/brcmfmac/sdio.c| 2 +-
.../broadcom/brcm80211/brcmfmac/usb.c | 6 ++---
.../broadcom/brcm80211/brcmsmac/aiutils.c | 2 +-
.../broadcom/brcm80211/brcmsmac/ampdu.c | 2 +-
.../broadcom/brcm80211/brcmsmac/antsel.c | 2 +-
.../broadcom/brcm80211/brcmsmac/channel.c | 2 +-
.../broadcom/brcm80211/brcmsmac/dma.c | 2 +-
.../broadcom/brcm80211/brcmsmac/mac80211_if.c | 2 +-
.../broadcom/brcm80211/brcmsmac/main.c| 23 +--
.../broadcom/brcm80211/brcmsmac/phy/phy_cmn.c | 4 ++--
.../broadcom/brcm80211/brcmsmac/phy/phy_lcn.c | 6 ++---
.../broadcom/brcm80211/brcmsmac/phy_shim.c| 4 ++--
14 files changed, 32 insertions(+), 33 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 13391c2d82aa..d35262335eaf 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1061,10 +1061,10 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
if (func->num != 2)
return -ENODEV;
- bus_if = kzalloc(sizeof(struct brcmf_bus), GFP_KERNEL);
+ bus_if = kzalloc(sizeof(*bus_if), GFP_KERNEL);
if (!bus_if)
return -ENOMEM;
- sdiodev = kzalloc(sizeof(struct brcmf_sdio_dev), GFP_KERNEL);
+ sdiodev = kzalloc(sizeof(*sdiodev), GFP_KERNEL);
if (!sdiodev) {
kfree(bus_if);
return -ENOMEM;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
index 7ea2631b8069..0c3d119d1219 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
@@ -358,10 +358,10 @@ static void brcmf_btcoex_handler(struct work_struct *work)
*/
int brcmf_btcoex_attach(struct brcmf_cfg80211_info *cfg)
{
- struct brcmf_btcoex_info *btci = NULL;
+ struct brcmf_btcoex_info *btci;
brcmf_dbg(TRACE, "enter\n");
- btci = kmalloc(sizeof(struct brcmf_btcoex_info), GFP_KERNEL);
+ btci = kmalloc(sizeof(*btci), GFP_KERNEL);
if (!btci)
return -ENOMEM;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 6b38d9de71af..1461dc453ac2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4450,7 +4450,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev
*sdiodev)
brcmf_dbg(TRACE, "Enter\n");
/* Allocate private bus interface state */
- bus = kzalloc(sizeof(struct brcmf_sdio), GFP_ATOMIC);
+ bus = kzalloc(sizeof(*bus), GFP_ATOMIC);
if (!bus)
goto fail;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 9a105e6debe1..8afbf529c745 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1236,8 +1236,8 @@ brcmf_usb_prepare_fw_request(struct brcmf_usbdev_info
*devinfo)
static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo,
enum brcmf_fwvendor fwvid)
{
- struct brcmf_bus *bus = NULL;
- struct brcmf_usbdev *bus_pub = NULL;
+ struct brcmf_bus *bus;
+ struct brcmf_usbdev *bus_pub;
struct device *dev = devinfo->dev;
struct brcmf_fw_request *fwreq;
int ret;
@@ -1247,7 +1247,7 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info
*devinfo,
if (!bus_pub)
return -ENODEV;
- bus = kzalloc(sizeof(struct brcmf_bus), GFP_ATOMIC);
+ bus = kzalloc(sizeof(*bus), GFP_ATOMIC);
if (!bus) {
ret = -ENOMEM;
goto fail;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/aiutils.c
b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/aiutils.c
index 20
Re: [PATCH] sctp: annotate struct sctp_assoc_ids with __counted_by()
Hi,
On Wed, May 01, 2024 at 07:01:22PM +0200, Erick Archer wrote:
> Prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time via
> CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> (for strcpy/memcpy-family functions).
>
> Suggested-by: Kees Cook
> Signed-off-by: Erick Archer
> ---
> include/uapi/linux/sctp.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index b7d91d4cf0db..836173e73401 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -1007,7 +1007,7 @@ enum sctp_sstat_state {
> */
> struct sctp_assoc_ids {
> __u32 gaids_number_of_ids;
> - sctp_assoc_tgaids_assoc_id[];
> + sctp_assoc_tgaids_assoc_id[] __counted_by(gaids_number_of_ids);
> };
>
> /*
Friendly ping: who can take this, please?
Regards,
Erick
[PATCH v4 0/3] Hardening perf subsystem
Hi everyone,
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
In the first patch, the "struct amd_uncore_ctx" can be refactored to
use a flex array for the "events" member. This way, the allocation/
freeing of the memory can be simplified. Then, the struct_size()
helper can be used to do the arithmetic calculation for the memory
to be allocated.
In the second patch, as the "struct intel_uncore_box" ends in a
flexible array, the preferred way in the kernel is to use the
struct_size() helper to do the arithmetic instead of the calculation
"size + count * size" in the kzalloc_node() function.
In the third patch, as the "struct perf_buffer" also ends in a
flexible array, the preferred way in the kernel is to use the
struct_size() helper to do the arithmetic instead of the calculation
"size + count * size" in the kzalloc_node() functions. At the same
time, prepare for the coming implementation by GCC and Clang of the
__counted_by attribute.
This time, I have decided to send these three patches in the same serie
since all of them has been rejected by the maintainers. I have used
the v4 tag since it is the latest iteration in one of the patches.
The reason these patches were rejected is that Peter Zijlstra detest
the struct_size() helper [3][4]. However, Kees Cook and I consider that
the use of this helper improves readability. But we can all say that it
is a matter of preference.
Anyway, leaving aside personal preferences, these patches has the
following pros:
- Code robustness improvement (__counted_by coverage).
- Code robustness improvement (use of struct_size() to do calculations
on memory allocator functions).
- Fewer lines of code.
- Follow the recommendations made in "Deprecated Interfaces, Language
Features, Attributes, and Conventions" [5] as the preferred method
of doing things in the kernel.
- Widely used in the kernel.
- Widely accepted in the kernel.
There are also patches in this subsystem that use the struct_size()
helper:
6566f907bf31 ("Convert intel uncore to struct_size") by Matthew Wilcox
dfbc411e0a5e ("perf/x86/rapl: Prefer struct_size() over open coded arithmetic")
by me
Therefore, I would like these patches to be applied this time.
Best regards,
Erick
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Link:
https://lore.kernel.org/linux-hardening/20240430091833.gd40...@noisy.programming.kicks-ass.net
[3]
Link:
https://lore.kernel.org/linux-hardening/20240430091504.gc40...@noisy.programming.kicks-ass.net
[4]
Link: https://docs.kernel.org/process/deprecated.html [5]
---
Changes in v4:
- Add the "Reviewed-by:" tag to the three patches.
- Rebase against linux next (tag next-20240531).
Previous versions:
perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237e4848b44a5226bd3551c8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237f4d39bf6aa0ff40e91638b...@as8pr02mb7237.eurprd02.prod.outlook.com/
perf/ring_buffer: Prefer struct_size over open coded arithmetic
v3 ->
https://lore.kernel.org/linux-hardening/as8pr02mb72379b4807f3951a1b926ba58b...@as8pr02mb7237.eurprd02.prod.outlook.com/
v2 ->
https://lore.kernel.org/linux-hardening/as8pr02mb7237569e4fbe0b26f62fdfdb8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
v1 ->
https://lore.kernel.org/linux-hardening/as8pr02mb72372ab065ea8340d960ccc48b...@as8pr02mb7237.eurprd02.prod.outlook.com/
Changes in v3:
- Refactor the logic, compared to the previous version, of the second
rb_alloc() function to gain __counted_by() coverage (Kees Cook and
Christophe JAILLET).
Changes in v2:
- Annotate "struct perf_buffer" with __counted_by() attribute (Kees Cook).
- Refactor the logic to gain __counted_by() coverage (Kees Cook).
---
Erick Archer (3):
perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
perf/ring_buffer: Prefer struct_size over open coded arithmetic
arch/x86/events/amd/uncore.c | 18 +-
arch/x86/events/intel/uncore.c | 7 +++
kernel/events/internal.h | 2 +-
kernel/events/ring_buffer.c| 15 ---
4 files changed, 13 insertions(+), 29 deletions(-)
--
2.25.1
[PATCH v4 1/3] perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
The "struct amd_uncore_ctx" can be refactored to use a flex array for
the "events" member. This way, the allocation/freeing of the memory can
be simplified.
Specifically, as the "curr" variable is a pointer to the amd_uncore_ctx
structure and it now ends up in a flexible array:
struct amd_uncore_ctx {
[...]
struct perf_event *events[];
};
the two-step allocation can be simplifief by using just one kzalloc_node
function and the struct_size() helper to do the arithmetic calculation
for the memory to be allocated.
This way, the code is more readable and safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Suggested-by: Christophe JAILLET
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
arch/x86/events/amd/uncore.c | 18 +-
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 0fafe233bba4..9d43048bfdab 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -37,8 +37,8 @@ static int pmu_version;
struct amd_uncore_ctx {
int refcnt;
int cpu;
- struct perf_event **events;
struct hlist_node node;
+ struct perf_event *events[];
};
struct amd_uncore_pmu {
@@ -430,10 +430,8 @@ static void amd_uncore_ctx_free(struct amd_uncore *uncore,
unsigned int cpu)
if (cpu == ctx->cpu)
cpumask_clear_cpu(cpu, &pmu->active_mask);
- if (!--ctx->refcnt) {
- kfree(ctx->events);
+ if (!--ctx->refcnt)
kfree(ctx);
- }
*per_cpu_ptr(pmu->ctx, cpu) = NULL;
}
@@ -478,19 +476,13 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore,
unsigned int cpu)
/* Allocate context if sibling does not exist */
if (!curr) {
node = cpu_to_node(cpu);
- curr = kzalloc_node(sizeof(*curr), GFP_KERNEL, node);
+ curr = kzalloc_node(struct_size(curr, events,
+ pmu->num_counters),
+ GFP_KERNEL, node);
if (!curr)
goto fail;
curr->cpu = cpu;
- curr->events = kzalloc_node(sizeof(*curr->events) *
- pmu->num_counters,
- GFP_KERNEL, node);
- if (!curr->events) {
- kfree(curr);
- goto fail;
- }
-
cpumask_set_cpu(cpu, &pmu->active_mask);
}
--
2.25.1
[PATCH v4 2/3] perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "box" variable is a pointer to "struct intel_uncore_box" and
this structure ends in a flexible array:
struct intel_uncore_box {
[...]
struct intel_uncore_extra_reg shared_regs[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc_node() function.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
arch/x86/events/intel/uncore.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 419c517b8594..b0d518d752aa 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -350,12 +350,11 @@ static void uncore_pmu_init_hrtimer(struct
intel_uncore_box *box)
static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type
*type,
int node)
{
- int i, size, numshared = type->num_shared_regs ;
+ int i, numshared = type->num_shared_regs;
struct intel_uncore_box *box;
- size = sizeof(*box) + numshared * sizeof(struct intel_uncore_extra_reg);
-
- box = kzalloc_node(size, GFP_KERNEL, node);
+ box = kzalloc_node(struct_size(box, shared_regs, numshared), GFP_KERNEL,
+ node);
if (!box)
return NULL;
--
2.25.1
[PATCH v4 3/3] perf/ring_buffer: Prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "rb" variable is a pointer to "struct perf_buffer" and this
structure ends in a flexible array:
struct perf_buffer {
[...]
void*data_pages[];
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc_node() functions.
In the "rb_alloc" function defined in the else branch of the macro
#ifndef CONFIG_PERF_USE_VMALLOC
the count in the struct_size helper is the literal "1" since only one
pointer to void is allocated. Also, remove the "size" variable as it
is no longer needed.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions). In this case, it is important to note
that the logic needs a little refactoring to ensure that the "nr_pages"
member is initialized before the first access to the flex array.
In one case, it is only necessary to move the "nr_pages" assignment
before the array-writing loop while in the other case the access to the
flex array needs to be moved inside the if branch and after the
"nr_pages" assignment.
This way, the code is more safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Reviewed-by: Kees Cook
Signed-off-by: Erick Archer
---
kernel/events/internal.h| 2 +-
kernel/events/ring_buffer.c | 15 ---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 5150d5f84c03..dc8d39b01adb 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -55,7 +55,7 @@ struct perf_buffer {
void*aux_priv;
struct perf_event_mmap_page *user_page;
- void*data_pages[];
+ void*data_pages[] __counted_by(nr_pages);
};
extern void rb_free(struct perf_buffer *rb);
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 4013408ce012..d123fa2096cf 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
unsigned long size;
int i, node;
- size = sizeof(struct perf_buffer);
- size += nr_pages * sizeof(void *);
-
+ size = struct_size(rb, data_pages, nr_pages);
if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER)
goto fail;
@@ -833,6 +831,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
if (!rb)
goto fail;
+ rb->nr_pages = nr_pages;
rb->user_page = perf_mmap_alloc_page(cpu);
if (!rb->user_page)
goto fail_user_page;
@@ -843,8 +842,6 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
goto fail_data_pages;
}
- rb->nr_pages = nr_pages;
-
ring_buffer_init(rb, watermark, flags);
return rb;
@@ -916,15 +913,11 @@ void rb_free(struct perf_buffer *rb)
struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
{
struct perf_buffer *rb;
- unsigned long size;
void *all_buf;
int node;
- size = sizeof(struct perf_buffer);
- size += sizeof(void *);
-
node = (cpu == -1) ? cpu : cpu_to_node(cpu);
- rb = kzalloc_node(size, GFP_KERNEL, node);
+ rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
if (!rb)
goto fail;
@@ -935,9 +928,9 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark,
int cpu, int flags)
goto fail_all_buf;
rb->user_page = all_buf;
- rb->data_pages[0] = all_buf + PAGE_SIZE;
if (nr_pages) {
rb->nr_pages = 1;
+ rb->data_pages[0] = all_buf + PAGE_SIZE;
rb->page_order = ilog2(nr_pages);
}
--
2.25.1
[PATCH] nvdimm/btt: use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type) due to the type of the variable can change and one needs not change the former (unlike the latter). This patch has no effect on runtime behavior. Signed-off-by: Erick Archer --- drivers/nvdimm/btt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 1e5aedaf8c7b..b25df8fa8e8e 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -751,7 +751,7 @@ static struct arena_info *alloc_arena(struct btt *btt, size_t size, u64 logsize, mapsize, datasize; u64 available = size; - arena = kzalloc(sizeof(struct arena_info), GFP_KERNEL); + arena = kzalloc(sizeof(*arena), GFP_KERNEL); if (!arena) return NULL; arena->nd_btt = btt->nd_btt; @@ -978,7 +978,7 @@ static int btt_arena_write_layout(struct arena_info *arena) if (ret) return ret; - super = kzalloc(sizeof(struct btt_sb), GFP_NOIO); + super = kzalloc(sizeof(*super), GFP_NOIO); if (!super) return -ENOMEM; -- 2.25.1
[PATCH] soc: fsl: qe: use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not
change the former (unlike the latter). This patch has no effect
on runtime behavior.
Signed-off-by: Erick Archer
---
drivers/soc/fsl/qe/ucc_fast.c | 2 +-
drivers/soc/fsl/qe/ucc_slow.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/fsl/qe/ucc_fast.c b/drivers/soc/fsl/qe/ucc_fast.c
index 53d8aafc9317..32044a40c278 100644
--- a/drivers/soc/fsl/qe/ucc_fast.c
+++ b/drivers/soc/fsl/qe/ucc_fast.c
@@ -191,7 +191,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct
ucc_fast_private ** ucc
return -EINVAL;
}
- uccf = kzalloc(sizeof(struct ucc_fast_private), GFP_KERNEL);
+ uccf = kzalloc(sizeof(*uccf), GFP_KERNEL);
if (!uccf) {
printk(KERN_ERR "%s: Cannot allocate private data\n",
__func__);
diff --git a/drivers/soc/fsl/qe/ucc_slow.c b/drivers/soc/fsl/qe/ucc_slow.c
index d5ac1ac0ed3c..c80bb5014725 100644
--- a/drivers/soc/fsl/qe/ucc_slow.c
+++ b/drivers/soc/fsl/qe/ucc_slow.c
@@ -148,7 +148,7 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct
ucc_slow_private ** ucc
return -EINVAL;
}
- uccs = kzalloc(sizeof(struct ucc_slow_private), GFP_KERNEL);
+ uccs = kzalloc(sizeof(*uccs), GFP_KERNEL);
if (!uccs) {
printk(KERN_ERR "%s: Cannot allocate private data\n",
__func__);
--
2.25.1
[PATCH] auxdisplay: Use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type) due to the type of the variable can change and one needs not change the former (unlike the latter). This patch has no effect on runtime behavior. Signed-off-by: Erick Archer --- drivers/auxdisplay/arm-charlcd.c | 2 +- drivers/auxdisplay/hd44780.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/auxdisplay/arm-charlcd.c b/drivers/auxdisplay/arm-charlcd.c index 0b1c99cca733..a7eae99a48f7 100644 --- a/drivers/auxdisplay/arm-charlcd.c +++ b/drivers/auxdisplay/arm-charlcd.c @@ -270,7 +270,7 @@ static int __init charlcd_probe(struct platform_device *pdev) struct charlcd *lcd; struct resource *res; - lcd = kzalloc(sizeof(struct charlcd), GFP_KERNEL); + lcd = kzalloc(sizeof(*lcd), GFP_KERNEL); if (!lcd) return -ENOMEM; diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c index 7ac0b1b1d548..025dc6855cb2 100644 --- a/drivers/auxdisplay/hd44780.c +++ b/drivers/auxdisplay/hd44780.c @@ -230,7 +230,7 @@ static int hd44780_probe(struct platform_device *pdev) if (!lcd) goto fail1; - hd = kzalloc(sizeof(struct hd44780), GFP_KERNEL); + hd = kzalloc(sizeof(*hd), GFP_KERNEL); if (!hd) goto fail2; -- 2.25.1
[PATCH 0/2] atmel: at76c50x: improve code robustness
Hi, This series of patches attempts to improve the at76c50x code robustness. In the first patch, it is preferred to use sizeof(*pointer) instead of sizeof(type) due to the type of the variable can change and one needs not change the former (unlike the latter). The second patch is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1][2]. As the "struct at76_command" ends in a flexible array the preferred way in the kernel is to use the struct_size() helper to do the arithmetic instead of the calculation "size + count" in the kmalloc() function. At the same time, prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). This way, the code is more readable and safer. Regards, Erick Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/160 [2] --- Erick Archer (2): atmel: at76c50x: use sizeof(*pointer) instead of sizeof(type) atmel: at76c50x: prefer struct_size over open coded arithmetic drivers/net/wireless/atmel/at76c50x-usb.c | 56 ++- drivers/net/wireless/atmel/at76c50x-usb.h | 2 +- 2 files changed, 26 insertions(+), 32 deletions(-) -- 2.25.1
[PATCH 1/2] atmel: at76c50x: use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not
change the former (unlike the latter). This patch has no effect
on runtime behavior.
At the same time remove some redundant NULL initializations.
Signed-off-by: Erick Archer
---
drivers/net/wireless/atmel/at76c50x-usb.c | 44 ++-
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/net/wireless/atmel/at76c50x-usb.c
b/drivers/net/wireless/atmel/at76c50x-usb.c
index 0b55a272bfd6..0ca2f629683d 100644
--- a/drivers/net/wireless/atmel/at76c50x-usb.c
+++ b/drivers/net/wireless/atmel/at76c50x-usb.c
@@ -332,7 +332,7 @@ static int at76_dfu_get_status(struct usb_device *udev,
ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), DFU_GETSTATUS,
USB_TYPE_CLASS | USB_DIR_IN | USB_RECIP_INTERFACE,
- 0, 0, status, sizeof(struct dfu_status),
+ 0, 0, status, sizeof(*status),
USB_CTRL_GET_TIMEOUT);
return ret;
}
@@ -366,7 +366,7 @@ static int at76_usbdfu_download(struct usb_device *udev, u8
*buf, u32 size,
u32 dfu_timeout = 0;
int bsize = 0;
int blockno = 0;
- struct dfu_status *dfu_stat_buf = NULL;
+ struct dfu_status *dfu_stat_buf;
u8 *dfu_state = NULL;
u8 *block = NULL;
@@ -378,7 +378,7 @@ static int at76_usbdfu_download(struct usb_device *udev, u8
*buf, u32 size,
return -EINVAL;
}
- dfu_stat_buf = kmalloc(sizeof(struct dfu_status), GFP_KERNEL);
+ dfu_stat_buf = kmalloc(sizeof(*dfu_stat_buf), GFP_KERNEL);
if (!dfu_stat_buf) {
ret = -ENOMEM;
goto exit;
@@ -931,14 +931,12 @@ static void at76_dump_mib_mac_addr(struct at76_priv *priv)
{
int i;
int ret;
- struct mib_mac_addr *m = kmalloc(sizeof(struct mib_mac_addr),
-GFP_KERNEL);
+ struct mib_mac_addr *m = kmalloc(sizeof(*m), GFP_KERNEL);
if (!m)
return;
- ret = at76_get_mib(priv->udev, MIB_MAC_ADDR, m,
- sizeof(struct mib_mac_addr));
+ ret = at76_get_mib(priv->udev, MIB_MAC_ADDR, m, sizeof(*m));
if (ret < 0) {
wiphy_err(priv->hw->wiphy,
"at76_get_mib (MAC_ADDR) failed: %d\n", ret);
@@ -961,13 +959,12 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
int i;
int ret;
int key_len;
- struct mib_mac_wep *m = kmalloc(sizeof(struct mib_mac_wep), GFP_KERNEL);
+ struct mib_mac_wep *m = kmalloc(sizeof(*m), GFP_KERNEL);
if (!m)
return;
- ret = at76_get_mib(priv->udev, MIB_MAC_WEP, m,
- sizeof(struct mib_mac_wep));
+ ret = at76_get_mib(priv->udev, MIB_MAC_WEP, m, sizeof(*m));
if (ret < 0) {
wiphy_err(priv->hw->wiphy,
"at76_get_mib (MAC_WEP) failed: %d\n", ret);
@@ -997,14 +994,12 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
{
int ret;
- struct mib_mac_mgmt *m = kmalloc(sizeof(struct mib_mac_mgmt),
-GFP_KERNEL);
+ struct mib_mac_mgmt *m = kmalloc(sizeof(*m), GFP_KERNEL);
if (!m)
return;
- ret = at76_get_mib(priv->udev, MIB_MAC_MGMT, m,
- sizeof(struct mib_mac_mgmt));
+ ret = at76_get_mib(priv->udev, MIB_MAC_MGMT, m, sizeof(*m));
if (ret < 0) {
wiphy_err(priv->hw->wiphy,
"at76_get_mib (MAC_MGMT) failed: %d\n", ret);
@@ -1035,12 +1030,12 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv
*priv)
static void at76_dump_mib_mac(struct at76_priv *priv)
{
int ret;
- struct mib_mac *m = kmalloc(sizeof(struct mib_mac), GFP_KERNEL);
+ struct mib_mac *m = kmalloc(sizeof(*m), GFP_KERNEL);
if (!m)
return;
- ret = at76_get_mib(priv->udev, MIB_MAC, m, sizeof(struct mib_mac));
+ ret = at76_get_mib(priv->udev, MIB_MAC, m, sizeof(*m));
if (ret < 0) {
wiphy_err(priv->hw->wiphy,
"at76_get_mib (MAC) failed: %d\n", ret);
@@ -1072,12 +1067,12 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
static void at76_dump_mib_phy(struct at76_priv *priv)
{
int ret;
- struct mib_phy *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
+ struct mib_phy *m = kmalloc(sizeof(*m), GFP_KERNEL);
if (!m)
return;
- ret = at76_get_mib(priv->udev, MIB_PHY, m, sizeof(struct mib_phy));
+ ret = at76_get_mi
[PATCH 2/2] atmel: at76c50x: prefer struct_size over open coded arithmetic
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "cmd_buf" variable is a pointer to "struct at76_command" and
this structure ends in a flexible array:
struct at76_command {
[...]
u8 data[];
} __packed;
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count" in the
kmalloc() function.
Also, declare a new variable (total_size) since the return value of the
struct_size() helper is used several times.
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions). In this case, it is important to note
that the attribute used is "__counted_by_le" since the counter type is
"__le16".
This way, the code is more readable and safer.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
[1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer
---
drivers/net/wireless/atmel/at76c50x-usb.c | 12 ++--
drivers/net/wireless/atmel/at76c50x-usb.h | 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/atmel/at76c50x-usb.c
b/drivers/net/wireless/atmel/at76c50x-usb.c
index 0ca2f629683d..baa53cfefe48 100644
--- a/drivers/net/wireless/atmel/at76c50x-usb.c
+++ b/drivers/net/wireless/atmel/at76c50x-usb.c
@@ -721,9 +721,11 @@ static int at76_set_card_command(struct usb_device *udev,
u8 cmd, void *buf,
int buf_size)
{
int ret;
- struct at76_command *cmd_buf = kmalloc(sizeof(struct at76_command) +
- buf_size, GFP_KERNEL);
+ size_t total_size;
+ struct at76_command *cmd_buf;
+ total_size = struct_size(cmd_buf, data, buf_size);
+ cmd_buf = kmalloc(total_size, GFP_KERNEL);
if (!cmd_buf)
return -ENOMEM;
@@ -732,15 +734,13 @@ static int at76_set_card_command(struct usb_device *udev,
u8 cmd, void *buf,
cmd_buf->size = cpu_to_le16(buf_size);
memcpy(cmd_buf->data, buf, buf_size);
- at76_dbg_dump(DBG_CMD, cmd_buf, sizeof(struct at76_command) + buf_size,
+ at76_dbg_dump(DBG_CMD, cmd_buf, total_size,
"issuing command %s (0x%02x)",
at76_get_cmd_string(cmd), cmd);
ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x0e,
USB_TYPE_VENDOR | USB_DIR_OUT | USB_RECIP_DEVICE,
- 0, 0, cmd_buf,
- sizeof(struct at76_command) + buf_size,
- USB_CTRL_GET_TIMEOUT);
+ 0, 0, cmd_buf, total_size, USB_CTRL_GET_TIMEOUT);
kfree(cmd_buf);
return ret;
}
diff --git a/drivers/net/wireless/atmel/at76c50x-usb.h
b/drivers/net/wireless/atmel/at76c50x-usb.h
index 746e64dfd8aa..843146a0de64 100644
--- a/drivers/net/wireless/atmel/at76c50x-usb.h
+++ b/drivers/net/wireless/atmel/at76c50x-usb.h
@@ -151,7 +151,7 @@ struct at76_command {
u8 cmd;
u8 reserved;
__le16 size;
- u8 data[];
+ u8 data[] __counted_by_le(size);
} __packed;
/* Length of Atmel-specific Rx header before 802.11 frame */
--
2.25.1
[PATCH] input: misc: use sizeof(*pointer) instead of sizeof(type)
It is preferred to use sizeof(*pointer) instead of sizeof(type)
due to the type of the variable can change and one needs not
change the former (unlike the latter). This patch has no effect
on runtime behavior.
Signed-off-by: Erick Archer
---
drivers/input/misc/88pm80x_onkey.c | 2 +-
drivers/input/misc/cma3000_d0x.c| 2 +-
drivers/input/misc/ims-pcu.c| 4 ++--
drivers/input/misc/max8997_haptic.c | 2 +-
drivers/input/misc/pcap_keys.c | 2 +-
drivers/input/misc/powermate.c | 2 +-
drivers/input/misc/uinput.c | 2 +-
drivers/input/misc/yealink.c| 2 +-
8 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/input/misc/88pm80x_onkey.c
b/drivers/input/misc/88pm80x_onkey.c
index 4b0685f96113..6477a41c4bac 100644
--- a/drivers/input/misc/88pm80x_onkey.c
+++ b/drivers/input/misc/88pm80x_onkey.c
@@ -57,7 +57,7 @@ static int pm80x_onkey_probe(struct platform_device *pdev)
struct pm80x_onkey_info *info;
int err;
- info = kzalloc(sizeof(struct pm80x_onkey_info), GFP_KERNEL);
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
if (!info)
return -ENOMEM;
diff --git a/drivers/input/misc/cma3000_d0x.c b/drivers/input/misc/cma3000_d0x.c
index 1772846708d2..0c68e924a1cc 100644
--- a/drivers/input/misc/cma3000_d0x.c
+++ b/drivers/input/misc/cma3000_d0x.c
@@ -292,7 +292,7 @@ struct cma3000_accl_data *cma3000_init(struct device *dev,
int irq,
goto err_out;
}
- data = kzalloc(sizeof(struct cma3000_accl_data), GFP_KERNEL);
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
input_dev = input_allocate_device();
if (!data || !input_dev) {
error = -ENOMEM;
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 80d16c92a08b..408a586f8c36 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -287,7 +287,7 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
struct input_dev *input;
int error;
- gamepad = kzalloc(sizeof(struct ims_pcu_gamepad), GFP_KERNEL);
+ gamepad = kzalloc(sizeof(*gamepad), GFP_KERNEL);
input = input_allocate_device();
if (!gamepad || !input) {
dev_err(pcu->dev,
@@ -1993,7 +1993,7 @@ static int ims_pcu_probe(struct usb_interface *intf,
struct ims_pcu *pcu;
int error;
- pcu = kzalloc(sizeof(struct ims_pcu), GFP_KERNEL);
+ pcu = kzalloc(sizeof(*pcu), GFP_KERNEL);
if (!pcu)
return -ENOMEM;
diff --git a/drivers/input/misc/max8997_haptic.c
b/drivers/input/misc/max8997_haptic.c
index 8861a67be575..11cac4b7dddc 100644
--- a/drivers/input/misc/max8997_haptic.c
+++ b/drivers/input/misc/max8997_haptic.c
@@ -249,7 +249,7 @@ static int max8997_haptic_probe(struct platform_device
*pdev)
return -EINVAL;
}
- chip = kzalloc(sizeof(struct max8997_haptic), GFP_KERNEL);
+ chip = kzalloc(sizeof(*chip), GFP_KERNEL);
input_dev = input_allocate_device();
if (!chip || !input_dev) {
dev_err(&pdev->dev, "unable to allocate memory\n");
diff --git a/drivers/input/misc/pcap_keys.c b/drivers/input/misc/pcap_keys.c
index 8a7e9ada5952..f8954a2cab24 100644
--- a/drivers/input/misc/pcap_keys.c
+++ b/drivers/input/misc/pcap_keys.c
@@ -49,7 +49,7 @@ static int pcap_keys_probe(struct platform_device *pdev)
struct pcap_keys *pcap_keys;
struct input_dev *input_dev;
- pcap_keys = kmalloc(sizeof(struct pcap_keys), GFP_KERNEL);
+ pcap_keys = kmalloc(sizeof(*pcap_keys), GFP_KERNEL);
if (!pcap_keys)
return err;
diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c
index db2ba89adaef..4b039abffc4b 100644
--- a/drivers/input/misc/powermate.c
+++ b/drivers/input/misc/powermate.c
@@ -320,7 +320,7 @@ static int powermate_probe(struct usb_interface *intf,
const struct usb_device_i
0, interface->desc.bInterfaceNumber, NULL, 0,
USB_CTRL_SET_TIMEOUT);
- pm = kzalloc(sizeof(struct powermate_device), GFP_KERNEL);
+ pm = kzalloc(sizeof(*pm), GFP_KERNEL);
input_dev = input_allocate_device();
if (!pm || !input_dev)
goto fail1;
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index d98212d55108..d23f3225b00f 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -379,7 +379,7 @@ static int uinput_open(struct inode *inode, struct file
*file)
{
struct uinput_device *newdev;
- newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
+ newdev = kzalloc(sizeof(*newdev), GFP_KERNEL);
if (!newdev)
return -ENOMEM;
diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
index 69420781db30..c3221b960a75 100644
--- a/drivers/input/misc/yealink.c
+++ b/drivers/input/misc/yealink.c
@@
