From 6505056267dffada1d0fa326f318b241f4b64747 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 8 Jul 2022 11:26:34 +0200 Subject: [PATCH] Fix fetch context use-after-free bugs fctx_decreference() may call fctx_destroy(), which in turn may free the fetch context by calling isc_mem_putanddetach(). This means that whenever fctx_decreference() is called, the fetch context pointer should be assumed to point to garbage after that call. Meanwhile, the following pattern is used in several places in lib/dns/resolver.c: LOCK(&res->buckets[fctx->bucketnum].lock); bucket_empty = fctx_decreference(fctx); UNLOCK(&res->buckets[fctx->bucketnum].lock); Given that 'fctx' may be freed by the fctx_decreference() call, there is no guarantee that the value of fctx->bucketnum will be the same before and after the fctx_decreference() call. This can cause all kinds of locking issues as LOCK() calls no longer match up with their UNLOCK() counterparts. Fix by always using a helper variable to hold the bucket number when the pattern above is used. Note that fctx_try() still uses 'fctx' after calling fctx_decreference() (it calls fctx_done()). This is safe to do because the reference count for 'fctx' is increased a few lines earlier and it also cannot be zero right before that increase happens, so the fctx_decreference() call in that particular location never invokes fctx_destroy(). Nevertheless, use a helper variable for that call site as well, to retain consistency and to prevent copy-pasted code from causing similar problems in the future. --- lib/dns/resolver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 15297024c0e..154248ec891 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -4357,9 +4357,9 @@ fctx_try(fetchctx_t *fctx, bool retrying, bool badcache) { options, 0, fctx->qc, task, resume_qmin, fctx, &fctx->qminrrset, NULL, &fctx->qminfetch); if (result != ISC_R_SUCCESS) { - LOCK(&fctx->res->buckets[fctx->bucketnum].lock); + LOCK(&res->buckets[bucketnum].lock); RUNTIME_CHECK(!fctx_decreference(fctx)); - UNLOCK(&fctx->res->buckets[fctx->bucketnum].lock); + UNLOCK(&res->buckets[bucketnum].lock); fctx_done(fctx, DNS_R_SERVFAIL, __LINE__); } return; @@ -6138,9 +6138,9 @@ cleanup_event: dns_message_detach(&message); isc_event_free(&event); - LOCK(&res->buckets[fctx->bucketnum].lock); + LOCK(&res->buckets[bucketnum].lock); bucket_empty = fctx_decreference(fctx); - UNLOCK(&res->buckets[fctx->bucketnum].lock); + UNLOCK(&res->buckets[bucketnum].lock); if (bucket_empty) { empty_bucket(res); } @@ -7528,6 +7528,7 @@ resume_dslookup(isc_task_t *task, isc_event_t *event) { dns_resolver_t *res; fetchctx_t *fctx; isc_result_t result; + uint32_t bucketnum; bool bucket_empty; dns_rdataset_t nameservers; dns_fixedname_t fixed; @@ -7538,6 +7539,7 @@ resume_dslookup(isc_task_t *task, isc_event_t *event) { fctx = event->ev_arg; REQUIRE(VALID_FCTX(fctx)); res = fctx->res; + bucketnum = fctx->bucketnum; UNUSED(task); FCTXTRACE("resume_dslookup"); @@ -7665,9 +7667,9 @@ cleanup: if (dns_rdataset_isassociated(&nameservers)) { dns_rdataset_disassociate(&nameservers); } - LOCK(&res->buckets[fctx->bucketnum].lock); + LOCK(&res->buckets[bucketnum].lock); bucket_empty = fctx_decreference(fctx); - UNLOCK(&res->buckets[fctx->bucketnum].lock); + UNLOCK(&res->buckets[bucketnum].lock); if (bucket_empty) { empty_bucket(res); } -- GitLab