[PATCH] 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:

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

On 10/24/2015 06:22 AM, Mike Frysinger wrote:

On 23 Oct 2015 22:07, Florian Weimer wrote:

quoted

--- /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

Added.

quoted

+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 ...

Thanks.  I added static asserts to make sure that the actual type does
not cause problems with the use of -1 and an int return value.

I do not want to use SO_DOMAIN here because I expect this to eventually
move into generic code because we probably should do similar checking on
other internally-used sockets (where reporting impossible errors to the
caller would be grossly misleading).

I'm still waiting for comments from the kernel people. :)

Florian

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:

quoted

(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 <redacted>
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:

quoted

Hello,

On Fri, Oct 23, 2015, at 21:07, Florian Weimer wrote:

quoted

(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 <redacted>

Bye,
Hannes