On 6/27/17, 11:04 AM, "Joe Stringer" <[email protected]> wrote:
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.
No, as I stated already, since the un_nat creation (and also conn) never
completed
the conntype is not set and create_un_nat_conn() is not called.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
