glibc: Terminate process on invalid netlink response from kernel [BZ #12926]

  This patch revisits this glibc bug:

  https://sourceware.org/bugzilla/show_bug.cgi?id=12926

For some reason, this particular code path is very good at picking up
file descriptors which have been reused in correctly.  This happens if
other threads have a race, close the wrong file descriptor (the one used
in the glibc netlink code), and reopen another one in its place.

The netlink requests we send to the kernel are:

  struct req
  {
    struct nlmsghdr nlh;
    struct rtgenmsg g;
    /* struct rtgenmsg consists of a single byte.  This means there
       are three bytes of padding included in the REQ definition.
       We make them explicit here.  */
    char pad[3];
  } req;

  req.nlh.nlmsg_len = sizeof (req);
  req.nlh.nlmsg_type = RTM_GETADDR;
  req.nlh.nlmsg_flags = NLM_F_ROOT | NLM_F_MATCH | NLM_F_REQUEST;
  req.nlh.nlmsg_pid = 0;
  req.nlh.nlmsg_seq = time (NULL);
  req.g.rtgen_family = AF_UNSPEC;

  req.nlh.nlmsg_len = sizeof (req);
  req.nlh.nlmsg_type = RTM_GETLINK;
  req.nlh.nlmsg_flags = NLM_F_ROOT | NLM_F_MATCH | NLM_F_REQUEST;
  req.nlh.nlmsg_pid = 0;
  req.nlh.nlmsg_seq = time (NULL);
  req.g.rtgen_family = AF_UNSPEC;

I discussed this with Hannes and he thinks that a zero-length reply (as
received by recvmsg) is impossible at this point, for these specific
types of netlink requests.  The new assert triggers for zero-length
replies, but also for replies less than sizeof (struct nlmsghdr) bytes
long, and for unexpected errors (EBADF, ENOTSOCK, ENOTCONN,
ECONNREFUSED, and EAGAIN on a non-blocking sockets—ours are all blocking).

This is purely a defense against silent data corruption and bug reports
incorrectly blaming glibc (or the wrong part of glibc at least).  I
added it to all three copies of the netlink code in glibc.

The glibc netlink code is still broken: It does not time out and retry
(needed in case the request gets lots), does not handle NLM_F_DUMP_INTR,
and does not deal with NLMSG_ERROR and ENOBUFS.  But these are separate
issues.  SOCK_CLOEXEC is not used, either. If we fix those issues, the
assert would remain in place, except for the EAGAIN part.

(By the way, we'd also love to have a better kernel interface to fulfill
the needs for getaddrinfo address sorting.  The netlink requests we
currently use are much too slow if the host has many addresses configured.)

I have tested that basic getaddrinfo operations still work after the
patch, but glibc testsuite coverage in this area is very limited, and I
have yet to do full-system testing with this patch.

Florian
  

On 23 Oct 2015 22:07, Florian Weimer wrote:
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/netlink_assert_response.c
> @@ -0,0 +1,100 @@
> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.

guess we like to have the first line be a short desc of the file

> +static int
> +get_address_family (int fd)
> +{
> ...
> +  return sa.ss_family;

ss_family is of type sa_family_t, not int ... not a big deal, but the
two do differ in sign ...
-mike
  

Hello,

On Sat, Oct 24, 2015, at 06:22, Mike Frysinger wrote:
> On 23 Oct 2015 22:07, Florian Weimer wrote:
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/netlink_assert_response.c
> > @@ -0,0 +1,100 @@
> > +/* Copyright (C) 2015 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> 
> guess we like to have the first line be a short desc of the file
> 
> > +static int
> > +get_address_family (int fd)
> > +{
> > ...
> > +  return sa.ss_family;
> 
> ss_family is of type sa_family_t, not int ... not a big deal, but the
> two do differ in sign ...

Another option is to simply use a setsockopt SO_DOMAIN to retrieve the
socket family. Does not need 128 bytes on the stack, but that shouldn't
be really a problem.

Bye,
Hannes
  

Hello,

On Fri, Oct 23, 2015, at 21:07, Florian Weimer wrote:
> (By the way, we'd also love to have a better kernel interface to fulfill
> the needs for getaddrinfo address sorting.  The netlink requests we
> currently use are much too slow if the host has many addresses
> configured.)

One solution would be to finish the IPv6 ioctl interface to list
addresses. The ioctl interface would need less memory allocations and is
a synchronous interface which would make it much more easier for glibc
to deal with. No timeouts and retries like with netlink are necessary.

David, would such code still be accepted?

Thanks,
Hannes
  

On 11/03/2015 02:48 PM, Hannes Frederic Sowa wrote:
> Hello,
> 
> On Fri, Oct 23, 2015, at 21:07, Florian Weimer wrote:
>> (By the way, we'd also love to have a better kernel interface to fulfill
>> the needs for getaddrinfo address sorting.  The netlink requests we
>> currently use are much too slow if the host has many addresses
>> configured.)
> 
> One solution would be to finish the IPv6 ioctl interface to list
> addresses. The ioctl interface would need less memory allocations and is
> a synchronous interface which would make it much more easier for glibc
> to deal with. No timeouts and retries like with netlink are necessary.

The more fundamental question is whether we actually have to copy all
the addresses to userspace.  In the end, it may be better to hand a list
of destination addresses to the kernel and have it sort them according
to some algorithm.  But for the algorithm proposed in RFC 6724 section
6, this may be not worth the effort because there are so many
configurable bits.

I still think most of the address sorting is bogus because it appears to
make guarantees which can break after renumbering.

Florian
  

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Tue, 03 Nov 2015 14:48:12 +0100

> David, would such code still be accepted?

I would rather see the netlink interface become more usable, so my
answer is no.
  

Hello,

On Tue, Nov 3, 2015, at 15:00, Florian Weimer wrote:
> On 11/03/2015 02:48 PM, Hannes Frederic Sowa wrote:
> > Hello,
> > 
> > On Fri, Oct 23, 2015, at 21:07, Florian Weimer wrote:
> >> (By the way, we'd also love to have a better kernel interface to fulfill
> >> the needs for getaddrinfo address sorting.  The netlink requests we
> >> currently use are much too slow if the host has many addresses
> >> configured.)
> > 
> > One solution would be to finish the IPv6 ioctl interface to list
> > addresses. The ioctl interface would need less memory allocations and is
> > a synchronous interface which would make it much more easier for glibc
> > to deal with. No timeouts and retries like with netlink are necessary.
> 
> The more fundamental question is whether we actually have to copy all
> the addresses to userspace.  In the end, it may be better to hand a list
> of destination addresses to the kernel and have it sort them according
> to some algorithm.  But for the algorithm proposed in RFC 6724 section
> 6, this may be not worth the effort because there are so many
> configurable bits.

I would rather not provide a holistic sort function in the kernel for
both IPv4 and IPv6 which would a requirement by glibc, no?

> I still think most of the address sorting is bogus because it appears to
> make guarantees which can break after renumbering.

Yes, of course.

Your patch for glibc looks fine to me, so

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Bye,
Hannes
  

On 11/05/2015 03:36 PM, Hannes Frederic Sowa wrote:

>> The more fundamental question is whether we actually have to copy all
>> the addresses to userspace.  In the end, it may be better to hand a list
>> of destination addresses to the kernel and have it sort them according
>> to some algorithm.  But for the algorithm proposed in RFC 6724 section
>> 6, this may be not worth the effort because there are so many
>> configurable bits.
> 
> I would rather not provide a holistic sort function in the kernel for
> both IPv4 and IPv6 which would a requirement by glibc, no?

That depends on what we are trying to solve, and what the desired
semantics.  RFC 6724 probably does not real -world requirements or what
any system does, unfortunately.  Our situation is also different from
other designs because glibc has at most a single-process view, and for
dead address detection, you'd want a whole-system view (or at least
whole-user view, to avoid local information disclosure).

If we get rid of longest prefix matching, I am not sure how much sorting
actually needs kernel assistance.  We still need to know if there is
IPv4 or IPv6 support, but maybe it's better to perform a routing table
lookup and drop addresses which are known to be unreachable from a host
perspective.

>> I still think most of the address sorting is bogus because it appears to
>> make guarantees which can break after renumbering.
> 
> Yes, of course.
> 
> Your patch for glibc looks fine to me, so
> 
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Thanks, I have committed this now.  I hope this is okay—you have
reviewed the kernel aspect, and Mike looked at things from a glibc
perspective.

Florian