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). -- :wq Claudio
