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: <20231005123107.GB9350@breakpoint.cc>
Date: Thu, 5 Oct 2023 14:31:07 +0200
From: Florian Westphal <fw@...len.de>
To: xiaolinkui <xiaolinkui@....com>
Cc: pablo@...filter.org, kadlec@...filter.org, fw@...len.de,
	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

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.

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.

Powered by blists - more mailing lists