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

Message ID 1531239309-18700-1-git-send-email-dlu998@gmail.com
State Superseded
Headers show
Series
  • [ovs-dev,v1] conntrack: Fix conn_update_state_alg use after free.
Related show

Commit Message

Darrell Ball July 10, 2018, 4:15 p.m.
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.

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

Comments

Ben Pfaff July 10, 2018, 8:27 p.m. | #1
On Tue, Jul 10, 2018 at 09:15:09AM -0700, Darrell Ball wrote:
> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> Signed-off-by: Darrell Ball <dlu998@gmail.com>

This could use a little longer commit message.  Is the following
accurate:

When conn_update_state() returns true, it has freed 'conn' which
therefore should not be passed into handle_ftp_ctl().

Thanks,

Ben.

> ---
> 
> Needs backporting as far back as 2.8.
> 
>  lib/conntrack.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 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;
>      }
> -- 
> 1.9.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Darrell Ball July 10, 2018, 11:44 p.m. | #2
sorry, I only had a few minutes in the morning
I sent a V2, with a proper commit message.

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

Thanks Darrell


On Tue, Jul 10, 2018 at 1:27 PM, Ben Pfaff <blp@ovn.org> wrote:

> On Tue, Jul 10, 2018 at 09:15:09AM -0700, Darrell Ball wrote:
> > Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> > Signed-off-by: Darrell Ball <dlu998@gmail.com>
>
> This could use a little longer commit message.  Is the following
> accurate:
>
> When conn_update_state() returns true, it has freed 'conn' which
> therefore should not be passed into handle_ftp_ctl().
>
> Thanks,
>
> Ben.
>
> > ---
> >
> > Needs backporting as far back as 2.8.
> >
> >  lib/conntrack.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > 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;
> >      }
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

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;
     }