Re: [PATCH v4 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later

inline

On Tue, Sep 6, 2016 at 6:17 PM, Pablo Neira Ayuso [off-list ref] wrote:

On Tue, Sep 06, 2016 at 09:57:23AM +0800, fgao@ikuai8.com wrote:

quoted

From: Gao Feng <redacted>

When memory is exhausted, nfct_seqadj_ext_add may fail to add the seqadj
extension. But the function nf_ct_seqadj_init doesn't check if get valid
seqadj pointer by the nfct_seqadj.

Now drop the packet directly when fail to add seqadj extension to avoid
dereference NULL pointer in nf_ct_seqadj_init.

Signed-off-by: Gao Feng <redacted>
---
 v4: Drop the packet directly when fail to add seqadj extension;
 v3: Remove the warning log when seqadj is null;
 v2: Remove the unnessary seqadj check in nf_ct_seq_adjust
 v1: Initial patch

 net/netfilter/nf_conntrack_core.c | 6 +++++-
 net/netfilter/nf_nat_core.c       | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index dd2c43a..dfa76ce 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c

@@ -1036,7 +1036,11 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
              return (struct nf_conntrack_tuple_hash *)ct;

      if (tmpl && nfct_synproxy(tmpl)) {
-             nfct_seqadj_ext_add(ct);
+             if (!nfct_seqadj_ext_add(ct)) {
+                     nf_conntrack_free(ct);
+                     pr_debug("Can't add seqadj extension\n");
+                     return NULL;
+             }
              nfct_synproxy_ext_add(ct);

I think this is part of the same logical change, ie. nf_ct_ext_add()
returns NULL, then I would also fix nfct_synproxy_ext_add() in this
go.

Sorry, I am not clear well.
Need I modify the logic of this part?

quoted

      }

diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index de31818..b82282a 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c

@@ -441,7 +441,8 @@ nf_nat_setup_info(struct nf_conn *ct,
                      ct->status |= IPS_DST_NAT;

              if (nfct_help(ct))
-                     nfct_seqadj_ext_add(ct);
+                     if (!nfct_seqadj_ext_add(ct))
+                             return NF_DROP;

ctnetlink may have created a conntrack with seqadj in place by when we
call nf_nat_setup_info() so NF_ACCEPT would be more conservative, eg.
via conntrackd state synchronization.

Actually, after a quick look at ctnetlink, I don't see any any call to
nfct_seqadj_ext_add() from there, so I suspect this is broken since
SYNPROXY was introduced. It would be great if you can review this and
send us patches to fix this, if indeed needed.

Thanks!

Let me confirm the problem, or I am afraid I would misunderstand our meaning.
This patch only need to modify the "NF_DROP" to "NF_ACCEPT", is it?

Then I could commit another patch to fix the ctnetlink lost nfct_seqadj_ext_add.

Best Regards
Feng