Re: [PATCH 01/11] staging: wilc1000: refactor scan() to free kmalloc memory on failure cases

Hi Dan,

Thanks for your detailed review comments.

On Tue, 20 Mar 2018 22:46:32 +0300
Dan Carpenter [off-list ref] wrote:

On Tue, Mar 20, 2018 at 10:25:34PM +0530, Ajay Singh wrote:

quoted

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@microchip.com>

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.

quoted

+
+	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.

quoted

+	}
+	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