On 26 June 2017 at 13:22, Joe Stringer <[email protected]> wrote: > On 26 June 2017 at 11:45, Darrell Ball <[email protected]> wrote: >> On 6/26/17, 10:22 AM, "Joe Stringer" <[email protected]> wrote: >> On 23 June 2017 at 18:57, Darrell Ball <[email protected]> wrote: >> > On 6/23/17, 4:08 PM, "[email protected] on behalf of Joe >> Stringer" <[email protected] on behalf of [email protected]> wrote: >> > On 17 June 2017 at 15:53, Darrell Ball <[email protected]> wrote: >> > > @@ -554,34 +681,50 @@ conn_not_found(struct conntrack *ct, >> struct dp_packet *pkt, >> > > nc->rev_key = nc->key; >> > > conn_key_reverse(&nc->rev_key); >> > > >> > > + if (helper) { >> > > + nc->alg = xstrdup(helper); >> > > + } >> > > + >> > > + if (alg_exp) { >> > > + nc->alg_related = true; >> > > + nc->mark = alg_exp->master_mark; >> > > + nc->label = alg_exp->master_label; >> > > + nc->master_key = alg_exp->master_key; >> > > + } >> > > + >> > > if (nat_action_info) { >> > > nc->nat_info = xmemdup(nat_action_info, sizeof >> *nc->nat_info); >> > > - ct_rwlock_wrlock(&ct->nat_resources_lock); >> > > - >> > > - bool nat_res = nat_select_range_tuple(ct, nc, >> > > - >> conn_for_un_nat_copy); >> > > >> > > - if (!nat_res) { >> > > - free(nc->nat_info); >> > > - nc->nat_info = NULL; >> > > - free (nc); >> > > - ct_rwlock_unlock(&ct->nat_resources_lock); >> > > - return NULL; >> > > - } >> > > + if (alg_exp) { >> > > + nc->rev_key.src.addr = alg_nat_repl_addr; >> > > + nc->nat_info->nat_action = NAT_ACTION_DST; >> > > + *conn_for_un_nat_copy = *nc; >> > > + } else { >> > > + ct_rwlock_wrlock(&ct->resources_lock); >> > > + bool nat_res = nat_select_range_tuple( >> > > + ct, nc, >> conn_for_un_nat_copy); >> > > + >> > > + if (!nat_res) { >> > > + free(nc->nat_info); >> > > + nc->nat_info = NULL; >> > > + free (nc); >> > >> > I think that nc->alg may be leaked here? any reason it doesn't use >> > delete_conn()? >> > >> > Good >> > Yes, alg will leak in this rare error case and yes, delete_conn() >> should be used >> > here, as everywhere. >> >> OK. >> >> > > + ct_rwlock_unlock(&ct->resources_lock); >> > > + return NULL; >> > > + } >> > > >> > > - if (conn_for_un_nat_copy && >> > > - nc->conn_type == CT_CONN_TYPE_DEFAULT) { >> > > *nc = *conn_for_un_nat_copy; >> > >> > Perhaps nc->alg and/or nc->nat_info may be leaked here? >> > >> > No, the un_nat conn has no such allocations, so there is nothing to >> leak. >> >> I don't mean conn_for_un_nat_copy, I mean *nc which could have had an >> xstrdup()'d 'alg' attached. Won't this overwrite all fields in 'nc'? >> >> I see your question now. >> No, at this point, the copy gets the same pointers to the alg string and >> nat_info. >> Only nc needs them and the un_nat copy ptrs are nulled. >> There is only one allocation set. > > Hmm. Maybe I'm just missing something, let me walk through it step by > step below and let's see where it goes. > > if (helper) { > nc->alg = xstrdup(helper); > ^ nc->alg is set > } > > if (alg_exp) { > ^ false; do not execute this block > nc->alg_related = true; > nc->mark = alg_exp->master_mark; > nc->label = alg_exp->master_label; > nc->master_key = alg_exp->master_key; > } > > if (nat_action_info) { > ^ true, execute this part > nc->nat_info = xmemdup(nat_action_info, sizeof *nc->nat_info); > > if (alg_exp) { > ^ false; skip to else > nc->rev_key.src.addr = alg_nat_repl_addr; > nc->nat_info->nat_action = NAT_ACTION_DST; > *conn_for_un_nat_copy = *nc; > } else { > ^ We go through this condition > ct_rwlock_wrlock(&ct->resources_lock); > bool nat_res = nat_select_range_tuple( > ct, nc, conn_for_un_nat_copy); > > if (!nat_res) { > ^ false; do not execute this block > free(nc->nat_info); > nc->nat_info = NULL; > free (nc); > ct_rwlock_unlock(&ct->resources_lock); > return NULL; > } > > *nc = *conn_for_un_nat_copy; > ^ Now: > nc->alg is overwritten by conn_for_un_nat_copy->alg > nc->nat_info is overwritten by conn_for_un_nat_copy->nat_info > > We don't free either of these.
As discussed offline, the copy of '*nc' into '*conn_for_un_nat_copy' nested inside nat_select_range_tuple() is very well hidden. This means that the above is not a problem... but what if (!nat_res) ? Then conn_for_un_nat_copy() has a reference to these alg/nat_info parameters which are freed from 'nc' inside that block, then 'conn_for_un_nat_copy' is returned. Could there be a use-after-free then? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
