On 7/15/19 12:45 PM, Claudio Jeker wrote:
On Mon, Jul 15, 2019 at 11:08:53AM +0300, Vadim Penzin wrote:
On 7/15/19 10:28 AM, Claudio Jeker wrote:
On Sun, Jul 14, 2019 at 01:15:40PM +0300, Vadim Penzin wrote:
Since `sa_len' describes the size of a `sockaddr' (or one of its
derivatives) /including/ `sa_len' (maybe I am wrong, but this is my
interpretation of the comment `total length' that appears near the
definition of `struct sockaddr' in <sys/socket.h>), `sa_len' just
cannot be zero.
Yes, it can not be zero.
The current definition of ROUNDUP() might hide a bug. In addition, on
some machines, it disturbs the pipeline of the CPU by introducing a
branch (for no real reason, as it seems, while I might be
nitpicking). At very least, it looks confusing.
The following patch amends the issue:
Index: route.c
===================================================================
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.387
diff -u -p -r1.387 route.c
--- route.c 24 Jun 2019 22:26:25 -0000 1.387
+++ route.c 14 Jul 2019 10:05:04 -0000
@@ -138,7 +138,7 @@
#include <net/bfd.h>
#endif
-#define ROUNDUP(a) (a>0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) :
sizeof(long))
+#define ROUNDUP(a) (1 + (((a) - 1) | (sizeof(long) - 1)))
/* Give some jitter to hash, to avoid synchronization between routers. */
static uint32_t rt_hashjitter;
This only touches only one version of ROUNDUP() and looking at how ROUNDUP()
is used in route.c I wonder if it is actually needed at all.
Looking at the code in route.c and rtsock.c I think it is better to leave
this like it is or fix both files making sure that the necessary checks
are put in place to make sure that a sa_len of 0 can't get into the system.
If you wish to remove ROUNDUP() then fine, however, leaving it as is does
not help.
Imagine the behaviour of the original ROUNDUP() in the unlikely event when
the kernel does somehow produce a sockaddr with sa_len of zero. The kernel
will send an unvalid sockaddr to the user. ROUNDUP() alone will not help
detection or correction of that fault.
Now imagine the behaviour of the new version. The kernel will not send an
incorrect sockaddr to the user (ROUNDUP() returns zero) but a sockaddr will
be missing entirely.
What is better: having something unusable and misleading or not having it
(with a good way of knowing that it is missing)? That's philosophy to me.
In any case, ROUNDUP() cannot magically help when sa_len is zero.
Some time ago there was an infinite loop because of a 0 length advance and
so the parser code never moved over the data. At least this does not
happen with a ROUNDUP() that ensures that at least sizeof(long) data is
consumed on every call.
Keep in mind userland can send in any kind of crap and syzkaller told me
that the rtsock code is way to trusting. This is why I think this is
incomplete. The ROUNDUP() version in rtsock.c and route.c should be kept
in sync (and maybe moved to a .h file).
Unless I am missing something, ROUTE() is not used directly for parsing
user data. ADVANCE() does `call' ROUTE() for parsing, and it would look
more natural if ADVANCE() were checking that it, well, advances :-)
I think that explicit consistency checks and aborting calls early is a
better strategy than relying on ROUNDUP()'s sweeping the garbage under
the rug.
At any rate, providing a user-visible macro that reports the size of
memory that is allocated for a sockaddr is a much-needed companion to
route(4). Digging knowledge up from the kernel sources should not be a
strict condition for application development.
