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

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

[Bjoern Doebel]

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@vger.kernel.org
Signed-off-by: Bjoern Doebel <doebel@amazon.de>
Assisted-by: Kiro:claude-opus-5
---
Verified that the reproducer in the commit message fails 100% right now
and no longer fails after the patch.
---
 drivers/net/loopback.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 1fb6ce6843ade..31ae10a9d7911 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -72,6 +72,23 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
 {
 	int len;
 
+	/* The skb_orphan() below will run the skb's destructor, which
+	 * for AF_PACKET TX-ring senders marks the slot as TP_STATUS_AVAILABLE
+	 * again, even though it still has zerocopy frags pointing to it that
+	 * will only be copied later in the receive path's
+	 * skb_orphan_frags_rx(). As such, if the receive path gets deferred,
+	 * for example by RPS steering the packet to another CPU, this creates
+	 * a race where userspace may fill in new data into the frag before the
+	 * old data gets copied out.
+	 *
+	 * Take a kernel-private copy.
+	 */
+	if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC))) {
+		dev_core_stats_tx_dropped_inc(dev);
+		kfree_skb_reason(skb, SKB_DROP_REASON_SKB_UCOPY_FAULT);
+		return NETDEV_TX_OK;
+	}
+
 	skb_tx_timestamp(skb);
 
 	/* do not fool net_timestamp_check() with various clock bases */
-- 
2.50.1

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@vger.kernel.org
> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> 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.

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] Incomplete fix: the bug class the commit claims to close
  (AF_PACKET PACKET_TX_RING zerocopy frags being released to userspace…
- [Low] No in-tree test pins down the newly fixed behaviour.

--

loopback: orphan zerocopy frags before releasing the sender in loopback_xmit()

This adds a skb_orphan_frags_rx() call at the top of loopback_xmit() so that
AF_PACKET PACKET_TX_RING zerocopy frags are copied into kernel memory before
the bare skb_orphan() runs tpacket_destruct_skb() and returns the ring slot to
userspace.  On allocation failure the skb is dropped and the tx_dropped
counter is bumped.

> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index 1fb6ce6843ad..31ae10a9d791 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -72,6 +72,23 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>  {
>  	int len;
>  
> +	/* The skb_orphan() below will run the skb's destructor, which
> +	 * for AF_PACKET TX-ring senders marks the slot as TP_STATUS_AVAILABLE
> +	 * again, even though it still has zerocopy frags pointing to it that
> +	 * will only be copied later in the receive path's
> +	 * skb_orphan_frags_rx(). As such, if the receive path gets deferred,
> +	 * for example by RPS steering the packet to another CPU, this creates
> +	 * a race where userspace may fill in new data into the frag before the
> +	 * old data gets copied out.
> +	 *
> +	 * Take a kernel-private copy.
> +	 */
> +	if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC))) {
> +		dev_core_stats_tx_dropped_inc(dev);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_SKB_UCOPY_FAULT);
> +		return NETDEV_TX_OK;
> +	}
> +

[Severity: High]
Is loopback_xmit() the only local reinjection site with this pattern?
vrf_local_xmit() in drivers/net/vrf.c looks like the same sequence, and its
comment even says it reinserts "similar to loopback handling":

drivers/net/vrf.c:vrf_local_xmit() {
	unsigned int len = skb->len;

	skb_orphan(skb);

	skb_dst_set(skb, dst);
	...
	if (likely(__netif_rx(skb) == NET_RX_SUCCESS))
		dev_dstats_rx_add(dev, len);
	...
}

There is no skb_orphan_frags_rx() before that skb_orphan(), and the path is
reachable from an AF_PACKET TX-ring send on a VRF device:

  tpacket_fill_skb()      /* skb_zcopy_set_nouarg(skb, ph.raw) */
    -> dev_queue_xmit()
      -> vrf_xmit()
        -> is_ip_tx_frame()
          -> vrf_process_v4_outbound()   /* rt->dst.dev == vrf_dev */
            -> vrf_local_xmit()

vrf_process_v4_outbound() only pulls ETH_HLEN + sizeof(struct iphdr), so the
payload stays in the user pages, and __netif_rx() can again defer the read to
another CPU through RPS/backlog after skb_orphan() has already marked the slot
TP_STATUS_AVAILABLE.

Would it make sense to mirror this hunk in vrf_local_xmit(), or to do the copy
once in the common receive-injection path (netif_rx_internal() /
enqueue_to_backlog()) so that all local reinjection sites are covered?

>  	skb_tx_timestamp(skb);
>  
>  	/* do not fool net_timestamp_check() with various clock bases */

Regarding the reproducer in the changelog:

>       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

[Severity: Low]
Could the rps_cpus and taskset setup be added to the in-tree harness as well?

tools/testing/selftests/net/txring_overwrite.c binds the TX ring to "lo" in
setup_tx() and main() sends the two frames through the reused slot, but
nothing configures rx deferral:

tools/testing/selftests/net/run_afpackettests:
	./in_netns.sh ./txring_overwrite

tools/testing/selftests/net/in_netns.sh:setup() {
	ip netns add "${NETNS}"
	ip -netns "${NETNS}" link set lo up
}

Without the rps_cpus mask and the CPU pinning, __netif_rx() consumes the frags
in line on the sending CPU, so txring_overwrite passes both with and without
the new skb_orphan_frags_rx() call in loopback_xmit().  As it stands, removing
that call again would not make any in-tree test fail.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909085542.3370986-1-doebel%40amazon.de