Re: [PATCH] wext: handle NULL exta data in iwe_stream_add_point better

On Wed, 2017-01-11 at 16:00 +0100, Arnd Bergmann wrote:

On Wed, Jan 11, 2017 at 3:38 PM, Johannes Berg
[off-list ref] wrote:

quoted

On Wed, 2017-01-11 at 15:35 +0100, Arnd Bergmann wrote:

quoted

This works fine here because iwe->u.data.length is guaranteed to
be
NULL, and the memcpy doesn't actually have an effect.

I think you mean 0, not NULL, but I can fix that when I apply it.

Right, thanks!

Applied. Also fixed the typo in the subject :)

johannes

On Wednesday, January 11, 2017 4:06:17 PM CET Johannes Berg wrote:

Applied. Also fixed the typo in the subject :)

Thanks! Unfortunately I now got another warning for the same function,
and though I would have expected the patch to fix it, that did not work:

In file included from /git/arm-soc/drivers/net/wireless/intersil/prism54/islpci_dev.h:27:0,
                 from /git/arm-soc/drivers/net/wireless/intersil/prism54/isl_ioctl.h:24,
                 from /git/arm-soc/drivers/net/wireless/intersil/prism54/isl_ioctl.c:32:
/git/arm-soc/drivers/net/wireless/intersil/prism54/isl_ioctl.c: In function 'prism54_get_scan':
/git/arm-soc/include/net/iw_handler.h:560:4: error: argument 2 null where non-null expected [-Werror=nonnull]
    memcpy(stream + point_len, extra, iwe->u.data.length);

The change below kills that warning too, but it gets even uglier there:

diff --git a/include/net/iw_handler.h b/include/net/iw_handler.h
index 1a41043688bc..c2aa73e5e6bb 100644
--- a/include/net/iw_handler.h
+++ b/include/net/iw_handler.h

@@ -556,7 +556,7 @@ iwe_stream_add_point(struct iw_request_info *info, char *stream, char *ends,
 		memcpy(stream + lcp_len,
 		       ((char *) &iwe->u) + IW_EV_POINT_OFF,
 		       IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN);
-		if (iwe->u.data.length)
+		if (iwe->u.data.length && extra)
 			memcpy(stream + point_len, extra, iwe->u.data.length);
 		stream += event_len;
 	}

Let me know if you want a proper follow-up patch, or if you can amend your
commit, or you have a better idea for resolving that warning.

	Arnd

On Wed, 2017-01-11 at 21:39 +0100, Arnd Bergmann wrote:

On Wednesday, January 11, 2017 4:06:17 PM CET Johannes Berg wrote:

quoted

Applied. Also fixed the typo in the subject :)

Thanks! Unfortunately I now got another warning for the same
function, and though I would have expected the patch to fix it, that
did not work:

I've come to expect better of you (i.e. testing your own patches) ;-)


Come to think of it, I'm thinking I should drop this patch and the
driver should just use iwe_stream_add_event() instead? It'll be
somewhat tricky to get the length correct though.

Alternatively, perhaps we should just uninline all the crap and then
the compiler can't bother us :)

johannes

On Wed, 2017-01-11 at 21:39 +0100, Arnd Bergmann wrote:

On Wednesday, January 11, 2017 4:06:17 PM CET Johannes Berg wrote:

quoted

Applied. Also fixed the typo in the subject :)

Thanks! Unfortunately I now got another warning for the same
function, and though I would have expected the patch to fix it, that
did not work:

In file included from /git/arm-
soc/drivers/net/wireless/intersil/prism54/islpci_dev.h:27:0,
                 from /git/arm-
soc/drivers/net/wireless/intersil/prism54/isl_ioctl.h:24,
                 from /git/arm-
soc/drivers/net/wireless/intersil/prism54/isl_ioctl.c:32:
/git/arm-soc/drivers/net/wireless/intersil/prism54/isl_ioctl.c: In
function 'prism54_get_scan':
/git/arm-soc/include/net/iw_handler.h:560:4: error: argument 2 null
where non-null expected [-Werror=nonnull]
    memcpy(stream + point_len, extra, iwe->u.data.length);

And I realized only now that this was a different place ...

I've just added the check you suggested - spent way too much time
already on this old crap :)

johannes

On Thursday, January 12, 2017 10:16:00 AM CET Johannes Berg wrote:

And I realized only now that this was a different place ...

Right, it was a few hundred randconfigs later after I had confirmed
that the first patch fixed all the configurations that were broken
at first.

I've just added the check you suggested - spent way too much time
already on this old crap 

Ok, thanks! Let's hope it doesn't come back once more.

I'm still trying to categorize the newly added warnings in gcc-7,
there a number of very useful warnings that got added, but some of
them are rather noisy and find both a number of real bugs and
false positives. The NULL check had only a few findings that all
seemed worth fixing.

	Arnd