Re: [PATCH net] loopback: orphan zerocopy frags before releasing the sender in loopback_xmit()

[Willem de Bruijn]

Bjoern Doebel wrote:
> AF_PACKET PACKET_TX_RING transmission over the loopback interface can
> silently corrupt packet payloads in flight. tpacket_fill_skb() builds
> the transmit skb using zerocopy frags. loopback_xmit() then calls bare
> skb_orphan(), which runs skb->destructor (tpacket_destruct_skb()) and
> marks the ring slot TP_STATUS_AVAILABLE, telling userspace the buffer is
> reusable while the in-flight skb frags still reference that buffer.
> 
> This is ok if the packet gets processed immediately in loopback's xmit
> path before userspace gets a chance to reuse the frag buffer. However,
> if the packet gets redirected for instance to another CPU (via RPS),
> this opens a window where userspace may already write new data into the
> frag buffer before the receiver reads the original content.
> 
> Reproducer using txring_overwrite from the net:run_afpackettests selftest:
> 
>   ip netns add ns && ip -netns ns link set lo up
>   ip netns exec ns sh -c \
>       'echo 100 > /sys/class/net/lo/queues/rx-0/rps_cpus'
>   taskset -c 0 ip netns exec ns ./txring_overwrite
> 
> Commit 5cd8d46ea156 ("packet: copy user buffers before orphan or clone")
> is meant to trigger this copy from the skb_orphan_frags{_rx}() call
> sites, but loopback_xmit() calls bare skb_orphan() before any of them
> run. Address this by taking a kernel-private copy of the skb frags
> before going down the receive path.
> 
> Fixes: 5cd8d46ea156 ("packet: copy user buffers before orphan or clone")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Bjoern Doebel <doebel@xxxxxxxxx>
> Assisted-by: Kiro:claude-opus-5

Thanks for the report. We're receiving a number of these.

This issue is not limited to loopback. It is indeed possible to insert
skb_orphan_frags(_rx) statements before skb_orphan in specific
callsites.

But that is not sufficient in all cases, and a game of whack-a-mole
where we may miss instances.

I'm reviewing the options. The alternative, a deep fix in tx_ring,
appears to be non-trivial so not without risk itself.