tcp: Fix uninitialized access in skb frags array for Rx 0cp.

From: Arjun Roy <[email protected]>

TCP Receive zerocopy iterates through the SKB queue via tcp_recv_skb(), acquiring a pointer to an SKB and an offset within that SKB to read from. From there, it iterates the SKB frags array to determine which offset to start remapping pages from.

However, this is built on the assumption that the offset read so far within the SKB is smaller than the SKB length. If this assumption is violated, we can attempt to read an invalid frags array element, which would cause a fault.

tcp_recv_skb() can cause such an SKB to be returned when the TCP FIN flag is set. Therefore, we must guard against this occurrence inside skb_advance_frag().

One way that we can reproduce this error follows:
1) In a receiver program, call getsockopt(TCP_ZEROCOPY_RECEIVE) with:
char some_array[32 * 1024];
struct tcp_zerocopy_receive zc = {
.copybuf_address = (__u64) &some_array[0],
.copybuf_len = 32 * 1024,
};

2) In a sender program, after a TCP handshake, send the following sequence of packets:
i) Seq = [X, X+4000]
ii) Seq = [X+4000, X+5000]
iii) Seq = [X+4000, X+5000], Flags = FIN | URG, urgptr=1000

(This can happen without URG, if we have a signal pending, but URG is a convenient way to reproduce the behaviour).

In this case, the following event sequence will occur on the receiver:

tcp_zerocopy_receive():
-> receive_fallback_to_copy() // copybuf_len >= inq
-> tcp_recvmsg_locked() // reads 5000 bytes, then breaks due to URG -> tcp_recv_skb() // yields skb with skb->len == offset -> tcp_zerocopy_set_hint_for_skb()
-> skb_advance_to_frag() // will returns a frags ptr. >= nr_frags -> find_next_mappable_frag() // will dereference this bad frags ptr.

With this patch, skb_advance_to_frag() will no longer return an invalid frags pointer, and will return NULL instead, fixing the issue.

Signed-off-by Arjun Roy <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

Fixes 05255b823a61 ("tcp: add TCP_ZEROCOPY_RECEIVE support for zerocopy receive")

---
 net/ipv4/tcp.c | 3 +++
 1 file changed, 3 insertions(+)

net/ipv4/tcp.c +3 -1

index bc7f419184aa..ef896847f190 100644
@@ -1741,6 +1741,9 @@ static skb_frag_t *skb_advance_to_frag(struct sk_buff *skb, u32 offset_skb,
 {
        skb_frag_t *frag;
 
+       if (unlikely(offset_skb >= skb->len))
+               return NULL;
+
        offset_skb -= skb_headlen(skb);
        if ((int)offset_skb < 0 || skb_has_frag_list(skb))
                return NULL;
--

2.34.0.rc1.387.gb447b232ab-goog

Quoted 70 lines · Arjun Roy

From: Arjun Roy <[email protected]>

TCP Receive zerocopy iterates through the SKB queue via tcp_recv_skb(), acquiring a pointer to an SKB and an offset within that SKB to read from. From there, it iterates the SKB frags array to determine which offset to start remapping pages from.

However, this is built on the assumption that the offset read so far within the SKB is smaller than the SKB length. If this assumption is violated, we can attempt to read an invalid frags array element, which would cause a fault.

tcp_recv_skb() can cause such an SKB to be returned when the TCP FIN flag is set. Therefore, we must guard against this occurrence inside skb_advance_frag().

One way that we can reproduce this error follows:
1) In a receiver program, call getsockopt(TCP_ZEROCOPY_RECEIVE) with:
char some_array[32 * 1024];
struct tcp_zerocopy_receive zc = {
.copybuf_address = (__u64) &some_array[0],
.copybuf_len = 32 * 1024,
};

2) In a sender program, after a TCP handshake, send the following sequence of packets:
i) Seq = [X, X+4000]
ii) Seq = [X+4000, X+5000]
iii) Seq = [X+4000, X+5000], Flags = FIN | URG, urgptr=1000

(This can happen without URG, if we have a signal pending, but URG is a convenient way to reproduce the behaviour).

In this case, the following event sequence will occur on the receiver:

tcp_zerocopy_receive():
-> receive_fallback_to_copy() // copybuf_len >= inq
-> tcp_recvmsg_locked() // reads 5000 bytes, then breaks due to URG -> tcp_recv_skb() // yields skb with skb->len == offset -> tcp_zerocopy_set_hint_for_skb()
-> skb_advance_to_frag() // will returns a frags ptr. >= nr_frags -> find_next_mappable_frag() // will dereference this bad frags ptr.

With this patch, skb_advance_to_frag() will no longer return an invalid frags pointer, and will return NULL instead, fixing the issue.

Signed-off-by Arjun Roy <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

Fixes 05255b823a61 ("tcp: add TCP_ZEROCOPY_RECEIVE support for zerocopy receive")

---
 net/ipv4/tcp.c | 3 +++
 1 file changed, 3 insertions(+)

net/ipv4/tcp.c +3 -1

index bc7f419184aa..ef896847f190 100644
@@ -1741,6 +1741,9 @@ static skb_frag_t *skb_advance_to_frag(struct sk_buff *skb, u32 offset_skb,
 {
        skb_frag_t *frag;
 
+       if (unlikely(offset_skb >= skb->len))
+               return NULL;
+
        offset_skb -= skb_headlen(skb);
        if ((int)offset_skb < 0 || skb_has_frag_list(skb))
                return NULL;
--

Quoted 2 lines

2.34.0.rc1.387.gb447b232ab-goog

Interestingly, netdevbpf list claims a netdev/build_32bit failure here:
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

But the v1 patch seemed to be fine (that one had a wrong "Fixes" tag, it's the only thing that changed in v2). Also, "make ARCH=i386" is working fine for me, and the significant amount of error output
(https://patchwork.hopto.org/static/nipa/578999/12615889/build_32bit/)
does not actually have any errors inside net/ipv4/tcp.c . I assume, then, this must be a tooling false positive, and I do not have to send a v3 (which would have no changes)?