| Message ID | 20180824011540.GA25716@embeddedor.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luca Coelho |
| Headers | show |
| Series | iwlwifi: d3: use struct_size() in kzalloc() | expand |
On Thu, Aug 23, 2018 at 6:15 PM, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote: > One of the more common cases of allocation size calculations is finding > the size of a structure that has a zero-sized array at the end, along > with memory for some number of elements for that array. For example: > > struct foo { > int stuff; > void *entry[]; > }; > > instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL); > > Instead of leaving these open-coded and prone to type mistakes, we can > now use the new struct_size() helper: > > instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL); > > This issue was detected with the help of Coccinelle. > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > index 79bdae9..6960318 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > @@ -1769,8 +1769,7 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm, > n_matches = 0; > } > > - net_detect = kzalloc(sizeof(*net_detect) + > - (n_matches * sizeof(net_detect->matches[0])), > + net_detect = kzalloc(struct_size(net_detect, matches, n_matches), > GFP_KERNEL); > if (!net_detect || !n_matches) > goto out_report_nd; > @@ -1785,8 +1784,7 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm, > for (j = 0; j < SCAN_OFFLOAD_MATCHING_CHANNELS_LEN; j++) > n_channels += hweight8(fw_match->matching_channels[j]); > > - match = kzalloc(sizeof(*match) + > - (n_channels * sizeof(*match->channels)), > + match = kzalloc(struct_size(match, channels, n_channels), > GFP_KERNEL); > if (!match) > goto out_report_nd; > -- > 2.7.4 >
On Thu, 2018-08-23 at 20:03 -0700, Kees Cook wrote: > On Thu, Aug 23, 2018 at 6:15 PM, Gustavo A. R. Silva > <gustavo@embeddedor.com> wrote: > > One of the more common cases of allocation size calculations is finding > > the size of a structure that has a zero-sized array at the end, along > > with memory for some number of elements for that array. For example: > > > > struct foo { > > int stuff; > > void *entry[]; > > }; Question for Gustavo. Did you find any existing instances that are miscalculated? I believe there are some cases like: size = sizeof(struct foo) + count * sizeof(something); ptr = kmalloc(size); memset(ptr + sizeof(struct foo), 0, size - sizeof(struct foo)); where something could go wrong and not be detected.
Hi Joe, On 8/23/18 11:33 PM, Joe Perches wrote: > On Thu, 2018-08-23 at 20:03 -0700, Kees Cook wrote: >> On Thu, Aug 23, 2018 at 6:15 PM, Gustavo A. R. Silva >> <gustavo@embeddedor.com> wrote: >>> One of the more common cases of allocation size calculations is finding >>> the size of a structure that has a zero-sized array at the end, along >>> with memory for some number of elements for that array. For example: >>> >>> struct foo { >>> int stuff; >>> void *entry[]; >>> }; > > Question for Gustavo. > > Did you find any existing instances that are miscalculated? > I found the following bug: https://lore.kernel.org/patchwork/patch/977357/ > I believe there are some cases like: > > size = sizeof(struct foo) + count * sizeof(something); > ptr = kmalloc(size); > memset(ptr + sizeof(struct foo), 0, size - sizeof(struct foo)); > > where something could go wrong and not be detected. > It might be worth it to write a Coccinelle script for this. -- Gustavo
On Thu, 2018-08-23 at 20:15 -0500, Gustavo A. R. Silva wrote: > One of the more common cases of allocation size calculations is finding > the size of a structure that has a zero-sized array at the end, along > with memory for some number of elements for that array. For example: > > struct foo { > int stuff; > void *entry[]; > }; > > instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL); > > Instead of leaving these open-coded and prone to type mistakes, we can > now use the new struct_size() helper: > > instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL); > > This issue was detected with the help of Coccinelle. > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > index 79bdae9..6960318 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > @@ -1769,8 +1769,7 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm, > n_matches = 0; > } > > - net_detect = kzalloc(sizeof(*net_detect) + > - (n_matches * sizeof(net_detect->matches[0])), > + net_detect = kzalloc(struct_size(net_detect, matches, n_matches), > GFP_KERNEL); > if (!net_detect || !n_matches) > goto out_report_nd; > @@ -1785,8 +1784,7 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm, > for (j = 0; j < SCAN_OFFLOAD_MATCHING_CHANNELS_LEN; j++) > n_channels += hweight8(fw_match->matching_channels[j]); > > - match = kzalloc(sizeof(*match) + > - (n_channels * sizeof(*match->channels)), > + match = kzalloc(struct_size(match, channels, n_channels), > GFP_KERNEL); > if (!match) > goto out_report_nd; Thanks! I applied this to our internal tree and it will reach the mainline following our normal process. -- Cheers, Luca.
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c index 79bdae9..6960318 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c @@ -1769,8 +1769,7 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm, n_matches = 0; } - net_detect = kzalloc(sizeof(*net_detect) + - (n_matches * sizeof(net_detect->matches[0])), + net_detect = kzalloc(struct_size(net_detect, matches, n_matches), GFP_KERNEL); if (!net_detect || !n_matches) goto out_report_nd; @@ -1785,8 +1784,7 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm, for (j = 0; j < SCAN_OFFLOAD_MATCHING_CHANNELS_LEN; j++) n_channels += hweight8(fw_match->matching_channels[j]); - match = kzalloc(sizeof(*match) + - (n_channels * sizeof(*match->channels)), + match = kzalloc(struct_size(match, channels, n_channels), GFP_KERNEL); if (!match) goto out_report_nd;
One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example: struct foo { int stuff; void *entry[]; }; instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL); Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper: instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL); This issue was detected with the help of Coccinelle. Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)