Merge ~sergiodj/ubuntu/+source/bind9:bug1997375-segfault-isc-nm-tcp into ubuntu/+source/bind9:ubuntu/focal-devel

[Sergio Durigan Junior]

This MP fixes the segmentation fault experienced by some users on bug #1997375.

Unfortunately, I wasn't able to reliably reproduce the crash locally. It involves a complex scenario where bind9 is put under stress by having to handle a lot of requests and then there's a race condition on the tcpdns code (which is accessing socket internal fields in an unsafe manner), all of that finally leading to the segmentation fault.

This issue is affecting several community members, and they have been very helpful in determining the right fix. After one of them provided me with a coredump, I was able to inspect it and pinpoint what seemed to be the commit that fixes the problem:

https://gitlab.isc.org/isc-projects/bind9/-/commit/4ea84740e64f44ff1d397f1a317682633f174b0d

I backported it, put the package in a PPA and ask the community to try it out. As you can see in the bug report, I've had 3 people confirming that the segfaults don't happen with the patched bind9 anymore.

I decided to go ahead with the SRU and, again, rely on the community's good will to help me with the test plan.

There's a PPA with the proposed changes here:

https://launchpad.net/~sergiodj/+archive/ubuntu/bind9-bugfix/+packages

autopkgtest results:

