PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[D. Hugh Redelmeier]

Using netlink.h's NLMSG_OK correctly will cause GCC to issue a warning
on systems with 32-bit userland.  The definition can easily be changed
to avoid this.

GCC's warning is to flag comparisons where C's implicit "Usual
Arithmetic Conversions" could lead to a surprising result.

Consider this context:
	int i;
	unsigned u;
The C standard says that i<u is evaluated as (unsigned)i < u.
This can easily be a surprise because a negative value of i will be 
silently treated as a very large positive value.

In include/uapi/linux/netlink.h:
#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
			   (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
			   (nlh)->nlmsg_len <= (len))

This comparison looks suspicious to GCC:
	(nlh)->nlmsg_len <= (len)
nlmsg_len is of type __u32.  If int is 32 bits, this compare will be
done as
	(nlh)->nlmsg_len <= (unsigned)(len)
We know that this is actually safe because the first conjunct
determined that len isn't negative but the GCC apparently doesn't know.

A change that would calm GCC and also be correct would be to add
a cast to unsigned:
	(nlh)->nlmsg_len <= (unsigned)(len))
But I imagine that len might well actually have type ssize_t.  It is
often the result of a call to recvfrom(2), which is a ssize_t.  So I
think that this would be safer:
	(nlh)->nlmsg_len <= (size_t)(len))
I know of no system where size_t is narrower than unsigned.

This problem came up when building a userland component of libreswan in a 
32-bit environment with a recent GCC and was reported by Lennart Sorensen.

Signed-off-by: D. Hugh Redelmeier <hugh@mimosa.com>

diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 6f3fe16..dd15537 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -86,7 +86,7 @@ struct nlmsghdr {
 				  (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
 #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
 			   (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
-			   (nlh)->nlmsg_len <= (len))
+			   (nlh)->nlmsg_len <= (size_t)(len))
 #define NLMSG_PAYLOAD(nlh,len) ((nlh)->nlmsg_len - NLMSG_SPACE((len)))
 
 #define NLMSG_NOOP		0x1	/* Nothing.		*/

Re: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[David Miller]

From: "D. Hugh Redelmeier" <hugh@mimosa.com>
Date: Tue, 8 Sep 2015 09:46:56 -0400 (EDT)

> Using netlink.h's NLMSG_OK correctly will cause GCC to issue a warning
> on systems with 32-bit userland.  The definition can easily be changed
> to avoid this.

Everyone either uses an unsigned type (such as "size_t") or adds an
explicit cast to an unsinged type for the second argument.

The code you were trying to compile should do so as well.

I'm not applying this, sorry.

Re: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[D. Hugh Redelmeier]

| From: David Miller <davem@davemloft.net>

| From: "D. Hugh Redelmeier" <hugh@mimosa.com>
| Date: Tue, 8 Sep 2015 09:46:56 -0400 (EDT)
| 
| > Using netlink.h's NLMSG_OK correctly will cause GCC to issue a warning
| > on systems with 32-bit userland.  The definition can easily be changed
| > to avoid this.
| 
| Everyone either uses an unsigned type (such as "size_t") or adds an
| explicit cast to an unsinged type for the second argument.

1) netlink(3) says that the type of the second parameter is "int".
   From the synopsis:
       int NLMSG_OK(struct nlmsghdr *nlh, int len);
   Surely then "int" should be appropriate.

2) if you use the type "unsigned int" on a 32-bit machine, you get the 
   warning for an earlier conjunct:

#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
			   (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
			   (nlh)->nlmsg_len <= (len))

   (len) >= (int)sizeof(struct nlmsghdr)  <=== unsigned >= int

3) on a 32-bit machine, size_t is likely "unsigned int" so the
   same problem as (2) should arise.

4) on a 64-bit machine with 64-bit ints, the same problems are likely.
   I don't have one to test on.

Casting to "short" or "unsigned short" works, but I don't know that
the value is guaranteed to fit in either of them.

| I'm not applying this, sorry.

Thanks for looking at this.  Could you reconsider?

