[PATCH net] xfrm: iptfs: do not seek the sequential skb reader backwards

[PATCH net] xfrm: iptfs: do not seek the sequential skb reader backwards

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

iptfs_reassem_cont() completes a saved runt by reading the missing bytes
at payload offset "data", and then reads from that same offset again to
fill the inner IP header, deliberately re-copying them.

skb_seq_read() is forward-only. Once a read has stepped past the current
segment of a non-linear skb, st->stepped_offset is larger than the offset
of the re-read, and

	*data = st->cur_skb->data + (abs_offset - st->stepped_offset);

underflows in unsigned arithmetic. The companion return value
"block_limit - abs_offset" stays large and positive, so
skb_copy_seq_read() memcpy()s up to iphlen - runtlen bytes from about
4GiB past the skb data.

  BUG: unable to handle page fault for address: ffffed1025327a04
  Oops: 0000 [#1] SMP KASAN
  RIP: 0010:kasan_check_range+0x1bd/0x220
  Call Trace:
   __asan_memcpy+0x23/0x60
   skb_copy_seq_read+0x13a/0x150
   iptfs_reassem_cont+0x363/0x1780
   iptfs_input_ordered+0x673/0x880
   iptfs_input+0x5f7/0xed0
   xfrm_input+0x332b/0x7960
   xfrm4_esp_rcv+0xb3/0x290
   ip_local_deliver+0x1c3/0x600

The sibling parser __input_process_payload() already restarts the walk
when the reader has stepped; do the same after the runt fill. While here,
pass 0 rather than "data" as the new lower bound in that sibling, since
every caller keeps handing skb_copy_seq_read() absolute payload offsets
and skb_seq_read() adds lower_offset to them.

Fixes: 075694765446 ("xfrm: iptfs: handle received fragmented inner packets")
Cc: stable@kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Assisted-by: tencentos-corvus-ai:hy4-preview
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
A KASAN reproducer for this issue is available if requested.

 net/xfrm/xfrm_iptfs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 597aedeac26e..c614f3483f78 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -845,6 +845,12 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
 		 */
 		memcpy(skb_put(newskb, runtlen), xtfs->ra_runt,
 		       sizeof(xtfs->ra_runt));
+
+		/* The re-read below cannot back up a stepped seq read. */
+		if (unlikely(st->stepped_offset)) {
+			skb_abort_seq_read(st);
+			skb_prepare_seq_read(skb, 0, skb->len, st);
+		}
 	}
 
 	/* Continue reassembling the packet */
@@ -1038,7 +1044,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
 			struct sk_buff *save = skbseq->root_skb;
 
 			skb_abort_seq_read(skbseq);
-			skb_prepare_seq_read(save, data, tail, skbseq);
+			skb_prepare_seq_read(save, 0, tail, skbseq);
 		}
 
 		if (first_skb) {
-- 
2.55.0

Re: [PATCH net] xfrm: iptfs: do not seek the sequential skb reader backwards

[Steffen Klassert]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
commit d3c6d0500e742002c13ba4e7b9eff45bdb2c394b
Author: Jun Yang <junvyyang@tencent.com>
xfrm: iptfs: do not seek the sequential skb reader backwards

iptfs_reassem_cont() can underflow its offset arithmetic when reading
a saved runt if the sequential reader stepped, leading to an out-of-bounds
memcpy. This patch aborts and restarts the sequence read to avoid backwards
seeks. It also fixes a sibling parser to use offset 0 when restarting.

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..751cceab738a1 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
[ ... ]
> @@ -1046,7 +1052,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  			struct sk_buff *save = skbseq->root_skb;
>  
>  			skb_abort_seq_read(skbseq);
> -			skb_prepare_seq_read(save, data, tail, skbseq);
> +			skb_prepare_seq_read(save, 0, tail, skbseq);
>  		}
>  
>  		if (first_skb) {

Does this code introduce a data race that corrupts reassembly?

When __input_process_payload() saves a runt, it calls iptfs_input_save_runt()
without acquiring xtfs->drop_lock or any other SA-level lock:

net/xfrm/xfrm_iptfs.c:__input_process_payload() {
    ...
    /* save the bytes we have, advance data and exit */
    iptfs_input_save_runt(xtfs, seq, hbytes, remaining);
    ...
}

This locklessly modifies xtfs->ra_runt, xtfs->ra_runtlen, and
xtfs->ra_wantseq.

Concurrently, another CPU processing packets for the same IP-TFS SA might
execute iptfs_input_ordered() and read xtfs->ra_runtlen locklessly, then
acquire drop_lock for iptfs_reassem_cont():

net/xfrm/xfrm_iptfs.c:iptfs_input_ordered() {
    ...
    /* check before locking i.e., maybe */
    if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {
        spin_lock(&xtfs->drop_lock);

        /* check again after lock */
        if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {
            data = iptfs_reassem_cont(xtfs, seq, &skbseq, skb, data,
                                      blkoff, &sublist);
        }
        spin_unlock(&xtfs->drop_lock);
    }
    ...
}

Because iptfs_input_save_runt() does not hold drop_lock, the lockless read
and locked memcpy of xtfs->ra_runt race directly with the lockless writes.
Can this cause memory corruption, torn reads, and packet corruption?

Can this code cause a lock context violation on PREEMPT_RT kernels?

In iptfs_input_ordered(), a sleepable spinlock is acquired while a
kmap_atomic mapping might still be held:

net/xfrm/xfrm_iptfs.c:iptfs_input_ordered() {
    ...
    if (skb_copy_seq_read(&skbseq, 0, ipth, sizeof(*ipth))) {
    ...
    if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {
        spin_lock(&xtfs->drop_lock);
    ...
}

If the packet is non-linear and the header resides in a paged fragment,
skb_copy_seq_read() maps the fragment using kmap_atomic() and leaves
skbseq.frag_data actively mapped.

Acquiring spin_lock(&xtfs->drop_lock) without first aborting the sequence
read violates lock context rules, as spinlock_t maps to a sleepable rt_mutex
on PREEMPT_RT.

Could this lead to a 'sleeping in atomic context' bug?