netdev - Re: [PATCH] netfilter: ipset: wait for xt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2c9efd36-f1f6-b77b-d4eb-f65932cfaba@netfilter.org>
Date: Thu, 5 Oct 2023 16:50:42 +0200 (CEST)
From: Jozsef Kadlecsik <kadlec@...filter.org>
To: Florian Westphal <fw@...len.de>
cc: xiaolinkui <xiaolinkui@....com>, Pablo Neira Ayuso <pablo@...filter.org>, 
    David Miller <davem@...emloft.net>, edumazet@...gle.com, kuba@...nel.org, 
    pabeni@...hat.com, justinstitt@...gle.com, kuniyu@...zon.com, 
    netfilter-devel@...r.kernel.org, coreteam@...filter.org, 
    netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
    Linkui Xiao <xiaolinkui@...inos.cn>
Subject: Re: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus

On Thu, 5 Oct 2023, Florian Westphal wrote:

> xiaolinkui <xiaolinkui@....com> wrote:
> > From: Linkui Xiao <xiaolinkui@...inos.cn>
> > 
> > Before destroying the ipset, take a check on sequence to ensure that the
> > ip_set_test operation of this ipset has been completed.
> > 
> > The code of set_match_v4 is protected by addend=xt_write_recseq_begin() and
> > xt_write_recseq_end(addend). So we can ensure that the test operation is
> > completed by reading seqcount.
> 
> Nope, please don't do this, the xt_set can also be used from nft_compat
> which doesn't use the xtables packet traversers.
> 
> I'd rather use synchonize_rcu() once in ip_set_destroy(), that will
> make sure all concurrent traversers are gone.

But ip_set_destroy() can be called only when there's no reference to the 
set in the kernel and thus there's no ipset function whatsoever in the 
packet path which would access it.

> That said, I still do not understand this fix, the
> match / target destroy hooks are called after the table has
> been completely replaced, i.e., while packets can still be in flight
> no packets should be within the ipset lookup functions when
> this happens, and no more packets should be able to enter them.
> 
> AFAICS the request to delete the set will fail if its still referenced
> via any rule. xt_set holds references to the sets.
> 
> So:
> 1. set have dropped all references
> 2. userspace *can* delete the set
> 3. we get crash because xt_set was still within a sets eval
>   function.
> 
> I don't see how 3) can happen, xt table replace isn't supposed
> to call the xt_set destroy functions until after table replace.
> 
> We even release the entire x_table blob right afterwards.

I'd expect the author to send patches to netfilter-devel@...r.kernel.org 
first in order to review netfilter and ipset related patches there.

Best regards,
Jozsef
-
E-mail  : kadlec@...ckhole.kfki.hu, kadlecsik.jozsef@...ner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary