07d38ea3cb
Signed-off-by: huangyu <huangyu106@huawei.com> (cherry picked from commit cd59b6ec71f1147990c7f96b1e74baf413b7d4c9)
96 lines
3.5 KiB
Diff
96 lines
3.5 KiB
Diff
From 6505056267dffada1d0fa326f318b241f4b64747 Mon Sep 17 00:00:00 2001
|
|
From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
|
|
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
|
|
|