From 53800281fea22b2b32cec64e2e9174016581a2a9 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 1 Aug 2019 09:30:49 +1000 Subject: [PATCH 1/4] maintain a reference to 'rpz' when calling rpz.c:update_quantum --- lib/dns/rpz.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index d47a8bd0fd..b5763fab0a 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -169,6 +169,9 @@ struct dns_rpz_nm_data { dns_rpz_nm_zbits_t wild; }; +static void +rpz_detach(dns_rpz_zone_t **rpzp, dns_rpz_zones_t *rpzs); + #if 0 /* * Catch a name while debugging. @@ -1980,6 +1983,7 @@ update_quantum(isc_task_t *task, isc_event_t *event) { isc_ht_destroy(&rpz->newnodes); dns_db_closeversion(rpz->updb, &rpz->updbversion, false); dns_db_detach(&rpz->updb); + rpz_detach(&rpz, rpz->rpzs); } static void @@ -1994,6 +1998,7 @@ dns_rpz_update_from_db(dns_rpz_zone_t *rpz) { REQUIRE(rpz->updbit == NULL); REQUIRE(rpz->newnodes == NULL); + isc_refcount_increment(&rpz->refs); dns_db_attach(rpz->db, &rpz->updb); rpz->updbversion = rpz->dbversion; rpz->dbversion = NULL; @@ -2018,6 +2023,7 @@ dns_rpz_update_from_db(dns_rpz_zone_t *rpz) { isc_ht_destroy(&rpz->newnodes); dns_db_closeversion(rpz->updb, &rpz->updbversion, false); dns_db_detach(&rpz->updb); + rpz_detach(&rpz, rpz->rpzs); } /* From 9b10cfef56901ed417b1a7d77018dd8ff929d0e6 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 1 Aug 2019 11:15:05 +1000 Subject: [PATCH 2/4] keep rpzs around until everything referencing it has gone --- lib/dns/include/dns/rpz.h | 1 + lib/dns/rpz.c | 37 ++++++++++++++++++++++++++++++------- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/lib/dns/include/dns/rpz.h b/lib/dns/include/dns/rpz.h index 0c403d8a9e..b4c0c19b55 100644 --- a/lib/dns/include/dns/rpz.h +++ b/lib/dns/include/dns/rpz.h @@ -247,6 +247,7 @@ struct dns_rpz_zones { isc_timermgr_t *timermgr; isc_task_t *updater; isc_refcount_t refs; + isc_refcount_t irefs; /* * One lock for short term read-only search that guarantees the * consistency of the pointers. diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index b5763fab0a..391ea070d9 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -170,7 +170,10 @@ struct dns_rpz_nm_data { }; static void -rpz_detach(dns_rpz_zone_t **rpzp, dns_rpz_zones_t *rpzs); +rpz_detach(dns_rpz_zone_t **rpzp); + +static void +rpz_detach_rpzs(dns_rpz_zones_t **rpzsp); #if 0 /* @@ -1447,6 +1450,7 @@ dns_rpz_new_zones(dns_rpz_zones_t **rpzsp, char *rps_cstr, isc_mutex_init(&zones->maint_lock); isc_refcount_init(&zones->refs, 1); + isc_refcount_init(&zones->irefs, 1); zones->rps_cstr = rps_cstr; zones->rps_cstr_size = rps_cstr_size; @@ -1480,7 +1484,9 @@ cleanup_task: dns_rbt_destroy(&zones->rbt); cleanup_rbt: - INSIST(isc_refcount_decrement(&zones->refs) > 0); + INSIST(isc_refcount_decrement(&zones->irefs) == 1); + isc_refcount_destroy(&zones->irefs); + INSIST(isc_refcount_decrement(&zones->refs) == 1); isc_refcount_destroy(&zones->refs); isc_mutex_destroy(&zones->maint_lock); @@ -1543,6 +1549,7 @@ dns_rpz_new_zone(dns_rpz_zones_t *rpzs, dns_rpz_zone_t **rpzp) { zone->updb = NULL; zone->updbversion = NULL; zone->updbit = NULL; + isc_refcount_increment(&rpzs->irefs); zone->rpzs = rpzs; zone->db_registered = false; zone->addsoa = true; @@ -1563,7 +1570,7 @@ cleanup_timer: INSIST(isc_refcount_decrement(&zone->refs) > 0); isc_refcount_destroy(&zone->refs); - isc_mem_put(zone->rpzs->mctx, zone, sizeof(*zone)); + isc_mem_put(rpzs->mctx, zone, sizeof(*zone)); return (result); } @@ -1983,7 +1990,7 @@ update_quantum(isc_task_t *task, isc_event_t *event) { isc_ht_destroy(&rpz->newnodes); dns_db_closeversion(rpz->updb, &rpz->updbversion, false); dns_db_detach(&rpz->updb); - rpz_detach(&rpz, rpz->rpzs); + rpz_detach(&rpz); } static void @@ -2023,7 +2030,7 @@ dns_rpz_update_from_db(dns_rpz_zone_t *rpz) { isc_ht_destroy(&rpz->newnodes); dns_db_closeversion(rpz->updb, &rpz->updbversion, false); dns_db_detach(&rpz->updb); - rpz_detach(&rpz, rpz->rpzs); + rpz_detach(&rpz); } /* @@ -2063,8 +2070,9 @@ cidr_free(dns_rpz_zones_t *rpzs) { * before discarding the overall rpz structure. */ static void -rpz_detach(dns_rpz_zone_t **rpzp, dns_rpz_zones_t *rpzs) { +rpz_detach(dns_rpz_zone_t **rpzp) { dns_rpz_zone_t *rpz; + dns_rpz_zones_t *rpzs; REQUIRE(rpzp != NULL && *rpzp != NULL); @@ -2077,6 +2085,9 @@ rpz_detach(dns_rpz_zone_t **rpzp, dns_rpz_zones_t *rpzs) { isc_refcount_destroy(&rpz->refs); + rpzs = rpz->rpzs; + rpz->rpzs = NULL; + if (dns_name_dynamic(&rpz->origin)) { dns_name_free(&rpz->origin, rpzs->mctx); } @@ -2129,7 +2140,9 @@ rpz_detach(dns_rpz_zone_t **rpzp, dns_rpz_zones_t *rpzs) { isc_timer_detach(&rpz->updatetimer); isc_ht_destroy(&rpz->nodes); + isc_mem_put(rpzs->mctx, rpz, sizeof(*rpz)); + rpz_detach_rpzs(&rpzs); } void @@ -2166,10 +2179,20 @@ dns_rpz_detach_rpzs(dns_rpz_zones_t **rpzsp) { dns_rpz_zone_t *rpz = rpzs->zones[rpz_num]; rpzs->zones[rpz_num] = NULL; if (rpz != NULL) { - rpz_detach(&rpz, rpzs); + rpz_detach(&rpz); } } + rpz_detach_rpzs(&rpzs); + } +} +static void +rpz_detach_rpzs(dns_rpz_zones_t **rpzsp) { + REQUIRE(rpzsp != NULL && *rpzsp != NULL); + dns_rpz_zones_t *rpzs = *rpzsp; + *rpzsp = NULL; + + if (isc_refcount_decrement(&rpzs->irefs) == 1) { if (rpzs->rps_cstr_size != 0) { #ifdef USE_DNSRPS librpz->client_detach(&rpzs->rps_client); From 9cfd0ecccf23862c910aba19f8f487c4135dc505 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 1 Aug 2019 11:32:28 +1000 Subject: [PATCH 3/4] remove invalid comment --- lib/dns/rpz.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index 391ea070d9..b31ee6fd17 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -2162,10 +2162,7 @@ dns_rpz_detach_rpzs(dns_rpz_zones_t **rpzsp) { *rpzsp = NULL; if (isc_refcount_decrement(&rpzs->refs) == 1) { - /* - * Destroy the task first, so that nothing runs - * in the background that might race with us. - */ + isc_task_destroy(&rpzs->updater); /* From 49c31702bd47d8a2aec7eb56b26f38adae072b4d Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 1 Aug 2019 16:03:55 +1000 Subject: [PATCH 4/4] add CHANGES --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index c90484780a..9910fdabda 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5274. [bug] Address potential use after free race when shutting + down rpz. [GL #1175] + 5273. [bug] Check that bits [64..71] of a dns64 prefix are zero. [GL #1159]