[ovs-dev] [patch_v2 1/4] Userspace Datapath: Add ALG infra and FTP.
Joe Stringer
joe at ovn.org
Tue Jun 27 20:20:24 UTC 2017
On 27 June 2017 at 11:19, Darrell Ball <dball at vmware.com> wrote:
>
>
> On 6/27/17, 11:04 AM, "Joe Stringer" <joe at ovn.org> wrote:
>
> On 27 June 2017 at 10:51, Darrell Ball <dball at vmware.com> wrote:
> >
> >
> > On 6/27/17, 10:47 AM, "Joe Stringer" <joe at ovn.org> wrote:
> >
> > On 27 June 2017 at 10:42, Darrell Ball <dball at vmware.com> wrote:
> > >
> > >
> > > On 6/27/17, 10:40 AM, "Joe Stringer" <joe at ovn.org> wrote:
> > >
> > > On 26 June 2017 at 18:19, Darrell Ball <dball at vmware.com> wrote:
> > > >
> > > >
> > > > On 6/26/17, 4:49 PM, "Joe Stringer" <joe at ovn.org> wrote:
> > > >
> > > > On 26 June 2017 at 13:22, Joe Stringer <joe at ovn.org> wrote:
> > > > > On 26 June 2017 at 11:45, Darrell Ball <dball at vmware.com> wrote:
> > > > >> On 6/26/17, 10:22 AM, "Joe Stringer" <joe at ovn.org> wrote:
> > > > >> On 23 June 2017 at 18:57, Darrell Ball <dball at vmware.com> wrote:
> > > > >> > On 6/23/17, 4:08 PM, "ovs-dev-bounces at openvswitch.org on behalf of Joe Stringer" <ovs-dev-bounces at openvswitch.org on behalf of joe at ovn.org> wrote:
> > > > >> > On 17 June 2017 at 15:53, Darrell Ball <dlu998 at 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?
> > >
> > > 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.
OK, now I see what you mean.
I think that clearing these pointers in that error path would make the
code more explicit about the status of one of the parameters modified
by this function, and would limit side-effects. I don't see a negative
to this proposed change.
More information about the dev
mailing list