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: 
Patch proposal: 
Reply suggesting improved presentation:  
Revised patch proposal: 
Duplicate revised patch proposal: 

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.




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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[D. Hugh Redelmeier]

| From: David Miller 

| From: "D. Hugh Redelmeier" 
| 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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[D. Hugh Redelmeier]

| From: David Miller 

| 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 

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

[D. Hugh Redelmeier]

| From: David Laight 

| 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Miller]

From: "D. Hugh Redelmeier" 
Date: Wed, 9 Sep 2015 16:24:07 -0400 (EDT)

> | From: David Miller 
> 
> | From: "D. Hugh Redelmeier" 
> | 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[D. Hugh Redelmeier]

| From: David Miller 

| From: "D. Hugh Redelmeier" 
| 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Miller]

From: "D. Hugh Redelmeier" 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html