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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LRH.2.02.1509110845151.4754@redclaw.mimosa.com>
Date:	Fri, 11 Sep 2015 13:11:26 -0400 (EDT)
From:	"D. Hugh Redelmeier" <hugh@...osa.com>
To:	David Laight <David.Laight@...LAB.COM>
cc:	David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: PATCH: netdev: add a cast NLMSG_OK to avoid a GCC warning in
 users' code

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