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

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

The recvmsg system calls for netlink sockets have been particularly prone to picking up unrelated data after a file descriptor race (where the descriptor is closed and reopened concurrently in a multi-threaded process, as the result of a file descriptor management issue elsewhere). This commit adds additional error checking and aborts the process if a datagram of unexpected length (without the netlink header) is received, or an error code which cannot happen due to the way the netlink socket is used.

2015-10-23 Florian Weimer <[email protected]>

[BZ #12926]
Terminate process on invalid netlink response.
* sysdeps/unix/sysv/linux/netlinkaccess.h (__netlink_assert_response): Declare.
* sysdeps/unix/sysv/linux/netlink_assert_response.c: New file.
* sysdeps/unix/sysv/linux/Makefile [$(subdir) == inet] (sysdep_routines): Add netlink_assert_response.
* sysdeps/unix/sysv/linux/check_native.c (__check_native): Call __netlink_assert_response.
* sysdeps/unix/sysv/linux/check_pf.c (make_request): Likewise.
* sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_request): Likewise.
* sysdeps/unix/sysv/linux/Versions (GLIBC_PRIVATE): Add __netlink_assert_response.

index 2c67a66..d6cc529 100644
@@ -151,6 +151,7 @@ sysdep_headers += netinet/if_fddi.h netinet/if_tr.h \
 		  netipx/ipx.h netash/ash.h netax25/ax25.h netatalk/at.h \
 		  netrom/netrom.h netpacket/packet.h netrose/rose.h \
 		  neteconet/ec.h netiucv/iucv.h
+sysdep_routines += netlink_assert_response
 endif
 
 # Don't compile the ctype glue code, since there is no old non-GNU C library.

index 16bb281..202ffcc 100644
@@ -169,5 +169,7 @@ libc {
   GLIBC_PRIVATE {
     # functions used in other libraries
     __syscall_rt_sigqueueinfo;
+    # functions used by nscd
+    __netlink_assert_response;
   }
 }

index eaefca1..d04c8f2 100644
@@ -35,6 +35,7 @@
 
 #include <not-cancel.h>
 
+#include "netlinkaccess.h"
 
 void
 __check_native (uint32_t a1_index, int *a1_native,

@@ -117,6 +118,7 @@ __check_native (uint32_t a1_index, int *a1_native,
 	};
 
       ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0));
+      __netlink_assert_response (fd, read_len);
       if (read_len < 0)
 	goto out_fail;
 

index f072fb3..af4fdf8 100644
@@ -36,6 +36,7 @@
 #include <atomic.h>
 #include <nscd/nscd-client.h>
 
+#include "netlinkaccess.h"
 
 #ifndef IFA_F_HOMEADDRESS
 # define IFA_F_HOMEADDRESS 0

@@ -164,7 +165,8 @@ make_request (int fd, pid_t pid)
 	};
 
       ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0));
-      if (read_len <= 0)
+      __netlink_assert_response (fd, read_len);
+      if (read_len < 0)
 	goto out_fail;
 
       if (msg.msg_flags & MSG_TRUNC)

index 64b4a1c..768a7ed 100644
@@ -168,6 +168,7 @@ __netlink_request (struct netlink_handle *h, int type)
 	};
 
       read_len = TEMP_FAILURE_RETRY (__recvmsg (h->fd, &msg, 0));
+      __netlink_assert_response (h->fd, read_len);
       if (read_len < 0)
 	goto out_fail;
 

index c204b67..01ac35c 100644
@@ -19,6 +19,7 @@
 #define _NETLINKACCESS_H 1
 
 #include <stdint.h>
+#include <sys/types.h>
 #include <asm/types.h>
 #include <linux/netlink.h>
 #include <linux/rtnetlink.h>

@@ -48,5 +49,10 @@ extern void __netlink_close (struct netlink_handle *h);
 extern void __netlink_free_handle (struct netlink_handle *h);
 extern int __netlink_request (struct netlink_handle *h, int type);
 
+/* Terminate the process if RESULT is an invalid recvmsg result for
+   the netlink socket FD.  */
+void __netlink_assert_response (int fd, ssize_t result)
+  internal_function;
+libc_hidden_proto (__netlink_assert_response)
 
 #endif /* netlinkaccess.h */

Added.

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

