From 296b49cf2077bf262ca9d97f5117e6669e081f6b Mon Sep 17 00:00:00 2001 From: Erik Boasson Date: Sat, 16 May 2020 21:54:11 +0200 Subject: [PATCH] Fix use-after-free via (proxy)pp min lease pointer This changes the handling of the removal of the lease of a manual liveliness (proxy) writer from the (proxy) participant, such that the invariant maintained for the "min lease" objects in the (proxy) participant changes from: a clone of some lease with the minimum duration, to: a clone of the lease that is returned by the ddsrt_fibheap_min operation on the lease heap. This fixes a use-after-free of the entity pointed to by the cloned lease object in a scenario where the shortest lease duration is used by multiple writers and the removal of a lease from the heap shuffles the remaining entries around. For example (before this change): 1. initial situation: three writers w1, w2 and w3 with equal lease durations: - pp.heap = w1.lease : w2.lease w3.lease - pp.minl = clone of w1.lease 2. delete w2: - assuming deleting w2.lease from the heap moves w3.lease to the front (only guarantee is that there are no smaller keys in the heap than that of the entry returned by minimum operation) - min(pp.heap) = w1.lease != w2.lease thus: pp.minl unchanged, pp.minl.entity = w1 - pp.heap = w3.lease : w1.lease 3. delete w1: - min(pp.heap) = w3.lease != w1.lease, thus: pp.minl unchanged, pp.minl.entity = w1 - pp.heap = w3 - free w1 - now pp.minl.entity has a dangling pointer, touched on deleting the (proxy) particpant or on lease expiry. With this chamge, pp.minl is updated in step 2 to be a clone of w3.lease because the lease returned by min(pp.heap) changes. This ensures that in step 3 there is no dangling pointer and no use-after-free. Signed-off-by: Erik Boasson --- src/core/ddsi/include/dds/ddsi/q_entity.h | 6 ++-- src/core/ddsi/src/q_entity.c | 36 +++++++++++++---------- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/core/ddsi/include/dds/ddsi/q_entity.h b/src/core/ddsi/include/dds/ddsi/q_entity.h index 8a4f22f..ce1c7c3 100644 --- a/src/core/ddsi/include/dds/ddsi/q_entity.h +++ b/src/core/ddsi/include/dds/ddsi/q_entity.h @@ -231,7 +231,7 @@ struct participant int32_t builtin_refc; /* number of built-in endpoints in this participant [refc_lock] */ int builtins_deleted; /* whether deletion of built-in endpoints has been initiated [refc_lock] */ ddsrt_fibheap_t ldur_auto_wr; /* Heap that contains lease duration for writers with automatic liveliness in this participant */ - ddsrt_atomic_voidp_t minl_man; /* lease object for shortest manual-by-participant liveliness writer's lease */ + ddsrt_atomic_voidp_t minl_man; /* clone of min(leaseheap_man) */ ddsrt_fibheap_t leaseheap_man; /* keeps leases for this participant's writers (with liveliness manual-by-participant) */ #ifdef DDSI_INCLUDE_SECURITY struct participant_sec_attributes *sec_attr; @@ -369,9 +369,9 @@ struct proxy_participant unsigned bes; /* built-in endpoint set */ ddsi_guid_t privileged_pp_guid; /* if this PP depends on another PP for its SEDP writing */ struct ddsi_plist *plist; /* settings/QoS for this participant */ - ddsrt_atomic_voidp_t minl_auto; /* lease object for shortest automatic liveliness pwr's lease (includes this proxypp's lease) */ + ddsrt_atomic_voidp_t minl_auto; /* clone of min(leaseheap_auto) */ ddsrt_fibheap_t leaseheap_auto; /* keeps leases for this proxypp and leases for pwrs (with liveliness automatic) */ - ddsrt_atomic_voidp_t minl_man; /* lease object for shortest manual-by-participant liveliness pwr's lease */ + ddsrt_atomic_voidp_t minl_man; /* clone of min(leaseheap_man) */ ddsrt_fibheap_t leaseheap_man; /* keeps leases for this proxypp and leases for pwrs (with liveliness manual-by-participant) */ struct lease *lease; /* lease for this proxypp */ struct addrset *as_default; /* default address set to use for user data traffic */ diff --git a/src/core/ddsi/src/q_entity.c b/src/core/ddsi/src/q_entity.c index 5ea7def..2c3ca90 100644 --- a/src/core/ddsi/src/q_entity.c +++ b/src/core/ddsi/src/q_entity.c @@ -750,7 +750,7 @@ static void participant_add_wr_lease_locked (struct participant * pp, const stru minl_prev = ddsrt_fibheap_min (&lease_fhdef_pp, &pp->leaseheap_man); ddsrt_fibheap_insert (&lease_fhdef_pp, &pp->leaseheap_man, wr->lease); minl_new = ddsrt_fibheap_min (&lease_fhdef_pp, &pp->leaseheap_man); - /* if inserted lease is new shortest lease */ + /* ensure pp->minl_man is equivalent to min(leaseheap_man) */ if (minl_prev != minl_new) { ddsrt_etime_t texp = ddsrt_etime_add_duration (ddsrt_time_elapsed (), minl_new->tdur); @@ -770,21 +770,23 @@ static void participant_add_wr_lease_locked (struct participant * pp, const stru static void participant_remove_wr_lease_locked (struct participant * pp, struct writer * wr) { - struct lease *minl; + struct lease *minl_prev; + struct lease *minl_new; assert (wr->lease != NULL); assert (wr->xqos->liveliness.kind == DDS_LIVELINESS_MANUAL_BY_PARTICIPANT); - minl = ddsrt_fibheap_min (&lease_fhdef_pp, &pp->leaseheap_man); + minl_prev = ddsrt_fibheap_min (&lease_fhdef_pp, &pp->leaseheap_man); ddsrt_fibheap_delete (&lease_fhdef_pp, &pp->leaseheap_man, wr->lease); - /* if writer with min lease is removed: update participant lease to use new minimal duration */ - if (wr->lease == minl) + minl_new = ddsrt_fibheap_min (&lease_fhdef_pp, &pp->leaseheap_man); + /* ensure pp->minl_man is equivalent to min(leaseheap_man) */ + if (minl_prev != minl_new) { - if ((minl = ddsrt_fibheap_min (&lease_fhdef_pp, &pp->leaseheap_man)) != NULL) + if (minl_new != NULL) { - dds_duration_t trem = minl->tdur - wr->lease->tdur; + dds_duration_t trem = minl_new->tdur - minl_prev->tdur; assert (trem >= 0); ddsrt_etime_t texp = ddsrt_etime_add_duration (ddsrt_time_elapsed(), trem); - struct lease *lnew = lease_new (texp, minl->tdur, minl->entity); + struct lease *lnew = lease_new (texp, minl_new->tdur, minl_new->entity); participant_replace_minl (pp, lnew); lease_register (lnew); } @@ -4692,7 +4694,7 @@ static void proxy_participant_add_pwr_lease_locked (struct proxy_participant * p minl_prev = ddsrt_fibheap_min (&lease_fhdef_pp, lh); ddsrt_fibheap_insert (&lease_fhdef_pp, lh, pwr->lease); minl_new = ddsrt_fibheap_min (&lease_fhdef_pp, lh); - /* if inserted lease is new shortest lease */ + /* ensure proxypp->minl_man/minl_auto is equivalent to min(leaseheap_man/auto) */ if (proxypp->owns_lease && minl_prev != minl_new) { ddsrt_etime_t texp = ddsrt_etime_add_duration (ddsrt_time_elapsed (), minl_new->tdur); @@ -4713,24 +4715,26 @@ static void proxy_participant_add_pwr_lease_locked (struct proxy_participant * p static void proxy_participant_remove_pwr_lease_locked (struct proxy_participant * proxypp, struct proxy_writer * pwr) { - struct lease *minl; + struct lease *minl_prev; + struct lease *minl_new; bool manbypp; ddsrt_fibheap_t *lh; assert (pwr->lease != NULL); manbypp = (pwr->c.xqos->liveliness.kind == DDS_LIVELINESS_MANUAL_BY_PARTICIPANT); lh = manbypp ? &proxypp->leaseheap_man : &proxypp->leaseheap_auto; - minl = ddsrt_fibheap_min (&lease_fhdef_pp, lh); + minl_prev = ddsrt_fibheap_min (&lease_fhdef_pp, lh); ddsrt_fibheap_delete (&lease_fhdef_pp, lh, pwr->lease); - /* if pwr with min lease is removed: update proxypp lease to use new minimal duration */ - if (proxypp->owns_lease && pwr->lease == minl) + minl_new = ddsrt_fibheap_min (&lease_fhdef_pp, lh); + /* ensure proxypp->minl_man/minl_auto is equivalent to min(leaseheap_man/auto) */ + if (proxypp->owns_lease && minl_prev != minl_new) { - if ((minl = ddsrt_fibheap_min (&lease_fhdef_pp, lh)) != NULL) + if (minl_new != NULL) { - dds_duration_t trem = minl->tdur - pwr->lease->tdur; + dds_duration_t trem = minl_new->tdur - minl_prev->tdur; assert (trem >= 0); ddsrt_etime_t texp = ddsrt_etime_add_duration (ddsrt_time_elapsed(), trem); - struct lease *lnew = lease_new (texp, minl->tdur, minl->entity); + struct lease *lnew = lease_new (texp, minl_new->tdur, minl_new->entity); proxy_participant_replace_minl (proxypp, manbypp, lnew); lease_register (lnew); }