Re: [PATCH] netfilter: conntrack: fix wrong ct->timeout value

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Apr 14, 2023 at 11:52:02AM +0800, Tzung-Bi Shih wrote:
> On Thu, Apr 13, 2023 at 11:04:24AM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Apr 13, 2023 at 11:23:11AM +0800, Tzung-Bi Shih wrote:
> > > nf_ct_expires() got called by userspace program.  And the return
> > > value (which means the remaining timeout) will be the parameter for
> > > the next ctnetlink_change_timeout().
> > 
> > Unconfirmed conntrack is owned by the packet that refers to it, it is
> > not yet in the hashes. I don't see how concurrent access to the
> > timeout might occur.
> > 
> > Or are you referring to a different scenario that triggers the partial
> > state?
> 
> I think it isn't a concurrent access.  We observed that userspace program
> reads the remaining timeout and sets it back for unconfirmed conntrack.
> 
> Conceptually, it looks like:
> timeout = nf_ct_expires(...);  (1)
> ctnetlink_change_timeout(...timeout);

How could this possibly happen?

nf_ct_expires() is called from:

- ctnetlink_dump_timeout(), this is netlink dump path, since:

commit 1397af5bfd7d32b0cf2adb70a78c9a9e8f11d912
Author: Florian Westphal <fw@strlen.de>
Date:   Mon Apr 11 13:01:18 2022 +0200

    netfilter: conntrack: remove the percpu dying list

it is not possible to do any listing of unconfirmed, and that is fine.

As I said, nfnetlink_queue operates with an unconfirmed conntrack with
owns exclusively, which is not in the hashes.

- nf_ct_expires() calls from xt and nft matches are irrelevant:

net/netfilter/nft_ct.c:         *dest = jiffies_to_msecs(nf_ct_expires(ct));
net/netfilter/xt_conntrack.c:           unsigned long expires = nf_ct_expires(ct) / HZ;

- there is the garbage collector, but that only works with conntrack
  entries in the hashes:

net/netfilter/nf_conntrack_core.c:                      expires = clamp(nf_ct_expires(tmp), GC_SCAN_INTERVAL_MIN, GC_SCAN_INTERVAL_CLAMP);

- there is also /proc interface, but that only works with conntrack
  entries in the hashes:

net/netfilter/nf_conntrack_standalone.c:                seq_printf(s, "%ld ", nf_ct_expires(ct)  / HZ);

> At (1), `nfct_time_stamp` is wrongly involved in the calculation[4] because
> the conntrack is unconfirmed.  The `ct->timeout` is an internal but not a
> timestamp.

I can see there are two possible states for ct->timeout, thanks for
explaining this again.

> As a result, ctnetlink_change_timeout() sets the wrong value to `ct->timeout`.
> 
> [4]: https://elixir.bootlin.com/linux/v6.3-rc6/source/include/net/netfilter/nf_conntrack.h#L296
> 
> > > As you can see in [4], if this happens on an unconfirmed conntrack, the
> > > `nfct_time_stamp` would be wrongly invoved in the calculation again.
> > > That's why we take care of all `ct->timeout` accesses in v1.
> 
> Again, that's why v1 separates all `ct->timeout` accesses.
> 
> If the conntrack is confirmed:
> - `ct->timeout` is a timestamp.
> - `nfct_time_stamp` should be involved when getting/setting `ct->timeout`.
> 
> If the conntrack is unconfirmed:
> - `ct->timeout` is an interval.
> - `nfct_time_stamp` shouldn't be involved.
> 
> Only separate `ct->timeout` access in __nf_ct_set_timeout() is obviously
> insufficient.  I would suggest either take care of all `ct->timeout`
> accesses as v1 or at least change both __nf_ct_set_timeout() and
> nf_ct_expires().

it does not even make sense to use WRITE_ONCE from
__nf_conntrack_confirm(), see:

https://elixir.bootlin.com/linux/latest/source/net/netfilter/nf_conntrack_core.c#L1260

because the unconfirmed conntrack object is owned exclusively by the packet.

> In the latter case, it looks fine in our environment.  However, I'm not
> sure if any hidden cases we haven't seen.

Maybe you have an old kernel?