route(4) manual and sockaddrs; ROUNDUP()

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

route(4) manual and sockaddrs; ROUNDUP()

Vadim Penzin
Greetings,


1. The manual of route(4) explains the structure of its messages thus:

     `Messages are formed by a header followed by a small number of
     sockaddr structures (which are variable length), interpreted by
     position, and delimited by the length entry in the sockaddr.'

(That phrase is quite unfortunate in itself: `sa_len' is supposed to
/determine/ the length of a structure.  To me, the word `delimited'
implies the presence of a memory object between two adjacent
structures, which certainly was not the intent of the author.)

Armed with that knowledge, I wrote a parser the other day.  I very
much enjoyed OpenBSD until my program fatally stumbled on a peculiar
structure that does not fit the above description:

     05 00 00 00 ff 00 00 00

(The value of `sa_len' of that `sockaddr' is nothing that I am
familiar with, the address family is `AF_UNSPEC', and the most
disturbing: `sa_len' does not match the physical size of this
structure. These eight bytes were immediately followed by a perfectly
valid `sockaddr_in'.)

It took me some time to figure out that, apparently, I am looking at
the combined effect of in_socktrim() (see src/sys/netinet/in.c) and
ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of
0xff000000 that was stored as sockaddr_in'.

I believe that manuals should be more merciful.  In the absence of
proper documentation, the part of my program that handles this case
looks like a mysterious ritual; it should not be that way.


2. The definition of ROUNDUP() somewhat surprised me: it silently
handles zero. The macro is local to the compilation unit; it is used
this way:

         if (rtinfo == NULL || (sa = rtinfo->rti_info[i]) == NULL)
                continue;
        rtinfo->rti_addrs |= (1 << i);
        dlen = ROUNDUP(sa->sa_len);

and this way:

        if ((sa = rtinfo->rti_info[i]) == NULL)
                 continue;
        rtinfo->rti_addrs |= (1 << i);
        dlen = ROUNDUP(sa->sa_len);

(In the second case, the programmer does not check `rtinfo' for being
NULL.  I only hope that there exists a good explanation to that.)

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.

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.


Thank you for your time,
-- Vadim

Reply | Threaded
Open this post in threaded view
|

Re: route(4) manual and sockaddrs; ROUNDUP()

Claudio Jeker
On Fri, Apr 26, 2019 at 01:55:52PM +0300, Vadim Penzin wrote:

> Greetings,
>
>
> 1. The manual of route(4) explains the structure of its messages thus:
>
>     `Messages are formed by a header followed by a small number of
>     sockaddr structures (which are variable length), interpreted by
>     position, and delimited by the length entry in the sockaddr.'
>
> (That phrase is quite unfortunate in itself: `sa_len' is supposed to
> /determine/ the length of a structure.  To me, the word `delimited'
> implies the presence of a memory object between two adjacent
> structures, which certainly was not the intent of the author.)

Yes this is not quite right since the lenght is rounded to the next long
word to ensure that structures are properly aligned on strict align
architectures.
 

> Armed with that knowledge, I wrote a parser the other day.  I very
> much enjoyed OpenBSD until my program fatally stumbled on a peculiar
> structure that does not fit the above description:
>
>     05 00 00 00 ff 00 00 00
>
> (The value of `sa_len' of that `sockaddr' is nothing that I am
> familiar with, the address family is `AF_UNSPEC', and the most
> disturbing: `sa_len' does not match the physical size of this
> structure. These eight bytes were immediately followed by a perfectly
> valid `sockaddr_in'.)

This is a probably a netmask which are special in many senses from the old
time of the radix routing tree. For AF_UNSPEC it defines how many bytes
are used to store the netmask. It is wierd I agree.
 
> It took me some time to figure out that, apparently, I am looking at
> the combined effect of in_socktrim() (see src/sys/netinet/in.c) and
> ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of
> 0xff000000 that was stored as sockaddr_in'.
>
> I believe that manuals should be more merciful.  In the absence of
> proper documentation, the part of my program that handles this case looks
> like a mysterious ritual; it should not be that way.
 
Please send suggestions to better documentation to this list.
route(4) is not perfect for sure and could use some work.
 

> 2. The definition of ROUNDUP() somewhat surprised me: it silently handles
> zero. The macro is local to the compilation unit; it is used
> this way:
>
>         if (rtinfo == NULL || (sa = rtinfo->rti_info[i]) == NULL)
>        continue;
> rtinfo->rti_addrs |= (1 << i);
> dlen = ROUNDUP(sa->sa_len);
>
> and this way:
>
> if ((sa = rtinfo->rti_info[i]) == NULL)
>                 continue;
> rtinfo->rti_addrs |= (1 << i);
> dlen = ROUNDUP(sa->sa_len);

What code are you talking about?
 
> (In the second case, the programmer does not check `rtinfo' for being
> NULL.  I only hope that there exists a good explanation to that.)
>
> 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.