An alternative would be to change netlink(3) to say that len is
of type ssize_t or size_t (there are arguments for each).  This would
be cleaner, but it would be an API change (at least in theory).  The
macro would still have to be changed, just in a different way.

Re: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[David Miller]

From: "D. Hugh Redelmeier" <hugh@mimosa.com>
Date: Wed, 9 Sep 2015 16:24:07 -0400 (EDT)

> | From: David Miller <davem@davemloft.net>
> 
> | From: "D. Hugh Redelmeier" <hugh@mimosa.com>
> | Date: Tue, 8 Sep 2015 09:46:56 -0400 (EDT)
> | 
> | > Using netlink.h's NLMSG_OK correctly will cause GCC to issue a warning
> | > on systems with 32-bit userland.  The definition can easily be changed
> | > to avoid this.
> | 
> | Everyone either uses an unsigned type (such as "size_t") or adds an
> | explicit cast to an unsinged type for the second argument.
> 
> 1) netlink(3) says that the type of the second parameter is "int".
>    From the synopsis:
>        int NLMSG_OK(struct nlmsghdr *nlh, int len);
>    Surely then "int" should be appropriate.

Documentation can, and often is, wrong.  The code that has been there
for more than two decades determines what the interface and semantics
actually are.

Whatever is actually in the macro is what people have to accomodate
and cope with.

Re: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[D. Hugh Redelmeier]

| From: David Miller <davem@davemloft.net>

| From: "D. Hugh Redelmeier" <hugh@mimosa.com>
| Date: Wed, 9 Sep 2015 16:24:07 -0400 (EDT)

| > 1) netlink(3) says that the type of the second parameter is "int".
| >    From the synopsis:
| >        int NLMSG_OK(struct nlmsghdr *nlh, int len);
| >    Surely then "int" should be appropriate.
| 
| Documentation can, and often is, wrong.  The code that has been there
| for more than two decades determines what the interface and semantics
| actually are.
| 
| Whatever is actually in the macro is what people have to accomodate
| and cope with.

That's pretty scary.  Especially since I've found that more than half
the files in Fedora that used the macor used a signed type.

Thanks for responding to the start of my message.  Here's the rest
again in case you missed it.

2) if you use the type "unsigned int" on a 32-bit machine, you get the 
   warning for an earlier conjunct:

#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
			   (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
			   (nlh)->nlmsg_len <= (len))

   (len) >= (int)sizeof(struct nlmsghdr)  <=== unsigned >= int

3) on a 32-bit machine, size_t is likely "unsigned int" so the
   same problem as (2) should arise.

4) on a 64-bit machine with 64-bit ints, the same problems are likely.
   I don't have one to test on.

Casting to "short" or "unsigned short" works, but I don't know that
the value is guaranteed to fit in either of them.

| I'm not applying this, sorry.

Thanks for looking at this.  Could you reconsider?

An alternative would be to change netlink(3) to say that len is
of type ssize_t or size_t (there are arguments for each).  This would
be cleaner, but it would be an API change (at least in theory).  The
macro would still have to be changed, just in a different way.

RE: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[David Laight]

From: D. Hugh Redelmeier
> Sent: 09 September 2015 21:24
...
> 2) if you use the type "unsigned int" on a 32-bit machine, you get the
>    warning for an earlier conjunct:
> 
> #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
> 			   (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
> 			   (nlh)->nlmsg_len <= (len))
> 
>    (len) >= (int)sizeof(struct nlmsghdr)  <=== unsigned >= int
> 
> 3) on a 32-bit machine, size_t is likely "unsigned int" so the
>    same problem as (2) should arise.
> 
> 4) on a 64-bit machine with 64-bit ints, the same problems are likely.
>    I don't have one to test on.
> 
> Casting to "short" or "unsigned short" works, but I don't know that
> the value is guaranteed to fit in either of them.

Why not cast (nlh)->nl_msg_len instead?
Or subtract the two values and compare against zero?
Perhaps:
	(typeof (len))(nlh)->nlmsg_len <= (len)
which is almost certainly safe unless 'len' is 'signed char'.

	David

RE: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[D. Hugh Redelmeier]

| From: David Laight <David.Laight@ACULAB.COM>

