On 26 June 2017 at 18:19, Darrell Ball <db...@vmware.com> wrote:
>
>
> On 6/26/17, 4:49 PM, "Joe Stringer" <j...@ovn.org> wrote:
>
> On 26 June 2017 at 13:22, Joe Stringer <j...@ovn.org> wrote:
> > On 26 June 2017 at 11:45, Darrell Ball <db...@vmware.com> wrote:
> >> On 6/26/17, 10:22 AM, "Joe Stringer" <j...@ovn.org> wrote:
> >> On 23 June 2017 at 18:57, Darrell Ball <db...@vmware.com> wrote:
> >> > On 6/23/17, 4:08 PM, "ovs-dev-boun...@openvswitch.org on behalf
> of Joe Stringer" <ovs-dev-boun...@openvswitch.org on behalf of j...@ovn.org>
> wrote:
> >> > On 17 June 2017 at 15:53, Darrell Ball <dlu...@gmail.com>
> 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?
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