--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: route(4) manual and sockaddrs; ROUNDUP()

Vadim Penzin
Well, the manual shall tell the truth, whatever it is:

     Messages are formed by a header followed by a small number of
     sockaddr structures of variable length. The size of every
     sockaddr structure can be computed by rounding the value of the
     `sa_len' field of the current structure up to sizeof(long)
     bytes.

I provided a reference to the code in question in my original message:

> It took me some time to figure out that, apparently, I am looking at
> the combined effect of in_socktrim() (see src/sys/netinet/in.c) and
> ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of
> 0xff000000 that was stored as sockaddr_in'.

The following is a more precise description of its location:

See
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/rtsock.c?annotate=1.285

The macro ROUNDUP is defined on lines 1347-1348
It is also used in the definition of the macro ADVANCE on line 1349
The first direct use of ROUNDUP that I quoted is on line 1431.
The second is on line 1476.

--Vadim

On 4/26/19 6:14 PM, Claudio Jeker wrote:

> On Fri, Apr 26, 2019 at 01:55:52PM +0300, Vadim Penzin wrote:
>> Greetings,
>>
>>
>> 1. The manual of route(4) explains the structure of its messages thus:
>>
>>      `Messages are formed by a header followed by a small number of
>>      sockaddr structures (which are variable length), interpreted by
>>      position, and delimited by the length entry in the sockaddr.'
>>
>> (That phrase is quite unfortunate in itself: `sa_len' is supposed to
>> /determine/ the length of a structure.  To me, the word `delimited'
>> implies the presence of a memory object between two adjacent
>> structures, which certainly was not the intent of the author.)
>
> Yes this is not quite right since the lenght is rounded to the next long
> word to ensure that structures are properly aligned on strict align
> architectures.
>  
>> Armed with that knowledge, I wrote a parser the other day.  I very
>> much enjoyed OpenBSD until my program fatally stumbled on a peculiar
>> structure that does not fit the above description:
>>
>>      05 00 00 00 ff 00 00 00
>>
>> (The value of `sa_len' of that `sockaddr' is nothing that I am
>> familiar with, the address family is `AF_UNSPEC', and the most
>> disturbing: `sa_len' does not match the physical size of this
>> structure. These eight bytes were immediately followed by a perfectly
>> valid `sockaddr_in'.)
>
> This is a probably a netmask which are special in many senses from the old
> time of the radix routing tree. For AF_UNSPEC it defines how many bytes
> are used to store the netmask. It is wierd I agree.
>  
>> It took me some time to figure out that, apparently, I am looking at
>> the combined effect of in_socktrim() (see src/sys/netinet/in.c) and
>> ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of
>> 0xff000000 that was stored as sockaddr_in'.
>>
>> I believe that manuals should be more merciful.  In the absence of
>> proper documentation, the part of my program that handles this case looks
>> like a mysterious ritual; it should not be that way.
>  
> Please send suggestions to better documentation to this list.
> route(4) is not perfect for sure and could use some work.
>  
>> 2. The definition of ROUNDUP() somewhat surprised me: it silently handles
>> zero. The macro is local to the compilation unit; it is used
>> this way:
>>
>>          if (rtinfo == NULL || (sa = rtinfo->rti_info[i]) == NULL)
>>        continue;
>> rtinfo->rti_addrs |= (1 << i);
>> dlen = ROUNDUP(sa->sa_len);
>>
>> and this way:
>>
>> if ((sa = rtinfo->rti_info[i]) == NULL)
>>                  continue;
>> rtinfo->rti_addrs |= (1 << i);
>> dlen = ROUNDUP(sa->sa_len);
>
> What code are you talking about?
>  
>> (In the second case, the programmer does not check `rtinfo' for being
>> NULL.  I only hope that there exists a good explanation to that.)
>>
>> 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.
>
>

Reply | Threaded
Open this post in threaded view
|

Re: route(4) manual and sockaddrs; ROUNDUP()

Vadim Penzin
In reply to this post by Claudio Jeker
>> 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;

Reply | Threaded
Open this post in threaded view
|

Re: route(4) manual and sockaddrs; ROUNDUP()

Claudio Jeker
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.

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: route(4) manual and sockaddrs; ROUNDUP()

Vadim Penzin


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.

Reply | Threaded
Open this post in threaded view
|

Re: route(4) manual and sockaddrs; ROUNDUP()

Claudio Jeker
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

Reply | Threaded
Open this post in threaded view
|

Re: route(4) manual and sockaddrs; ROUNDUP()

Vadim Penzin


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.