| From: D. Hugh Redelmeier

| > #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
| > 			   (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
| > 			   (nlh)->nlmsg_len <= (len))

| Why not cast (nlh)->nl_msg_len instead?
| Perhaps:
| 	(typeof (len))(nlh)->nlmsg_len <= (len)
| which is almost certainly safe unless 'len' is 'signed char'.

(Nit pick: it doesn't feel safe to cast to short either.)

You would also want to adjust the first compare to
	(len) >= (typeof (len))sizeof(struct nlmsghdr)
Remember that, on 32-bit machines, with the current macro, GCC gives a
warning on the first compare if len is unsigned.

These casts together might cover up some weird typing errors, like
using char * or double.

This doesn't seem like a bad fix.  It probably doesn't break any
currently working code unless something depended on one of the
surprising conversions.

I think that this change is better than the status quo but
isn't the best place to get to.


The most common source of the value is from a call to one of the
recv(2) functions.  These functions return a ssize_t.  In particular,
the error indicator is -1.

=> The error indicator -1 is only handled correctly by NLMSG_OK if the
   first compare is a signed compare.

The right change is to force the type to be ssize_t.  This should
break no currently working code (unless it is accidentally working).
After all, no practical length would be positive in size_t and negative in
ssize_t.

Using "int" instead of ssize_t would work on all Linux machines that I
know and has the merit of matching the documentation, so that is the
code change I suggested.

| Or subtract the two values and compare against zero?

If it is an unsigned subtract, the result cannot be negative, so that
doesn't work.

If it is a signed subtract, you have just the same problems as a
signed compare, plus you can get overflow (undefined in C, but
probably not in Linux).

So this isn't a win.

Re: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[D. Hugh Redelmeier]

| From: David Miller <davem@davemloft.net>

| Everyone either uses an unsigned type (such as "size_t") or adds an
| explicit cast to an unsinged type for the second argument.

That sounded odd to me.  So I looked through the Fedora code base, as
indexed by searchcode.com.  This took a stupid amount of time so I
didn't check my work.

Summary: 37 files use a signed type, 32 files use an unsigned type
(some are in each category!).

So your point is not correct.

It is amazing how many private definitions there are for NLMSG_OK.
They are mostly just copies but at least some leave out the int cast of
the result of sizeof.

The kernel's own version seems to be an inline function (with a
lower-case name).  It would be nice to replace the NLMSG_OK macro with
an inline function: this supports better type checking.  I don't know
if there are technical reasons not to do this.

Supporting data:

strongswan /strongswan-5.0.0/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c
	int, size_t, size_t, size_t, size_t, size_t

libnfnetlink /libnfnetlink-1.0.0/src/libnfnetlink.c
	size_t, size_t, size_t, unsigned int

glibc /glibc-2.16-75f0d304/sysdeps/unix/sysv/linux/ifaddrs.c
arm-gp2x-linux-glibc /arm-gp2x-linux-glibc-2.3.6/glibc-2.3.6/sysdeps/unix/sysv/linux/ifaddrs.c
	size_t, size_t, size_t

strongswan /strongswan-5.0.0/src/libhydra/plugins/kernel_netlink/kernel_netlink_net.c
	int, size_t, size_t, size_t

fcoe-utils /fcoe-utils-1.0.23/fcoemon.c
	int, int, int

uClibc /uClibc-0.9.32/libc/inet/ifaddrs.c
	size_t, size_t, size_t

glibc /glibc-2.16-75f0d304/sysdeps/unix/sysv/linux/if_index.c
arm-gp2x-linux-glibc /arm-gp2x-linux-glibc-2.3.6/glibc-2.3.6/sysdeps/unix/sysv/linux/if_index.c
	size_t, size_t

bootchart /bootchart2-0.14.0/collector/tasks-netlink.c
	size_, size_t

netlabel_tools /netlabel_tools-0.19/libnetlabel/mod_mgmt.c
netlabel_tools /netlabel_tools-0.19/libnetlabel/mod_unlabeled.c
	each has int, int

bird /bird-1.3.7/sysdep/linux/netlink/netlink.c
	unsigned int, unsigned int

