On 27 June 2017 at 10:51, Darrell Ball <[email protected]> wrote: > > > On 6/27/17, 10:47 AM, "Joe Stringer" <[email protected]> wrote: > > On 27 June 2017 at 10:42, Darrell Ball <[email protected]> wrote: > > > > > > On 6/27/17, 10:40 AM, "Joe Stringer" <[email protected]> wrote: > > > > On 26 June 2017 at 18:19, Darrell Ball <[email protected]> wrote: > > > > > > > > > On 6/26/17, 4:49 PM, "Joe Stringer" <[email protected]> wrote: > > > > > > 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? > > > > > > Nope, because there is no un_nat conn. > > > > So you mean that dangling references are returned inside > > conn_for_un_nat_copy but they're just not used? > > > > JTBC, nothing can be used, since there is not even a connection. > > So if nat_select_range_tuple() is called, and it runs the "*nat_conn = > *conn;" line which updates conn_for_un_nat_copy, then > nat_select_range_tuple() fails to select a tuple and returns false, > then the if (!nat_res) cleanup / return NULL path is called, then what > cleans up the dangling pointers that are now in conn_for_un_nat_copy? > > conn_for_un_nat_copy is a local variable in the caller
Which is then potentially passed to another function, create_un_nat_conn(), which then does an xmemdup() to propagate those dangling pointers further.. seems like it would be safest to clear out the conn_for_un_nat_copy->alg and ->nat_info pointers before freeing 'nc' in that path. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
