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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