gtk-gnutella /gtk-gnutella-0.97.1/src/lib/getgateway.c
	unsigned, unsigned

iproute /iproute2-3.4.0/lib/libnetlink.c
	int, int

iproute /iproute2-3.4.0/misc/ss.c
	int, ssize_t

lldpad /lldpad-0.9.44/lldp_dcbx_nl.c
	unsigned

dlm /dlm-3.99.5/dlm_controld/netlink.c
	int, int

gnet2 /gnet-2.0.8/src/usagi_ifaddrs.c
	int, int

dnsmasq /dnsmasq-2.59/src/netlink.c
	size_t (through cast of ssize_t value), size_t (through cast of ssize_t value)

net-snmp /net-snmp-5.7.1/agent/mibgroup/ip-mib/data_access/ipaddress_linux.c
	int, int

net-snmp /net-snmp-5.7.1/agent/mibgroup/ip-mib/data_access/defaultrouter_linux.c
	int

rb_libtorrent /libtorrent-rasterbar-0.16.2/src/enum_net.cpp
	int, int

bootparamd /netkit-bootparamd-0.17/rpc.bootparamd/bootparam_default_route.c
	u_int (through cast of int value), u_int (through cast of int value)

nss-myhostname /nss-myhostname-0.3/netlink.c
	size_t (through cast of ssize_t value)

glibc /glibc-2.16-75f0d304/sysdeps/unix/sysv/linux/check_native.c
glibc /glibc-2.16-75f0d304/sysdeps/unix/sysv/linux/check_pf.c
	size_t (through cast of ssize_t value)

audit /audit-2.2.1/lib/netlink.c
	unsigned int (through cast of int)

rstp /rstp/libnetlink.c
	int

fence-virt /fence-virt-0.3.0/common/ip_lookup.c
	int

libpcap /libpcap-1.3.0/pcap-netfilter-linux.c
	int

oidentd /oidentd-2.0.8/src/kernel/linux.c
	has it's own copy of the definition of NLMSG_OK, without cast to int
	size_t (cast of ssize_t value)

netlabel_tools /netlabel_tools-0.19/libnetlabel/netlabel_comm.c
	int

netlabel_tools /netlabel_tools-0.19/libnetlabel/mod_cipsov4.c
	int

keepalived /keepalived-1.2.2/keepalived/vrrp/vrrp_netlink.c
	int

xen /xen-4.1.2/tools/python/xen/lowlevel/netlink/libnetlink.c
	int

mipv6-daemon /mipv6-daemon-2.0.2.20110203bgit/libnetlink/libnetlink.c
	size_t (cast of int value)

avahi /avahi-0.6.31/avahi-core/netlink.c
	size_t (cast of ssize_t)

avahi /avahi-0.6.31/avahi-autoipd/iface-linux.c
	size_t

libcgroup /libcgroup-0.38/src/daemon/cgrulesengd.c
	size_t (but code would probably be more correct with ssize_t)

libnl /libnl-1.1/lib/nl.c
	int

pl /pl-6.0.2/packages/tipc/tipcutils/tipc-config.c
	int

mingw-glib2 /glib-2.33.1/gio/gnetworkmonitornetlink.c
	size_t (cast of gssize value)

fcoe-utils /fcoe-utils-1.0.23/lib/rtnetlink.c
	int

libnl3 /libnl-3.2.7/lib/nl.c
	int

iscsi-initiator-utils /open-iscsi-2.0-872-rc4-bnx2i/usr/dcb_app.c
	unsigned int (cast from int)

lldpad /lldpad-0.9.44/lldp_rtnl.c
	unsigned

lldpad /lldpad-0.9.44/nltest.c
	unsigned int (cast from int)

ebtables /ebtables-v2.0.10-4/examples/ulog/test_ulog.c
	65536 (odd! bug?); counts as an int

wpa_supplicant /wpa_supplicant-1.0-rc3/src/drivers/netlink.c
	int

ipsec-tools /ipsec-tools-0.8.0/src/racoon/grabmyaddr.c
	int

ulogd /ulogd-2.0.0beta4/libipulog/libipulog.c
	size_t

