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

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

[Arnd Bergmann]

gcc-7 complains that wl3501_cs passes NULL into a function that
then uses the argument as the input for memcpy:

drivers/net/wireless/wl3501_cs.c: In function 'wl3501_get_scan':
include/net/iw_handler.h:559:3: error: argument 2 null where non-null expected [-Werror=nonnull]
   memcpy(stream + point_len, extra, iwe->u.data.length);

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

Making the length check explicit avoids the warning and should have
no other effect here.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/net/iw_handler.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/iw_handler.h b/include/net/iw_handler.h
index e0f4109e64c6..1a41043688bc 100644
--- a/include/net/iw_handler.h
+++ b/include/net/iw_handler.h
@@ -556,7 +556,8 @@ 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);
-		memcpy(stream + point_len, extra, iwe->u.data.length);
+		if (iwe->u.data.length)
+			memcpy(stream + point_len, extra, iwe->u.data.length);
 		stream += event_len;
 	}
 	return stream;
-- 
2.9.0

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

[Johannes Berg]

On Wed, 2017-01-11 at 15:35 +0100, Arnd Bergmann wrote:
> gcc-7 complains that wl3501_cs passes NULL into a function that
> then uses the argument as the input for memcpy:
> 
> drivers/net/wireless/wl3501_cs.c: In function 'wl3501_get_scan':
> include/net/iw_handler.h:559:3: error: argument 2 null where non-null 
> expected [-Werror=nonnull]
>    memcpy(stream + point_len, extra, iwe->u.data.length);

I love wext ;-)

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

johannes

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

[Arnd Bergmann]

On Wed, Jan 11, 2017 at 3:38 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2017-01-11 at 15:35 +0100, Arnd Bergmann wrote:

>> 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!

    Arnd

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

[Johannes Berg]

On Wed, 2017-01-11 at 16:00 +0100, Arnd Bergmann wrote:
> On Wed, Jan 11, 2017 at 3:38 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Wed, 2017-01-11 at 15:35 +0100, Arnd Bergmann wrote:
> > > 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

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

[Arnd Bergmann]

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

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

[Johannes Berg]

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:
> > 
> > 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

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

[Johannes Berg]

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

No, turns out that's basically impossible with all the compat etc.
stuff here.

johannes

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

[Johannes Berg]

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:
> > 
> > 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

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

[Arnd Bergmann]

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