* [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later
@ 2016-09-06 14:43 fgao
2016-09-06 14:51 ` Florian Westphal
0 siblings, 1 reply; 5+ messages in thread
From: fgao @ 2016-09-06 14:43 UTC (permalink / raw)
To: pablo, netfilter-devel, fw, coreteam, netdev; +Cc: gfree.wind, Gao Feng
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>
---
v5: Return NF_ACCEPT instead of NF_DROP when nfct_seqadj_ext_add failed in nf_nat_setup_info
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);
}
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index de31818..f8b916a 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_ACCEPT;
}
if (maniptype == NF_NAT_MANIP_SRC) {
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later
2016-09-06 14:43 [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later fgao
@ 2016-09-06 14:51 ` Florian Westphal
2016-09-06 14:54 ` Gao Feng
2016-09-07 9:18 ` Pablo Neira Ayuso
0 siblings, 2 replies; 5+ messages in thread
From: Florian Westphal @ 2016-09-06 14:51 UTC (permalink / raw)
To: fgao; +Cc: pablo, netfilter-devel, fw, coreteam, netdev, gfree.wind
fgao@ikuai8.com <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>
> ---
> v5: Return NF_ACCEPT instead of NF_DROP when nfct_seqadj_ext_add failed in nf_nat_setup_info
> 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;
> + }
if (!nfct_add_synrpxy(ct, tmpl)) {
nf_conntrack_free(ct);
return NULL;
}
static bool nf_ct_add_synproxy(struct nf_conn *ct, const struct nf_conn *tmpl)
{
if (tmpl && nfct_synproxy(tmpl)) {
if (!nfct_seqadj_ext_add(ct))
return false;
if (!nfct_synproxy_ext_add(ct))
return false;
}
return true;
}
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index de31818..f8b916a 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_ACCEPT;
> }
Hmm, why accept?
We are asked to add extension to rewrite sequence numbers, but
we cannot. How can the connection work if we cannot munge/track
seqno rewrites?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later
2016-09-06 14:51 ` Florian Westphal
@ 2016-09-06 14:54 ` Gao Feng
2016-09-06 15:07 ` Gao Feng
2016-09-07 9:18 ` Pablo Neira Ayuso
1 sibling, 1 reply; 5+ messages in thread
From: Gao Feng @ 2016-09-06 14:54 UTC (permalink / raw)
To: Florian Westphal
Cc: Pablo Neira Ayuso, Netfilter Developer Mailing List, coreteam,
Linux Kernel Network Developers
inline
On Tue, Sep 6, 2016 at 10:51 PM, Florian Westphal <fw@strlen.de> wrote:
> fgao@ikuai8.com <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>
>> ---
>> v5: Return NF_ACCEPT instead of NF_DROP when nfct_seqadj_ext_add failed in nf_nat_setup_info
>> 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;
>> + }
>
> if (!nfct_add_synrpxy(ct, tmpl)) {
> nf_conntrack_free(ct);
> return NULL;
> }
>
> static bool nf_ct_add_synproxy(struct nf_conn *ct, const struct nf_conn *tmpl)
> {
> if (tmpl && nfct_synproxy(tmpl)) {
> if (!nfct_seqadj_ext_add(ct))
> return false;
>
> if (!nfct_synproxy_ext_add(ct))
> return false;
> }
>
> return true;
> }
>
>> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
>> index de31818..f8b916a 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_ACCEPT;
>> }
>
> Hmm, why accept?
>
> We are asked to add extension to rewrite sequence numbers, but
> we cannot. How can the connection work if we cannot munge/track
> seqno rewrites?
I thought we should drop the packet in that case before.
But Pablo point out one case that the ctnetlink also could add the
seqadj extension too.
There is one synchronization case.
I think he is right. Then modify the patch according to his advice.
Best Regards
Feng
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later
2016-09-06 14:54 ` Gao Feng
@ 2016-09-06 15:07 ` Gao Feng
0 siblings, 0 replies; 5+ messages in thread
From: Gao Feng @ 2016-09-06 15:07 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Netfilter Developer Mailing List
Hi Pablo,
On Tue, Sep 6, 2016 at 10:54 PM, Gao Feng <fgao@ikuai8.com> wrote:
> inline
>
> On Tue, Sep 6, 2016 at 10:51 PM, Florian Westphal <fw@strlen.de> wrote:
>> fgao@ikuai8.com <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>
>>> ---
>>> v5: Return NF_ACCEPT instead of NF_DROP when nfct_seqadj_ext_add failed in nf_nat_setup_info
>>> 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;
>>> + }
>>
>> if (!nfct_add_synrpxy(ct, tmpl)) {
>> nf_conntrack_free(ct);
>> return NULL;
>> }
>>
>> static bool nf_ct_add_synproxy(struct nf_conn *ct, const struct nf_conn *tmpl)
>> {
>> if (tmpl && nfct_synproxy(tmpl)) {
>> if (!nfct_seqadj_ext_add(ct))
>> return false;
>>
>> if (!nfct_synproxy_ext_add(ct))
>> return false;
>> }
>>
>> return true;
>> }
>>
>>> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
>>> index de31818..f8b916a 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_ACCEPT;
>>> }
>>
>> Hmm, why accept?
>>
>> We are asked to add extension to rewrite sequence numbers, but
>> we cannot. How can the connection work if we cannot munge/track
>> seqno rewrites?
>
> I thought we should drop the packet in that case before.
> But Pablo point out one case that the ctnetlink also could add the
> seqadj extension too.
> There is one synchronization case.
>
> I think he is right. Then modify the patch according to his advice.
>
> Best Regards
> Feng
I find the commit which uses seqadj extension firstly in ctnetlink.
But I think it should be ok and reply the details in my "v4" patch of
this subject.
Could you check it please?
If my understanding is wrong, I could send one patch to fix it.
Regards
Feng
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later
2016-09-06 14:51 ` Florian Westphal
2016-09-06 14:54 ` Gao Feng
@ 2016-09-07 9:18 ` Pablo Neira Ayuso
1 sibling, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2016-09-07 9:18 UTC (permalink / raw)
To: Florian Westphal; +Cc: fgao, netfilter-devel, coreteam, netdev, gfree.wind
On Tue, Sep 06, 2016 at 04:51:17PM +0200, Florian Westphal wrote:
> fgao@ikuai8.com <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>
> > ---
> > v5: Return NF_ACCEPT instead of NF_DROP when nfct_seqadj_ext_add failed in nf_nat_setup_info
> > 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;
> > + }
>
> if (!nfct_add_synrpxy(ct, tmpl)) {
> nf_conntrack_free(ct);
> return NULL;
> }
>
> static bool nf_ct_add_synproxy(struct nf_conn *ct, const struct nf_conn *tmpl)
> {
> if (tmpl && nfct_synproxy(tmpl)) {
> if (!nfct_seqadj_ext_add(ct))
> return false;
>
> if (!nfct_synproxy_ext_add(ct))
> return false;
> }
>
> return true;
> }
I like this helper idea, please send a v6 using this using what
Florian is proposing.
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index de31818..f8b916a 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_ACCEPT;
> > }
>
> Hmm, why accept?
>
> We are asked to add extension to rewrite sequence numbers, but
> we cannot. How can the connection work if we cannot munge/track
> seqno rewrites?
Actually we could just NF_DROP, but if we add support for ctnetlink to
inject entries with sequence adjustment with no NAT. I think we need
this to get synproxy working with conntrackd state synchronization.
Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-07 9:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-06 14:43 [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later fgao
2016-09-06 14:51 ` Florian Westphal
2016-09-06 14:54 ` Gao Feng
2016-09-06 15:07 ` Gao Feng
2016-09-07 9:18 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).