If i < slot_id is initially true then it will remain true. Also,
as i is being decremented it will end up accessing memory out of
bounds.
Fix this by incrementing *i* instead of decrementing it.
Addresses-Coverity-ID: 1468454 ("Infinite loop")
Fixes: faa657641081 ("staging: wilc1000: refactor scan() to free kmalloc
memory on failure cases")
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
BTW... at first sight it seems to me that variables slot_id
and i should be of type unsigned instead of signed.
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 3ca0c97..67104e8 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -608,7 +608,7 @@ wilc_wfi_cfg_alloc_fill_ssid(struct cfg80211_scan_request *request,
out_free:
- for (i = 0; i < slot_id ; i--)
+ for (i = 0; i < slot_id; i++)
kfree(ntwk->net_info[i].ssid);
kfree(ntwk->net_info);
--
2.7.4
Reviewed-by: Ajay Singh <[email protected]>
On Mon, 30 Apr 2018 07:50:40 -0500
"Gustavo A. R. Silva" <[email protected]> wrote:
> If i < slot_id is initially true then it will remain true. Also,
> as i is being decremented it will end up accessing memory out of
> bounds.
>
> Fix this by incrementing *i* instead of decrementing it.
Nice catch!
Thanks for submitting the changes.
>
> Addresses-Coverity-ID: 1468454 ("Infinite loop")
> Fixes: faa657641081 ("staging: wilc1000: refactor scan() to free
> kmalloc memory on failure cases")
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> ---
>
> BTW... at first sight it seems to me that variables slot_id
> and i should be of type unsigned instead of signed.
Yes, 'slot_id' & 'i' can be changed to unsigned int.
>
> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> 3ca0c97..67104e8 100644 ---
> a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -608,7
> +608,7 @@ wilc_wfi_cfg_alloc_fill_ssid(struct cfg80211_scan_request
> *request, out_free:
>
> - for (i = 0; i < slot_id ; i--)
> + for (i = 0; i < slot_id; i++)
> kfree(ntwk->net_info[i].ssid);
>
> kfree(ntwk->net_info);
Regards,
Ajay
On Mon, Apr 30, 2018 at 07:59:16PM +0530, Ajay Singh wrote:
> Reviewed-by: Ajay Singh <[email protected]>
>
> On Mon, 30 Apr 2018 07:50:40 -0500
> "Gustavo A. R. Silva" <[email protected]> wrote:
>
> > If i < slot_id is initially true then it will remain true. Also,
> > as i is being decremented it will end up accessing memory out of
> > bounds.
> >
> > Fix this by incrementing *i* instead of decrementing it.
>
> Nice catch!
> Thanks for submitting the changes.
>
> >
> > Addresses-Coverity-ID: 1468454 ("Infinite loop")
> > Fixes: faa657641081 ("staging: wilc1000: refactor scan() to free
> > kmalloc memory on failure cases")
> > Signed-off-by: Gustavo A. R. Silva <[email protected]>
> > ---
> >
> > BTW... at first sight it seems to me that variables slot_id
> > and i should be of type unsigned instead of signed.
>
> Yes, 'slot_id' & 'i' can be changed to unsigned int.
>
A lot of people think making things unsigned improves the code but I
hate "unsigned int i"... I understand that in certain cases we do loop
more than INT_MAX but those are a tiny tiny minority of loops.
Simple types like "int" are more readable than complicated types like
"unsigned int". Unsigned int just draws attention to itself and
distracts people from things that might potentially matter. We have
real life subtle loops like in xtea_encrypt() where we need to use
unsigned types.
And look at the function we're talking about here:
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
577 static inline int
578 wilc_wfi_cfg_alloc_fill_ssid(struct cfg80211_scan_request *request,
579 struct hidden_network *ntwk)
580 {
581 int i;
582 int slot_id = 0;
583
584 ntwk->net_info = kcalloc(request->n_ssids,
585 sizeof(struct hidden_network), GFP_KERNEL);
586 if (!ntwk->net_info)
587 goto out;
588
589 ntwk->n_ssids = request->n_ssids;
590
591 for (i = 0; i < request->n_ssids; i++) {
request->n_ssids is an int. It can't possibly be INT_MAX because the
kcalloc() will fail. Ideally the static analysis tool should be able to
tell you that if you seed it with the knowledge of how much memory it's
possible to kmalloc(). So it's just laziness here is why the static
checker complains, it should see there is no issue. Smatch fails here
as well but I'll see if I can fix it.
Anyway let's say it could be negative then 0 is greater than negative
values so the loop would be a no-op. I've seen code where the user
could set the loop bounds to s32min-4 but because it was "int i" instead
of "unsigned int i" then it meant the loop was a no-op instead of being
a security problem. In other words, unsigned can be less secure.
I honestly have never seen a bug in the kernel where we intended to loop
more than INT_MAX times, but there was a signedness bug preventing it.
That's the only reason I can see to change the signedness. Am I missing
something?
592 if (request->ssids[i].ssid_len > 0) {
593 struct hidden_net_info *info = &ntwk->net_info[slot_id];
594
595 info->ssid = kmemdup(request->ssids[i].ssid,
596 request->ssids[i].ssid_len,
597 GFP_KERNEL);
598 if (!info->ssid)
599 goto out_free;
600
601 info->ssid_len = request->ssids[i].ssid_len;
602 slot_id++;
603 } else {
604 ntwk->n_ssids -= 1;
605 }
606 }
607 return 0;
608
609 out_free:
610
611 for (i = 0; i < slot_id ; i--)
612 kfree(ntwk->net_info[i].ssid);
613
614 kfree(ntwk->net_info);
615 out:
616
617 return -ENOMEM;
618 }
regards,
dan carpenter
On Mon, 30 Apr 2018 18:23:21 +0300
Dan Carpenter <[email protected]> wrote:
> On Mon, Apr 30, 2018 at 07:59:16PM +0530, Ajay Singh wrote:
> > Reviewed-by: Ajay Singh <[email protected]>
> >
> > On Mon, 30 Apr 2018 07:50:40 -0500
> > "Gustavo A. R. Silva" <[email protected]> wrote:
> >
> > > If i < slot_id is initially true then it will remain true. Also,
> > > as i is being decremented it will end up accessing memory out of
> > > bounds.
> > >
> > > Fix this by incrementing *i* instead of decrementing it.
> >
> > Nice catch!
> > Thanks for submitting the changes.
> >
> > >
> > > Addresses-Coverity-ID: 1468454 ("Infinite loop")
> > > Fixes: faa657641081 ("staging: wilc1000: refactor scan() to free
> > > kmalloc memory on failure cases")
> > > Signed-off-by: Gustavo A. R. Silva <[email protected]>
> > > ---
> > >
> > > BTW... at first sight it seems to me that variables slot_id
> > > and i should be of type unsigned instead of signed.
> >
> > Yes, 'slot_id' & 'i' can be changed to unsigned int.
> >
>
> A lot of people think making things unsigned improves the code but I
> hate "unsigned int i"... I understand that in certain cases we do
> loop more than INT_MAX but those are a tiny tiny minority of loops.
>
> Simple types like "int" are more readable than complicated types
> like "unsigned int". Unsigned int just draws attention to itself
> and distracts people from things that might potentially matter. We
> have real life subtle loops like in xtea_encrypt() where we need to
> use unsigned types.
>
> And look at the function we're talking about here:
>
> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> 577 static inline int
> 578 wilc_wfi_cfg_alloc_fill_ssid(struct cfg80211_scan_request
> *request, 579 struct hidden_network
> *ntwk) 580 {
> 581 int i;
> 582 int slot_id = 0;
> 583
> 584 ntwk->net_info = kcalloc(request->n_ssids,
> 585 sizeof(struct
> hidden_network), GFP_KERNEL); 586 if (!ntwk->net_info)
> 587 goto out;
> 588
> 589 ntwk->n_ssids = request->n_ssids;
> 590
> 591 for (i = 0; i < request->n_ssids; i++) {
>
> request->n_ssids is an int. It can't possibly be INT_MAX because
> the kcalloc() will fail. Ideally the static analysis tool should
> be able to tell you that if you seed it with the knowledge of how
> much memory it's possible to kmalloc(). So it's just laziness here
> is why the static checker complains, it should see there is no
> issue. Smatch fails here as well but I'll see if I can fix it.
>
> Anyway let's say it could be negative then 0 is greater than
> negative values so the loop would be a no-op. I've seen code where
> the user could set the loop bounds to s32min-4 but because it was
> "int i" instead of "unsigned int i" then it meant the loop was a
> no-op instead of being a security problem. In other words,
> unsigned can be less secure.
>
> I honestly have never seen a bug in the kernel where we intended to
> loop more than INT_MAX times, but there was a signedness bug
> preventing it. That's the only reason I can see to change the
> signedness. Am I missing something?
>
Hi Dan,
Thanks for providing the detailed explanation.
I understand your point, but what's your opinion about variable
'slot_id', as this variable is added to take only the positive
value(i.e from 0 to maximum number of filled elements), so for more
readability should we keep it same.
BTW in my opinion 'request->n_ssids' should have been unsigned
because it is suppose to hold only positive values along with
smaller data type(may be u8 or u16, as the range might be enough to
hold the value of n_ssids). So we have advantage of size reduction
not just the increase in value range.
Suppose we get negative value for "request->n_ssids" & kmalloc is
pass then we have to add some more extra code to free data and
return error code in wilc_wfi_cfg_alloc_fill_ssid(). In your opinion
should this condition be handled as the received 'request->n_ssids' is
of 'int' type.
Regards,
Ajay
On Wed, 2 May 2018 11:39:36 +0300
Dan Carpenter <[email protected]> wrote:
> We're mainly discussing readability, right?
>
> To me when people use "int" that tells me as a reader that we don't
> need to think about the type. It's going to be a small number.
>
> Say you have data which the user can control, then it's super
> important to focus on the data types. We don't focus on it
> enough. There is some kind of idea that good developers should
> just be super focused on everything all the time, but I don't think
> humans can do it. So to me it's useful when the author tells me,
> "This an int type. It's fine. This is not critical."
>
> If you make request->n_ssids a u8 or u16, that isn't going to save
> any memory because the struct is padded. You'd also need to audit
> a bunch of code to make sure that we don't overflow the u16. If
> you wanted to overflow the int, you'd need to allocate several gigs
> of memory but kmalloc() is capped at KMALLOC_MAX_SIZE (4MB) so
> that's not possible. How many of these structs do we allocate? Is
> it really worth optimizing the heck out of it?
>
> There are times where want to be very deliberate with our types
> because we're dealing the large numbers, or user data or fast
> paths. But there are other times where int is fine...
>
As in this case, its fine to be of 'int' type.
So we can retain the current data type('int') for 'i' and 'slot_id'.
Thank you for sharing your insights,it was very helpful.
Regards,
Ajay
We're mainly discussing readability, right?
To me when people use "int" that tells me as a reader that we don't need
to think about the type. It's going to be a small number.
Say you have data which the user can control, then it's super important
to focus on the data types. We don't focus on it enough. There is some
kind of idea that good developers should just be super focused on
everything all the time, but I don't think humans can do it. So to me
it's useful when the author tells me, "This an int type. It's fine.
This is not critical."
If you make request->n_ssids a u8 or u16, that isn't going to save any
memory because the struct is padded. You'd also need to audit a bunch
of code to make sure that we don't overflow the u16. If you wanted to
overflow the int, you'd need to allocate several gigs of memory but
kmalloc() is capped at KMALLOC_MAX_SIZE (4MB) so that's not possible.
How many of these structs do we allocate? Is it really worth optimizing
the heck out of it?
There are times where want to be very deliberate with our types because
we're dealing the large numbers, or user data or fast paths. But there
are other times where int is fine...
regards,
dan carpenter