Terminate process on invalid netlink response from kernel [BZ

The recvmsg system calls for netlink sockets have been particularly prone to picking up unrelated data after a file descriptor race (where the descriptor is closed and reopened concurrently in a multi-threaded process, as the result of a file descriptor management issue elsewhere). This commit adds additional error checking and aborts the process if a datagram of unexpected length (without the netlink header) is received, or an error code which cannot happen due to the way the netlink socket is used.

2015-11-03 Florian Weimer <[email protected]>

[BZ #12926]
Terminate process on invalid netlink response.
* sysdeps/unix/sysv/linux/netlinkaccess.h (__netlink_assert_response): Declare.
* sysdeps/unix/sysv/linux/netlink_assert_response.c: New file.
* sysdeps/unix/sysv/linux/Makefile [$(subdir) == inet] (sysdep_routines): Add netlink_assert_response.
* sysdeps/unix/sysv/linux/check_native.c (__check_native): Call __netlink_assert_response.
* sysdeps/unix/sysv/linux/check_pf.c (make_request): Likewise.
* sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_request): Likewise.
* sysdeps/unix/sysv/linux/Versions (GLIBC_PRIVATE): Add __netlink_assert_response.

index ab7aa69..0b8ed92 100644
@@ -1,3 +1,19 @@
+2015-11-03  Florian Weimer  <[email protected]>
+
+	[BZ #12926]
+	Terminate process on invalid netlink response.
+	* sysdeps/unix/sysv/linux/netlinkaccess.h
+	(__netlink_assert_response): Declare.
+	* sysdeps/unix/sysv/linux/netlink_assert_response.c: New file.
+	* sysdeps/unix/sysv/linux/Makefile [$(subdir) == inet]
+	(sysdep_routines): Add netlink_assert_response.
+	* sysdeps/unix/sysv/linux/check_native.c (__check_native): Call
+	__netlink_assert_response.
+	* sysdeps/unix/sysv/linux/check_pf.c (make_request): Likewise.
+	* sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_request): Likewise.
+	* sysdeps/unix/sysv/linux/Versions (GLIBC_PRIVATE): Add
+	__netlink_assert_response.
+
 2015-11-02  Joseph Myers  <[email protected]>
 
 	* math/libm-test.inc (modf_test_data): Add more tests.

index 896eb02..d3738df 100644
@@ -24,6 +24,14 @@ Version 2.23
   19095, 19124, 19125, 19129, 19134, 19137, 19156, 19174, 19181, 19189,
   19201.
 
+* getaddrinfo now detects certain invalid responses on an internal netlink
+  socket.  If such responses are received, an affected process will
+  terminate with an error message of "Unexpected error <number> on netlink
+  descriptor <number>" or "Unexpected netlink response of size <number> on
+  descriptor <number>".  The most likely cause for these errors is a
+  multi-threaded application which erroneously closes and reuses the netlink
+  file descriptor while it is used by getaddrinfo.
+
 * A defect in the malloc implementation, present since glibc 2.15 (2012) or
   glibc 2.10 via --enable-experimental-malloc (2009), could result in the
   unnecessary serialization of memory allocation requests across threads.

index 2c67a66..d6cc529 100644
@@ -151,6 +151,7 @@ sysdep_headers += netinet/if_fddi.h netinet/if_tr.h \
 		  netipx/ipx.h netash/ash.h netax25/ax25.h netatalk/at.h \
 		  netrom/netrom.h netpacket/packet.h netrose/rose.h \
 		  neteconet/ec.h netiucv/iucv.h
+sysdep_routines += netlink_assert_response
 endif
 
 # Don't compile the ctype glue code, since there is no old non-GNU C library.

index 16bb281..202ffcc 100644
@@ -169,5 +169,7 @@ libc {
   GLIBC_PRIVATE {
     # functions used in other libraries
     __syscall_rt_sigqueueinfo;
+    # functions used by nscd
+    __netlink_assert_response;
   }
 }

index eaefca1..d04c8f2 100644
@@ -35,6 +35,7 @@
 
 #include <not-cancel.h>
 
+#include "netlinkaccess.h"
 
 void
 __check_native (uint32_t a1_index, int *a1_native,

@@ -117,6 +118,7 @@ __check_native (uint32_t a1_index, int *a1_native,
 	};
 
       ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0));
+      __netlink_assert_response (fd, read_len);
       if (read_len < 0)
 	goto out_fail;
 

index f072fb3..af4fdf8 100644
@@ -36,6 +36,7 @@
 #include <atomic.h>
 #include <nscd/nscd-client.h>
 
+#include "netlinkaccess.h"
 
 #ifndef IFA_F_HOMEADDRESS
 # define IFA_F_HOMEADDRESS 0

@@ -164,7 +165,8 @@ make_request (int fd, pid_t pid)
 	};
 
       ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0));
-      if (read_len <= 0)
+      __netlink_assert_response (fd, read_len);
+      if (read_len < 0)
 	goto out_fail;
 
       if (msg.msg_flags & MSG_TRUNC)

index 64b4a1c..768a7ed 100644
@@ -168,6 +168,7 @@ __netlink_request (struct netlink_handle *h, int type)
 	};
 
       read_len = TEMP_FAILURE_RETRY (__recvmsg (h->fd, &msg, 0));
+      __netlink_assert_response (h->fd, read_len);
       if (read_len < 0)
 	goto out_fail;
 

index c204b67..01ac35c 100644
@@ -19,6 +19,7 @@
 #define _NETLINKACCESS_H 1
 
 #include <stdint.h>
+#include <sys/types.h>
 #include <asm/types.h>
 #include <linux/netlink.h>
 #include <linux/rtnetlink.h>

@@ -48,5 +49,10 @@ extern void __netlink_close (struct netlink_handle *h);
 extern void __netlink_free_handle (struct netlink_handle *h);
 extern int __netlink_request (struct netlink_handle *h, int type);
 
+/* Terminate the process if RESULT is an invalid recvmsg result for
+   the netlink socket FD.  */
+void __netlink_assert_response (int fd, ssize_t result)
+  internal_function;
+libc_hidden_proto (__netlink_assert_response)
 
 #endif /* netlinkaccess.h */

2.4.3

Hello,

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

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

Hello,

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

Yes, of course.

Your patch for glibc looks fine to me, so

Bye,
Hannes

From: Hannes Frederic Sowa <[email protected]>
Date: Tue, 03 Nov 2015 14:48:12 +0100

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