Re: [PATCH nf v8 0/3] netfilter: bound asynchronous packet duplication

[Florian Westphal <fw@strlen.de>]

Zihan Xi <zihanx@nebusec.ai> wrote:
> Hi Linux kernel maintainers,
> 
> We found and validated an issue in netfilter packet duplication. The bug
> is reachable by an unprivileged user in user and network namespaces.
> Bounded tests exercised the fixes. Non-duplication packet semantics are
> intended to remain unchanged. MIRRED flow offload for affected netdev dup
> rules is rejected because it cannot enforce the duplication-depth limit.
> 
> We will provide detailed information about the bug
> in this email, along with a PoC to trigger it.

Series makes no sense to me, sorry.  I dislike the 2nd patch, I see no
reason why we need to make changes all over the place for this...?

Also, if we go with 2nd patch, why do we still need the first?

> The IPv4 and IPv6 duplication helpers use a task-local guard while a cloned
> packet traverses output hooks. If NFQUEUE suspends that traversal, the
> output call can return and clear the guard before a later verdict resumes
> the remaining hooks. The resumed packet can reach another duplication
> consumer without the original guard. This can cause repeated packet
> processing and consume CPU and network resources.

"Don't do that, then"?

I think the kernel should not crash when misused, but I don't think
kernel should have to do extreme stunts to detect such loops.

There are probably many other ways to make kernel allocate lots of
memory or waste cpu cycles.

E.g. via create use of veth and bridges, sched plug, teql, ...

> Patch 1 records the
> guard in the queue entry and restores it during reinjection. It keeps
> bottom halves disabled during the reinjection traversal, restoring the
> verdict task state before reenabling them.

Its small enough, so I have no objections to this patch.

> The netdev nft dup path has a separate synchronous transmit recursion
> counter. It does not accompany a duplicated skb through asynchronous
> software receive paths. Patch 2 carries a bounded duplication depth through
> skb and supported XDP conversions, resets it on reused buffers, and rejects
> MIRRED flow offload where the software depth cannot be enforced. This scope
> covers software skb/XDP continuity, not packet loops through physical
> hardware or userspace device round trips.

I think it would be better to first go to the drawing board and make ONE
facility that governs the recursion part.

From a quick glance, the difference is historical from time when
per-module pcpu counters were used, so nf_dup netdev could not see the state
of nft_dup toggle.  Now that its in task struct anyway, maybe its time to
just zap the egress recursion counter and first make a patch that
uses the bool toggle for both egress and normal dup?

I would like to remove code, not add more.

> At PRE_ROUTING and LOCAL_IN, the IPv4 and IPv6 helpers decrement TTL or
> hop limit on the copy. Patch 3 stops creating copies when the value is
> <= 1, so the decrement cannot reach zero or wrap a zero value to the
> maximum.

Thats unrelated problem.  There are other places that do TTL/HL
decrements.  I think it makes sense to clean this up in -next and
add common helpers and then use those.