netfilter: nf_nat: race in nf_nat_setup_info() corrupts nat_bysource list (GPF / soft lockup)

netfilter: nf_nat: race in nf_nat_setup_info() corrupts nat_bysource list (GPF / soft lockup)

[Vimal Agrawal]

Hi,

Our custom module can decide to send the first packet of a conntrack
to userspace via NFQUEUE while the rest of the packets for that
conntrack follow the standard kernel forward path.
In the case of a bridge, br_flood() clones skbs, and those clones
share one unconfirmed conntrack for the initial packet(s) (via
skb_clone()'s __nf_copy(), which just bumps the refcount and copies
the same ct pointer).

br_flood() clones the skb; Skb1 and Skb2 share one unconfirmed ct:
  Skb1 (cpu1): -> nfqueue -> [userspace verdict] -> reinject -> SNAT
-> confirm (cpu2)
  Skb2 (cpu1): -> SNAT -> confirm (cpu1)
Skb1's reinject-triggered SNAT (cpu2) races Skb2's synchronous SNAT
(cpu1) on the same, still-unconfirmed ct.

Because of this, SNAT for the two skbs sharing the unconfirmed
conntrack races across two different CPUs. nf_nat_setup_info() sees on
both CPUs that the conntrack isn't yet SNAT-initialized, and both try
to initialize it — pushing the same conntrack node onto the
nat_bysource hash chain twice, corrupting the list.
We've seen two instances of this in production: a soft lockup and a
crash due to an illegal address, both while traversing the
nat_bysource chain. Backtrace from the GPF instance:
[321916.534145] [   T6617] general protection fault, probably for
non-canonical address 0x15000bffffffb6: 0000 [#1] SMP NOPTI
[321916.567351] [   T6617] CPU: 0 PID: 6617 Comm: snort Kdump: loaded
Tainted: P        W  O       6.6.49 #1
[321916.595688] [   T6617] Hardware name: Sophos XGS/XGS, BIOS V112 02/09/2022
[321916.615439] [   T6617] RIP: 0010:nf_nat_setup_info+0x8b6/0xba0 [nf_nat]
[321916.634338] [   T6617] Code: ed 75 15 e9 f2 fb ff ff 4d 8b ad 90
00 00 00 4d 85 ed 0f 84 e2 fb ff ff 49 81 ed 90 00 00 00 0f 84 d5 fb
ff ff 0f b6 44 24 56 <41> 38 45 46 75 d8 49 8b 45 20 49 8b 55 28 48 33
44 24 30 48 33 54
...
[321917.017416] [   T6617]  nf_nat_masquerade_ipv4+0x11d/0x200 [nf_nat]
[321917.035180] [   T6617]  ? do_accept6+0x150/0x150 [xt_SFOS_SNAT]
[321917.051796] [   T6617]  do_masq4+0x49/0x70 [xt_SFOS_SNAT]
[321917.066699] [   T6617]  ? do_accept6+0x150/0x150 [xt_SFOS_SNAT]
[321917.083312] [   T6617]  rule_lookup+0xc9/0x130 [sfos_rules_framework]
[321917.101646] [   T6617]  snat_tg+0x235/0x300 [xt_SFOS_SNAT]
[321917.116827] [   T6617]  ipt_do_table+0x2b9/0x410 [ip_tables]
[321917.132578] [   T6617]  nf_nat_inet_fn+0x14f/0x260 [nf_nat]
[321917.148044] [   T6617]  nf_nat_ipv4_out+0x4d/0xe0 [nf_nat]
[321917.163221] [   T6617]  nf_reinject+0x177/0x1d0
Disassembly of the faulting/looping instruction shows it's inside
find_appropriate_src()'s inlined hlist_for_each_entry_rcu() walk of
nf_nat_bysource[h] (called from get_unique_tuple(), itself called from
nf_nat_setup_info() before the vulnerable insert). Both are static,
single-call-site functions, so they're fully inlined and never appear
as separate symbols — which is why the crash attributes entirely to
nf_nat_setup_info:
373    hlist_for_each_entry_rcu(ct, &nf_nat_bysource[h], nat_bysource) {
   0x0000000000000daa <+2170>:  48 8b 15 00 00 00 00  mov    0x0(%rip),%rdx
   0x0000000000000db1 <+2177>:  89 c0                mov    %eax,%eax
   ...
357    return (t->dst.protonum == tuple->dst.protonum &&
   0x0000000000000de1 <+2225>:  0f b6 44 24 56        movzbl 0x56(%rsp),%eax
   0x0000000000000de6 <+2230>:  41 38 45 46           cmp    %al,0x46(%r13)
Basically, nf_nat_setup_info() is not CPU-race-safe against two
callers concurrently operating on the same, still-unconfirmed ct.

This kernel (6.6) already carries the fix for the analogous race at
confirm time — commit 3a1ce9793855935a2d2b4d9dc5003d42f797f6fc
("netfilter: conntrack: fix cloned unconfirmed skb->_nfct race in
__nf_conntrack_confirm"), which detects this exact
shared-unconfirmed-ct-via-clone precondition and drops the losing skb.
But nf_nat_setup_info() runs before confirm, on the same shared ct,
and has no equivalent guard — so the identical precondition
(bridge-flood/broadcast clones sharing one unconfirmed ct across CPUs)
is still exploitable earlier in the pipeline, at the NAT-setup stage.

We have not been able to catch the nf_nat_setup_info() double-insert
directly via kprobe so far — the window there is narrow. We do,
however, see the __nf_conntrack_confirm() WARN_ON_ONCE ("Another skb
with the same unconfirmed conntrack may win the race...") fire live in
production whenever there's a race for confirmation, which confirms
the underlying precondition — clones sharing one unconfirmed ct racing
across CPUs — is genuinely occurring on this box, not just inferred
from the crash dumps. We just haven't yet been able to force the
earlier, nf_nat_setup_info()-stage race into a controlled
reproduction.

I think this needs a fix in nf_nat_setup_info(). Wanted to get initial
feedback before I start working on a patch.

Thanks,
Vimal

Re: netfilter: nf_nat: race in nf_nat_setup_info() corrupts nat_bysource list (GPF / soft lockup)

[Florian Westphal]

Vimal Agrawal <avimalin@gmail.com> wrote:
> Our custom module can decide to send the first packet of a conntrack
> to userspace via NFQUEUE while the rest of the packets for that
> conntrack follow the standard kernel forward path.
> In the case of a bridge, br_flood() clones skbs, and those clones
> share one unconfirmed conntrack for the initial packet(s) (via
> skb_clone()'s __nf_copy(), which just bumps the refcount and copies
> the same ct pointer).
> 
> br_flood() clones the skb; Skb1 and Skb2 share one unconfirmed ct:
>   Skb1 (cpu1): -> nfqueue -> [userspace verdict] -> reinject -> SNAT
> -> confirm (cpu2)
>   Skb2 (cpu1): -> SNAT -> confirm (cpu1)
> Skb1's reinject-triggered SNAT (cpu2) races Skb2's synchronous SNAT
> (cpu1) on the same, still-unconfirmed ct.
> 
> Because of this, SNAT for the two skbs sharing the unconfirmed
> conntrack races across two different CPUs. nf_nat_setup_info() sees on
> both CPUs that the conntrack isn't yet SNAT-initialized, and both try
> to initialize it — pushing the same conntrack node onto the
> nat_bysource hash chain twice, corrupting the list.

Yes, conntrack assumes exclusive access, this is why nfqueue has all
these hacks to drop cloned skbs where we don't own the unconfirmed skb,
e.g. in nfqnl_enqueue_packet().

We could add more hacks but "fixing" nf_nat_setup_info() doesn't really
help, its only one of many possible races.

If you want to add more hacks, you could move:

        /* It's done. */
        if (maniptype == NF_NAT_MANIP_DST)
                ct->status |= IPS_DST_NAT_DONE;
        else
                ct->status |= IPS_SRC_NAT_DONE;

in nf_nat_setup_info to test_and_Set_bit() done while holding the
lock to add to unconfirmed list, failure causes drop.
But it won't solve the underlying design defects.