Results: (from http://autopkgtest.ubuntu.com/results/autopkgtest-focal-sergiodj-bind9-bugfix/?format=plain)
  bind9 @ amd64:
    04.01.23 20:24:52 Log 🗒️ ✅ Triggers: bind9/1:9.16.1-0ubuntu2.12~ppa1
  bind9 @ arm64:
    04.01.23 20:27:21 Log 🗒️ ✅ Triggers: bind9/1:9.16.1-0ubuntu2.12~ppa1
  bind9 @ armhf:
    04.01.23 19:58:53 Log 🗒️ ✅ Triggers: bind9/1:9.16.1-0ubuntu2.12~ppa1
  bind9 @ ppc64el:
    04.01.23 20:28:12 Log 🗒️ ✅ Triggers: bind9/1:9.16.1-0ubuntu2.12~ppa1
  bind9 @ s390x:
    04.01.23 20:34:05 Log 🗒️ ✅ Triggers: bind9/1:9.16.1-0ubuntu2.12~ppa1

[Andreas Hasenack (ahasenack)]

I'll look at this

[Andreas Hasenack (ahasenack)]

Hi Sergio,

I was following the trails of this bug in upstream gitlab.

The fix you are backporting comes from MR #3721: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/3721/commits

The commit is from June 16th, 2020.

There is this other MR #3726: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/3726/commits#882b07ab55f884856324e81fb113e650836a94d9

It shares the backported commit, but has two others on top:
commit 1cf65cd8829f01cc38f47b1180d7fbe3ab710d35
Author: Witold Kręcicki <email address hidden>
Date: Mon Jun 22 15:46:11 2020 -0700

    Fix a shutdown race in netmgr udp

    We need to mark the socket as inactive early (and synchronously)
    in the stoplistening process; otherwise we might destroy the
    callback argument before we actually stop listening, and call
    the callback on bad memory.

And

commit 3704c4fff2757ade6dda56865aa87935d0c447b9
Author: Evan Hunt <email address hidden>
Date: Sat Jun 20 15:03:05 2020 -0700

    clean up outerhandle when a tcpdns socket is disconnected

    this prevents a crash when some non-netmgr thread, such as a
    recursive lookup, times out after the TCP socket is already
    disconnected.

They also refer to the 1937 Issue (probably because of the shared commit), but also a new one, 1938: https://gitlab.isc.org/isc-projects/bind9/-/issues/1938

Issue 1938 says that because of the crash caused by 1937, bind was being restarted by systemd, and that was also crashing, which became issue 1938. Here is an interesting comment in 1938:

    The server that was repeatedly crashing on restart was eventually restarted successfully
    after stopping client query traffic and letting named load all zones undisturbed before
    allowing client queries again

It *looks* like being inundated with queries (what caused the 1937 crash) while starting up or recovering from the crash also causes problems, and this time it was in the new code.

These are complicated scenarios, and backports. I was wondering if you considered the 1938 issue, or if you hadn't seen it. The commits fixing 1938 are a few days later than the 1937 ones, in the git log.

review: Needs Information

[Sergio Durigan Junior (sergiodj)]

Thanks for the review, Andreas.

I hadn't noticed MR #3726; interesting. It does seem to be a more complete fix for a more generic bug, but it's also a bit more complex and, as such, introduces more possibilities for regressions.

I'm now wondering whether it makes sense to bite the bullet and backport everything, or keep the fix more "contained" and just address the issue raised in our bug report. Something to be noted is that at least 3 people tested the changes I'm proposing here and so far haven't experienced negative side effects.

As a member of the SRU team, what's your opinion here? I'm inclined to be more conservative and not backport the other commits from MR #3726, but I'm open to suggestions.

[Andreas Hasenack (ahasenack)]

As a member of the SRU team, I normally look ahead in upstream commits whenever a big chunk of C code is refactoring something or introducing new functions, and this is such a case, which led me to find what I did. I don't know what it would look like when backported, and whether it's worth the risk. That's part of the risk analysis of the SRU. At a glance, it looks like whatever flood of queries that is causing the crash keeps incoming while systemd is restarting bind, and that is what causes the second crash.

If we start noticing the second crash after the SRU is released, then the second fix can still be applied I suppose. I would just confirm with those 3 people who tested the ppa package if they had the bind9 service unit configured in such a way as to restart bind whenever it crashed, I don't know if that's our default or not.

[Sergio Durigan Junior (sergiodj)]

OK, back to this MP.

I talked to Andreas yesterday and decided to leave a comment upstream asking for their input regarding the two MRs. I've come to believe that we really just need MR 3721 (which is the one I backported here), because it is the upstream backport of MR 3726 (as can be seen in this comment: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/3721#note_143992). In fact, you can confirm that MR 3721 targets the v9_16 branch, while MR 3726 targets the main branch. But in any case, I think it's a good idea to confirm with them.

I won't bother getting in touch with the folks who have been helping me with testing this package in Ubuntu for now. Hopefully upstream's answer will be enough to get us to move forward here. If they say that I should backport more commits, then I'll prepare another package, put it in the PPA and ask for more testing. Otherwise, I'll ask Andreas to finish his review of this MP.

[Sergio Durigan Junior (sergiodj)]

Upstream's reply wasn't really encouraging:

https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/3721#note_345089

While I agree with Ondřej's opinion, I also believe that we have to work with what's available for now. Hopefully Lena's effort to get an MRE for bind9 will bear fruit and we'll be able to have the proper, fuller fix for this problem shipped to our users, but meanwhile I believe it's justifiable to proceed with this MP as is.

Andreas, if you're OK with it I'd like to take my chances and upload this change to Focal.

[Andreas Hasenack (ahasenack)]

Ok, +1. Tiny optional nit, the DEP3 headers are missing Last-Update and Applied-Upstream. There is also that inline comment about perhaps changing the bug url to https://gitlab.isc.org/isc-projects/bind9/-/issues/1937, I don't know if you saw that, but it's also a nit.

review: Approve

[git-ubuntu bot (git-ubuntu-bot)]

Approvers: sergiodj, ahasenack
Uploaders: sergiodj, ahasenack
MP auto-approved

review: Approve

[Sergio Durigan Junior (sergiodj)]

On Thursday, January 19 2023, Andreas Hasenack wrote:

> Ok, +1. Tiny optional nit, the DEP3 headers are missing Last-Update
> and Applied-Upstream. There is also that inline comment about perhaps
> changing the bug url to
> https://gitlab.isc.org/isc-projects/bind9/-/issues/1937, I don't know
> if you saw that, but it's also a nit.

Thanks, Andreas.

I've addressed the points above and uploaded the package:

$ dput bind9_9.16.1-0ubuntu2.12_source.changes
Trying to upload package to ubuntu
Checking signature on .changes
gpg: /home/sergio/work/bind9/bind9_9.16.1-0ubuntu2.12_source.changes: Valid signature from 106DA1C8C3CBBF14
Checking signature on .dsc
gpg: /home/sergio/work/bind9/bind9_9.16.1-0ubuntu2.12.dsc: Valid signature from 106DA1C8C3CBBF14
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading bind9_9.16.1-0ubuntu2.12.dsc: done.
  Uploading bind9_9.16.1-0ubuntu2.12.debian.tar.xz: done.
  Uploading bind9_9.16.1-0ubuntu2.12_source.buildinfo: done.
  Uploading bind9_9.16.1-0ubuntu2.12_source.changes: done.
Successfully uploaded packages.

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

diff --git a/debian/changelog b/debian/changelog
index 277cb02..b487525 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+bind9 (1:9.16.1-0ubuntu2.13) focal; urgency=medium
+
+  * d/p/lp1997375-segfault-isc-nm-tcp-send.patch: Fix segfault on
+    isc__nm_tcpdns_send by moving the tcpdns processing to another
+    thread. (LP: #1997375)
+
+ -- Sergio Durigan Junior <[e-mail]>  Thu, 02 Feb 2023 13:38:24 -0500
+
 bind9 (1:9.16.1-0ubuntu2.12) focal-security; urgency=medium
 
   * SECURITY UPDATE: An UPDATE message flood may cause named to exhaust all
diff --git a/debian/patches/lp1997375-segfault-isc-nm-tcp-send.patch b/debian/patches/lp1997375-segfault-isc-nm-tcp-send.patch
new file mode 100644
index 0000000..901bb43
--- /dev/null
+++ b/debian/patches/lp1997375-segfault-isc-nm-tcp-send.patch
@@ -0,0 +1,207 @@
+From: =?utf-8?q?Witold_Kr=C4=99cicki?= <[e-mail]>
+Date: Tue, 16 Jun 2020 19:56:12 +0200
+Subject: Make netmgr tcpdns send calls asynchronous.
+
+isc__nm_tcpdns_send() was not asynchronous and accessed socket
+internal fields in an unsafe manner, which could lead to a race
+condition and subsequent crash. Fix it by moving the whole tcpdns
+processing to a proper netmgr thread.
+
+Origin: backport, https://gitlab.isc.org/isc-projects/bind9/-/commit/4ea84740e64f44ff1d397f1a317682633f174b0d
+Bug: https://gitlab.isc.org/isc-projects/bind9/-/issues/1937
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/bind9/+bug/1997375
+Reviewed-By: Sergio Durigan Junior <[e-mail]>
+Last-Update: 2023-01-09
+Applied-Upstream: 9.16.5
+
+---
+ lib/isc/netmgr/netmgr-int.h |  5 +++
+ lib/isc/netmgr/netmgr.c     |  3 ++
+ lib/isc/netmgr/tcpdns.c     | 94 +++++++++++++++++++++++++++++----------------
+ 3 files changed, 68 insertions(+), 34 deletions(-)
+
+diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
+index 86b7015..0323b97 100644
+--- a/lib/isc/netmgr/netmgr-int.h
++++ b/lib/isc/netmgr/netmgr-int.h
+@@ -138,6 +138,7 @@ typedef enum isc__netievent_type {
+ 	netievent_tcpstop,
+ 	netievent_tcpclose,
+ 	netievent_tcpdnsclose,
++	netievent_tcpdnssend,
+ 	netievent_prio = 0xff, /* event type values higher than this
+ 				* will be treated as high-priority
+ 				* events, which can be processed
+@@ -225,6 +226,7 @@ typedef struct isc__netievent__socket_req {
+ typedef isc__netievent__socket_req_t isc__netievent_tcpconnect_t;
+ typedef isc__netievent__socket_req_t isc__netievent_tcplisten_t;
+ typedef isc__netievent__socket_req_t isc__netievent_tcpsend_t;
++typedef isc__netievent__socket_req_t isc__netievent_tcpdnssend_t;
+ 
+ typedef struct isc__netievent__socket_streaminfo {
+ 	isc__netievent_type type;
+@@ -705,6 +707,9 @@ isc__nm_tcpdns_close(isc_nmsocket_t *sock);
+ void
+ isc__nm_async_tcpdnsclose(isc__networker_t *worker, isc__netievent_t *ev0);
+ 
++void
++isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0);
++
+ #define isc__nm_uverr2result(x) \
+ 	isc___nm_uverr2result(x, true, __FILE__, __LINE__)
+ isc_result_t
+diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
+index e2cb89f..d5799c8 100644
+--- a/lib/isc/netmgr/netmgr.c
++++ b/lib/isc/netmgr/netmgr.c
+@@ -615,6 +615,9 @@ process_queue(isc__networker_t *worker, isc_queue_t *queue) {
+ 		case netievent_tcpsend:
+ 			isc__nm_async_tcpsend(worker, ievent);
+ 			break;
++		case netievent_tcpdnssend:
++			isc__nm_async_tcpdnssend(worker, ievent);
++			break;
+ 		case netievent_tcpstop:
+ 			isc__nm_async_tcpstop(worker, ievent);
+ 			break;
+diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
+index 11d3385..03bb9ce 100644
+--- a/lib/isc/netmgr/tcpdns.c
++++ b/lib/isc/netmgr/tcpdns.c
+@@ -365,15 +365,6 @@ isc_nm_tcpdns_keepalive(isc_nmhandle_t *handle) {
+ 	atomic_store(&handle->sock->outer->keepalive, true);
+ }
+ 
+-typedef struct tcpsend {
+-	isc_mem_t *mctx;
+-	isc_nmhandle_t *handle;
+-	isc_region_t region;
+-	isc_nmhandle_t *orighandle;
+-	isc_nm_cb_t cb;
+-	void *cbarg;
+-} tcpsend_t;
+-
+ static void
+ resume_processing(void *arg) {
+ 	isc_nmsocket_t *sock = (isc_nmsocket_t *)arg;
+@@ -445,15 +436,40 @@ resume_processing(void *arg) {
+ 
+ static void
+ tcpdnssend_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
+-	tcpsend_t *ts = (tcpsend_t *)cbarg;
++	isc__nm_uvreq_t *req = (isc__nm_uvreq_t *)cbarg;
+ 
+ 	UNUSED(handle);
+ 
+-	ts->cb(ts->orighandle, result, ts->cbarg);
+-	isc_mem_put(ts->mctx, ts->region.base, ts->region.length);
++	req->cb.send(req->handle, result, req->cbarg);
++	isc_mem_put(req->sock->mgr->mctx, req->uvbuf.base, req->uvbuf.len);
++	isc__nm_uvreq_put(&req, req->handle->sock);
++}
++
++void
++isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) {
++	isc_result_t result;
++	isc__netievent_tcpdnssend_t *ievent =
++		(isc__netievent_tcpdnssend_t *)ev0;
++	isc__nm_uvreq_t *req = ievent->req;
++	isc_nmsocket_t *sock = ievent->sock;
++
++	REQUIRE(worker->id == sock->tid);
+ 
+-	isc_nmhandle_unref(ts->orighandle);
+-	isc_mem_putanddetach(&ts->mctx, ts, sizeof(*ts));
++	result = ISC_R_NOTCONNECTED;
++	if (atomic_load(&sock->active)) {
++		isc_region_t r;
++
++		r.base = (unsigned char *)req->uvbuf.base;
++		r.length = req->uvbuf.len;
++		result = isc__nm_tcp_send(sock->outer->tcphandle, &r,
++					  tcpdnssend_cb, req);
++	}
++
++	if (result != ISC_R_SUCCESS) {
++		req->cb.send(req->handle, result, req->cbarg);
++		isc_mem_put(sock->mgr->mctx, req->uvbuf.base, req->uvbuf.len);
++		isc__nm_uvreq_put(&req, sock);
++	}
+ }
+ 
+ /*
+@@ -462,7 +478,7 @@ tcpdnssend_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
+ isc_result_t
+ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
+ 		    isc_nm_cb_t cb, void *cbarg) {
+-	tcpsend_t *t = NULL;
++	isc__nm_uvreq_t *uvreq = NULL;
+ 
+ 	REQUIRE(VALID_NMHANDLE(handle));
+ 
+@@ -471,36 +487,46 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
+ 	REQUIRE(VALID_NMSOCK(sock));
+ 	REQUIRE(sock->type == isc_nm_tcpdnssocket);
+ 
+-	if (sock->outer == NULL) {
+-		/* The socket is closed */
+-		return (ISC_R_NOTCONNECTED);
+-	}
++	uvreq = isc__nm_uvreq_get(sock->mgr, sock);
++	uvreq->handle = handle;
++	isc_nmhandle_ref(uvreq->handle);
++	uvreq->cb.send = cb;
++	uvreq->cbarg = cbarg;
+ 
+-	t = isc_mem_get(sock->mgr->mctx, sizeof(*t));
+-	*t = (tcpsend_t){
+-		.cb = cb,
+-		.cbarg = cbarg,
+-		.handle = handle->sock->outer->tcphandle,
+-	};
++	uvreq->uvbuf.base = isc_mem_get(sock->mgr->mctx, region->length + 2);
++	uvreq->uvbuf.len = region->length + 2;
++	*(uint16_t *)uvreq->uvbuf.base = htons(region->length);
++	memmove(uvreq->uvbuf.base + 2, region->base, region->length);
+ 
+-	isc_mem_attach(sock->mgr->mctx, &t->mctx);
+-	t->orighandle = handle;
+-	isc_nmhandle_ref(t->orighandle);
++	if (sock->tid == isc_nm_tid()) {
++		isc_region_t r;
++
++		r.base = (unsigned char *)uvreq->uvbuf.base;
++		r.length = uvreq->uvbuf.len;
+ 
+-	t->region = (isc_region_t){ .base = isc_mem_get(t->mctx,
+-							region->length + 2),
+-				    .length = region->length + 2 };
++		return (isc__nm_tcp_send(sock->outer->tcphandle, &r,
++					 tcpdnssend_cb, uvreq));
++	} else {
++		isc__netievent_tcpdnssend_t *ievent = NULL;
+ 
+-	*(uint16_t *)t->region.base = htons(region->length);
+-	memmove(t->region.base + 2, region->base, region->length);
++		ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpdnssend);
++		ievent->req = uvreq;
++		ievent->sock = sock;
+ 
+-	return (isc__nm_tcp_send(t->handle, &t->region, tcpdnssend_cb, t));
++		isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
++				       (isc__netievent_t *)ievent);
++
++		return (ISC_R_SUCCESS);
++	}
++
++	return (ISC_R_UNEXPECTED);
+ }
+ 
+ static void
+ tcpdns_close_direct(isc_nmsocket_t *sock) {
+ 	REQUIRE(sock->tid == isc_nm_tid());
+ 	/* We don't need atomics here, it's all in single network thread */
++
+ 	if (sock->timer_initialized) {
+ 		/*
+ 		 * We need to fire the timer callback to clean it up,
diff --git a/debian/patches/series b/debian/patches/series
index c1e1ee1..965a609 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -21,3 +21,4 @@ CVE-2022-2795.patch
 CVE-2022-38177.patch
 CVE-2022-38178.patch
 CVE-2022-3094.patch
+lp1997375-segfault-isc-nm-tcp-send.patch