* Re: [PATCH v4 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later
2016-09-06 10:17 ` Pablo Neira Ayuso
@ 2016-09-06 10:37 ` Gao Feng
2016-09-06 15:01 ` Gao Feng
1 sibling, 0 replies; 4+ messages in thread
From: Gao Feng @ 2016-09-06 10:37 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Netfilter Developer Mailing List, Florian Westphal, coreteam,
Linux Kernel Network Developers
inline
On Tue, Sep 6, 2016 at 6:17 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Sep 06, 2016 at 09:57:23AM +0800, fgao@ikuai8.com wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> 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 <fgao@ikuai8.com>
>> ---
>> 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?
>> }
>>
>> 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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later
2016-09-06 10:17 ` Pablo Neira Ayuso
2016-09-06 10:37 ` Gao Feng
@ 2016-09-06 15:01 ` Gao Feng
1 sibling, 0 replies; 4+ messages in thread
From: Gao Feng @ 2016-09-06 15:01 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Netfilter Developer Mailing List, Florian Westphal
Hi Pablo,
On Tue, Sep 6, 2016 at 6:17 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Sep 06, 2016 at 09:57:23AM +0800, fgao@ikuai8.com wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> 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 <fgao@ikuai8.com>
>> ---
>> 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.
>
>> }
>>
>> 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!
I find the commit which uses seqadj instead of nat extenstion.
The commit is "41d73ec053d2424599c4ed8452b889374d523ade netfilter:
nf_conntrack: make sequence number adjustments usuable without NAT".
But I think the current ctnetlink codes are ok if it could not use
seqadj without nat.
When ctnetlink creates the nat info, it also add the seqadj extension.
The call path is:
ctnetlink_create_conntrack->ctnetlink_setup_nat->ctnetlink_parse_nat_setup->nfnetlink_parse_nat_setup_hook
(nfnetlink_parse_nat_setup)->nf_nat_setup_info->nfct_seqadj_ext_add.
So if the ctnetlink always uses the seqadj with nat, the current codes
should be ok.
If not, then need to add another nfct_seqadj_ext_add invocation.
Regards
Feng
^ permalink raw reply [flat|nested] 4+ messages in thread