[ovs-dev,v2] conntrack: Fix conn_update_state_alg use after free.

Message ID 1531266118-23483-1-git-send-email-dlu998@gmail.com
State Accepted
Headers show
Series
  • [ovs-dev,v2] conntrack: Fix conn_update_state_alg use after free.
Related show

Commit Message

Darrell Ball July 10, 2018, 11:41 p.m.
When conn_update_state() returns true, conn has been freed, so skip calling
handle_ftp_ctl() with this conn and instead follow code path for new
connections.

Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
Signed-off-by: Darrell Ball <dlu998@gmail.com>
---

Needs backporting as far back as 2.8.

v1->v2: Fix commit messaage.

 lib/conntrack.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Ben Pfaff July 11, 2018, 3:34 p.m. | #1
On Tue, Jul 10, 2018 at 04:41:58PM -0700, Darrell Ball wrote:
> When conn_update_state() returns true, conn has been freed, so skip calling
> handle_ftp_ctl() with this conn and instead follow code path for new
> connections.
> 
> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---
> 
> Needs backporting as far back as 2.8.
> 
> v1->v2: Fix commit messaage.

Thanks, applied and backported.
Ben Pfaff July 11, 2018, 3:36 p.m. | #2
On Wed, Jul 11, 2018 at 08:34:49AM -0700, Ben Pfaff wrote:
> On Tue, Jul 10, 2018 at 04:41:58PM -0700, Darrell Ball wrote:
> > When conn_update_state() returns true, conn has been freed, so skip calling
> > handle_ftp_ctl() with this conn and instead follow code path for new
> > connections.
> > 
> > Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> > Signed-off-by: Darrell Ball <dlu998@gmail.com>
> > ---
> > 
> > Needs backporting as far back as 2.8.
> > 
> > v1->v2: Fix commit messaage.
> 
> Thanks, applied and backported.

Actually, I spoke too soon.  I applied to master and branch-2.9, but
branch-2.8 isn't a clean backport.  Would you mind submitting a
branch-2.8 backport version?

Thanks,

Ben.
Darrell Ball July 11, 2018, 3:56 p.m. | #3
On Wed, Jul 11, 2018 at 8:36 AM, Ben Pfaff <blp@ovn.org> wrote:

> On Wed, Jul 11, 2018 at 08:34:49AM -0700, Ben Pfaff wrote:
> > On Tue, Jul 10, 2018 at 04:41:58PM -0700, Darrell Ball wrote:
> > > When conn_update_state() returns true, conn has been freed, so skip
> calling
> > > handle_ftp_ctl() with this conn and instead follow code path for new
> > > connections.
> > >
> > > Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> > > Signed-off-by: Darrell Ball <dlu998@gmail.com>
> > > ---
> > >
> > > Needs backporting as far back as 2.8.
> > >
> > > v1->v2: Fix commit messaage.
> >
> > Thanks, applied and backported.
>
> Actually, I spoke too soon.  I applied to master and branch-2.9, but
> branch-2.8 isn't a clean backport.  Would you mind submitting a
> branch-2.8 backport version?
>


oops, right.
Here it is:

https://patchwork.ozlabs.org/patch/942587/




>
> Thanks,
>
> Ben.
>
Darrell Ball July 11, 2018, 4 p.m. | #4
On Wed, Jul 11, 2018 at 8:56 AM, Darrell Ball <dlu998@gmail.com> wrote:

>
>
> On Wed, Jul 11, 2018 at 8:36 AM, Ben Pfaff <blp@ovn.org> wrote:
>
>> On Wed, Jul 11, 2018 at 08:34:49AM -0700, Ben Pfaff wrote:
>> > On Tue, Jul 10, 2018 at 04:41:58PM -0700, Darrell Ball wrote:
>> > > When conn_update_state() returns true, conn has been freed, so skip
>> calling
>> > > handle_ftp_ctl() with this conn and instead follow code path for new
>> > > connections.
>> > >
>> > > Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
>> > > Signed-off-by: Darrell Ball <dlu998@gmail.com>
>> > > ---
>> > >
>> > > Needs backporting as far back as 2.8.
>> > >
>> > > v1->v2: Fix commit messaage.
>> >
>> > Thanks, applied and backported.
>>
>> Actually, I spoke too soon.  I applied to master and branch-2.9, but
>> branch-2.8 isn't a clean backport.  Would you mind submitting a
>> branch-2.8 backport version?
>>
>
>
> oops, right.
> Here it is:
>
> https://patchwork.ozlabs.org/patch/942587/
>
>
>

actually, I just noticed the commit message mentions a new function in 2.9
I'll send a V2




>
>
>>
>> Thanks,
>>
>> Ben.
>>
>
>

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index e1c1f2e..b818584 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1159,8 +1159,11 @@  conn_update_state_alg(struct conntrack *ct, struct dp_packet *pkt,
         } else {
             *create_new_conn = conn_update_state(ct, pkt, ctx, &conn, now,
                                                 bucket);
-            handle_ftp_ctl(ct, ctx, pkt, conn, now, CT_FTP_CTL_OTHER,
-                           !!nat_action_info);
+
+            if (*create_new_conn == false) {
+                handle_ftp_ctl(ct, ctx, pkt, conn, now, CT_FTP_CTL_OTHER,
+                               !!nat_action_info);
+            }
         }
         return true;
     }