gupnp /gupnp-0.18.3/libgupnp/gupnp-linux-context-manager.c
	int

ike /ike/source/libip/libip.route.cpp
	int

libnfnetlink /libnfnetlink-1.0.0/src/rtnl.c
	int

dnsmasq /dnsmasq-2.59/contrib/wrt/dhcp_release.c
	size_t (cast from ssize_t)

netplug /netplug-1.2.9.1/netlink.c
	int

oidentd /oidentd-2.0.8/src/kernel/linux.c
	size_t (from ssize_t)

certmonger /certmonger-0.59/src/netlink.c
	size_t (cast from int)

certmonger /certmonger-0.59/src/nl-check.c
	unsigned int (cast from int)

radvd /radvd-1.9.1/netlink.c
	int

ptlib /ptlib-2.10.2/src/ptlib/unix/socket.cxx
	unsigned (cast from int)

acpid /acpid-2.0.16/kacpimon/libnetlink.c
	unsigned (cast from int)

net-snmp /net-snmp-5.7.1/agent/mibgroup/etherlike-mib/data_access/dot3stats_linux.c
	int

net-snmp /net-snmp-5.7.1/agent/mibgroup/if-mib/data_access/interface_linux.c
	int

net-snmp /net-snmp-5.7.1/agent/mibgroup/ip-mib/data_access/arp_netlink.c
	int

net-snmp /net-snmp-5.7.1/agent/mibgroup/mibII/tcpTable.c
	int

fprobe-ulog /fprobe-ulog-1.1/src/libipulog/libipulog.c
	size_t

busybox /busybox-1.19.4/networking/ifplugd.c
	ssize_t

busybox /busybox-1.19.4/networking/libiproute/libnetlink.c
	int

ntp /ntp-4.2.6p5/ntpd/ntp_io.c
	int

Re: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in users' code

[D. Hugh Redelmeier]

Fixes have been proposed for this problem at least twice before.

================

(These messages are not presented as a thread so I've put links to
each of them)

Problem report: <https://lkml.org/lkml/2013/12/15/59>
Patch proposal: <https://lkml.org/lkml/2013/12/15/60>
Reply suggesting improved presentation:  <https://lkml.org/lkml/2013/12/15/62>
Revised patch proposal: <https://lkml.org/lkml/2013/12/15/96>
Duplicate revised patch proposal: <https://lkml.org/lkml/2013/12/15/97>

Doron Tsur proposed:
-			   (nlh)->nlmsg_len <= (len))
+			   (int)(nlh)->nlmsg_len <= (len))

This would correctly function as long as nlmsg_length were <= INT_MAX.
I imagine that this would always be the case since Linux isn't used on
machines with ints narrower than 32 bits.

It would cause a GCC warning on 32-bit machines when len has an
unsigned type that is the same width as int.

Programs conforming the the netlink documentation would not get
warnings.

There was no reply to the revised patch proposal.

================

<https://lkml.org/lkml/2015/3/5/38>
Mike Frysinger proposed:
-#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
+#define NLMSG_OK(nlh,len) ((len) >= sizeof(struct nlmsghdr) && \

This should correctly function as long as len is an unsigned type or
it has a non-negative value.  It won't work correctly if len is size_t
and has the value -1 (the error indicator).

David Miller replied:

	I don't think we can change this.  If you get rid of the 'int'
	cast then code is going to end up with a signed comparison for
	the first test even if 'len' is signed, and that's a potential
	security issue.

I don't understand this response.  If you get rid of the int cast, and
the type of len, after the "integral promotions", is the same width as
size_t, the "usual arithmetic conversions" of the C language will
cause the comparison to be done in unsigned.

I would agree with a variant of the reply:

	I don't think we can change this.  If you get rid of the 'int'
	cast then IN MOST ENVIRONMENTS the code is going to end up
	with an UNSIGNED comparison for the first test even if 'len'
	is signed, and that's a potential security issue.

	Consider the case where len is a signed type with a negative
	value, such as -1, the error indicator described in recv(2).

The example code in netlink(7) about reading netlink message is a good
example of code that would misbehave if the cast were removed.