Hi Pablo,
On Tue, Sep 6, 2016 at 6:17 PM, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> On Tue, Sep 06, 2016 at 09:57:23AM +0800, fgao@xxxxxxxxxx wrote:
>> From: Gao Feng <fgao@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
