Hi Dan,
Thanks for your detailed review comments.
On Tue, 20 Mar 2018 22:46:32 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> On Tue, Mar 20, 2018 at 10:25:34PM +0530, Ajay Singh wrote:
> > Added changes to free the allocated memory in scan() for error condition.
> > Also added 'NULL' check validation before accessing allocated memory.
> >
> > Signed-off-by: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
> Don't put a blank line between the alloc and the check. They're as
> connected as can be. I hate "goto out;" but that is a personal
> preference which I would never push on to other developers...
>
I will modify the code to address the review comments and will send the
updated patch.
> > +
> > + ntwk->n_ssids = request->n_ssids;
> > +
> > + for (i = 0; i < request->n_ssids; i++) {
> > + if (request->ssids[i].ssid_len > 0) {
> > + struct hidden_net_info *info = &ntwk->net_info[i];
> > +
> > + info->ssid = kmemdup(request->ssids[i].ssid,
> > + request->ssids[i].ssid_len,
> > + GFP_KERNEL);
> > +
> > + if (!info->ssid)
> > + goto out_free;
> > +
> > + info->ssid_len = request->ssids[i].ssid_len;
> > + } else {
> > + ntwk->n_ssids -= 1;
> > + }
>
> You didn't introduce the problem, but this loop seems kind of buggy. We
> should have two iterators, one for request->ssids[i] and one for
> ntwk->net_info[i]. Otherwise we're copying the array information but
> we're leaving holes in the destination array. Which would be fine
> except we're not saving the totaly number of elements in the destination
> array, we're saving the number of elements with stuff in them.
>
> So imagine that we have a request->n_ssids == 10 but only the last
> three elements have request->ssids[i].ssid_len > 0. Then we record that
> ntwk->n_ssids is 3 but wthose elements are all holes. So that can't
> work. See handle_scan():
>
> for (i = 0; i < hidden_net->n_ssids; i++)
> valuesize += ((hidden_net->net_info[i].ssid_len) + 1);
>
> "valuesize" is wrong because it's looking at holes.
While testing, I found that the last element in request->ssids the
ssid_len is zero. For in between elements the values has some valid
length. I only tested for 'connect with AP' and 'iw scan' operation.
The scenario you have mention can occur for some instance. So, I will
modify the code to not have holes in allocated array at the time of
filling the data.
I will include these changes and send updated v2 patch set.
>
> > + }
> > + return true;
> > +
> > +out_free:
> > +
> > + for (; i >= 0 ; i--)
> > + kfree(ntwk->net_info[i].ssid);
>
> The first kfree(ntwk->net_info[i].ssid); is a no-op. You could write
> this like:
>
> while (--i >= 0)
> kfree(ntwk->net_info[i].ssid);
>
I will include these changes and send the updated patch.
Regards,
Ajay
