Commit 094b4522 authored by Dexuan Cui's avatar Dexuan Cui Committed by Kelsey Skunberg

udp: drop corrupt packets earlier to avoid data corruption

BugLink: https://bugs.launchpad.net/bugs/1892822

The v4.4 stable kernel lacks this bugfix:
commit 32786821 ("make skb_copy_datagram_msg() et.al. preserve ->msg_iter on error").
As a result, the v4.4 kernel can deliver corrupt data to the application
when a corrupt UDP packet is closely followed by a valid UDP packet: the
same invocation of the recvmsg() syscall can deliver the corrupt packet's
UDP payload to the application with the UDP payload length and the
"from IP/Port" of the valid packet.

Details:

For a UDP packet longer than 76 bytes (see the v5.8-rc6 kernel's
include/linux/skbuff.h:3951), Linux delays the UDP checksum verification
until the application invokes the syscall recvmsg().

In the recvmsg() syscall handler, while Linux is copying the UDP payload
to the application's memory, it calculates the UDP checksum. If the
calculated checksum doesn't match the received checksum, Linux drops the
corrupt UDP packet, and then starts to process the next packet (if any),
and if the next packet is valid (i.e. the checksum is correct), Linux
will copy the valid UDP packet's payload to the application's receiver
buffer.

The bug is: before Linux starts to copy the valid UDP packet, the data
structure used to track how many more bytes should be copied to the
application memory is not reset to what it was when the application just
entered the kernel by the syscall! Consequently, only a small portion or
none of the valid packet's payload is copied to the application's
receive buffer, and later when the application exits from the kernel,
actually most of the application's receive buffer contains the payload
of the corrupt packet while recvmsg() returns the length of the UDP
payload of the valid packet.

For the mainline kernel, the bug was fixed in commit 32786821,
but unluckily the bugfix is only backported to v4.9+. It turns out
backporting 32786821 to v4.4 means that some supporting patches
must be backported first, so the overall changes seem too big, so the
alternative is performs the csum validation earlier and drops the
corrupt packets earlier.
Signed-off-by: default avatarEric Dumazet <edumazet@google.com>
Signed-off-by: default avatarDexuan Cui <decui@microsoft.com>
Acked-by: default avatarEric Dumazet <edumazet@google.com>
Signed-off-by: default avatarSasha Levin <sashal@kernel.org>
Signed-off-by: default avatarKhalid Elmously <khalid.elmously@canonical.com>
Signed-off-by: default avatarIan May <ian.may@canonical.com>
Signed-off-by: default avatarKelsey Skunberg <kelsey.skunberg@canonical.com>
parent ddd9efb5
...@@ -1590,8 +1590,7 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) ...@@ -1590,8 +1590,7 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
} }
} }
if (rcu_access_pointer(sk->sk_filter) && if (udp_lib_checksum_complete(skb))
udp_lib_checksum_complete(skb))
goto csum_error; goto csum_error;
if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) { if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
......
...@@ -686,10 +686,8 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) ...@@ -686,10 +686,8 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
} }
} }
if (rcu_access_pointer(sk->sk_filter)) { if (udp_lib_checksum_complete(skb))
if (udp_lib_checksum_complete(skb)) goto csum_error;
goto csum_error;
}
if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) { if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
UDP6_INC_STATS_BH(sock_net(sk), UDP6_INC_STATS_BH(sock_net(sk),
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment