[PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data corruption

[PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data corruption

[Dexuan Cui]

The v4.4 stable kernel lacks this bugfix:
commit 327868212381 ("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 327868212381,
but unluckily the bugfix is only backported to v4.9+. It turns out
backporting 327868212381 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: Eric Dumazet <edumazet@google.com>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 net/ipv4/udp.c | 3 +--
 net/ipv6/udp.c | 6 ++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index bb30699..49ab587 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1589,8 +1589,7 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		}
 	}
 
-	if (rcu_access_pointer(sk->sk_filter) &&
-	    udp_lib_checksum_complete(skb))
+	if (udp_lib_checksum_complete(skb))
 		goto csum_error;
 
 	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 73f1112..2d6703d 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -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))
-			goto csum_error;
-	}
+	if (udp_lib_checksum_complete(skb))
+		goto csum_error;
 
 	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
 		UDP6_INC_STATS_BH(sock_net(sk),
-- 
1.8.3.1

RE: [PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data corruption

[Dexuan Cui]

> From: Dexuan Cui <decui@microsoft.com>
> Sent: Monday, July 27, 2020 6:55 PM
> To: gregkh@linuxfoundation.org; edumazet@google.com;
> stable@vger.kernel.org
> Cc: w@1wt.eu; Dexuan Cui <decui@microsoft.com>; Joseph Salisbury
> <Joseph.Salisbury@microsoft.com>; Michael Kelley <mikelley@microsoft.com>;
> viro@zeniv.linux.org.uk; netdev@vger.kernel.org; davem@davemloft.net;
> ohering@suse.com
> Subject: [PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data
> corruption
> 
> The v4.4 stable kernel lacks this bugfix:
> commit 327868212381 ("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 327868212381,
> but unluckily the bugfix is only backported to v4.9+. It turns out
> backporting 327868212381 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: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  net/ipv4/udp.c | 3 +--
>  net/ipv6/udp.c | 6 ++----
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index bb30699..49ab587 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1589,8 +1589,7 @@ int udp_queue_rcv_skb(struct sock *sk, struct
> sk_buff *skb)
>  		}
>  	}
> 
> -	if (rcu_access_pointer(sk->sk_filter) &&
> -	    udp_lib_checksum_complete(skb))
> +	if (udp_lib_checksum_complete(skb))
>  		goto csum_error;
> 
>  	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 73f1112..2d6703d 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -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))
> -			goto csum_error;
> -	}
> +	if (udp_lib_checksum_complete(skb))
> +		goto csum_error;
> 
>  	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
>  		UDP6_INC_STATS_BH(sock_net(sk),
> --
> 1.8.3.1

+Sasha

This patch is targeted to the linux-4.4.y branch of the stable tree.

Thanks,
-- Dexuan

Re: [PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data corruption

[Sasha Levin]

On Fri, Aug 07, 2020 at 06:03:00PM +0000, Dexuan Cui wrote:
>> From: Dexuan Cui <decui@microsoft.com>
>> Sent: Monday, July 27, 2020 6:55 PM
>> To: gregkh@linuxfoundation.org; edumazet@google.com;
>> stable@vger.kernel.org
>> Cc: w@1wt.eu; Dexuan Cui <decui@microsoft.com>; Joseph Salisbury
>> <Joseph.Salisbury@microsoft.com>; Michael Kelley <mikelley@microsoft.com>;
>> viro@zeniv.linux.org.uk; netdev@vger.kernel.org; davem@davemloft.net;
>> ohering@suse.com
>> Subject: [PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data
>> corruption
>>
>> The v4.4 stable kernel lacks this bugfix:
>> commit 327868212381 ("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 327868212381,
>> but unluckily the bugfix is only backported to v4.9+. It turns out
>> backporting 327868212381 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: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Dexuan Cui <decui@microsoft.com>
>> ---
>>  net/ipv4/udp.c | 3 +--
>>  net/ipv6/udp.c | 6 ++----
>>  2 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index bb30699..49ab587 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -1589,8 +1589,7 @@ int udp_queue_rcv_skb(struct sock *sk, struct
>> sk_buff *skb)
>>  		}
>>  	}
>>
>> -	if (rcu_access_pointer(sk->sk_filter) &&
>> -	    udp_lib_checksum_complete(skb))
>> +	if (udp_lib_checksum_complete(skb))
>>  		goto csum_error;
>>
>>  	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index 73f1112..2d6703d 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -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))
>> -			goto csum_error;
>> -	}
>> +	if (udp_lib_checksum_complete(skb))
>> +		goto csum_error;
>>
>>  	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
>>  		UDP6_INC_STATS_BH(sock_net(sk),
>> --
>> 1.8.3.1
>
>+Sasha
>
>This patch is targeted to the linux-4.4.y branch of the stable tree.

Eric, will you ack this (or have a missed a previous ack)?

-- 
Thanks,
Sasha

Re: [PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data corruption

[Eric Dumazet]

On Wed, Aug 12, 2020 at 5:06 PM Sasha Levin <sashal@kernel.org> wrote:
>
> On Fri, Aug 07, 2020 at 06:03:00PM +0000, Dexuan Cui wrote:
> >> From: Dexuan Cui <decui@microsoft.com>
> >> Sent: Monday, July 27, 2020 6:55 PM
> >> To: gregkh@linuxfoundation.org; edumazet@google.com;
> >> stable@vger.kernel.org
> >> Cc: w@1wt.eu; Dexuan Cui <decui@microsoft.com>; Joseph Salisbury
> >> <Joseph.Salisbury@microsoft.com>; Michael Kelley <mikelley@microsoft.com>;
> >> viro@zeniv.linux.org.uk; netdev@vger.kernel.org; davem@davemloft.net;
> >> ohering@suse.com
> >> Subject: [PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data
> >> corruption
> >>
> >> The v4.4 stable kernel lacks this bugfix:
> >> commit 327868212381 ("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 327868212381,
> >> but unluckily the bugfix is only backported to v4.9+. It turns out
> >> backporting 327868212381 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: Eric Dumazet <edumazet@google.com>
> >> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> >> ---
> >>  net/ipv4/udp.c | 3 +--
> >>  net/ipv6/udp.c | 6 ++----
> >>  2 files changed, 3 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> >> index bb30699..49ab587 100644
> >> --- a/net/ipv4/udp.c
> >> +++ b/net/ipv4/udp.c
> >> @@ -1589,8 +1589,7 @@ int udp_queue_rcv_skb(struct sock *sk, struct
> >> sk_buff *skb)
> >>              }
> >>      }
> >>
> >> -    if (rcu_access_pointer(sk->sk_filter) &&
> >> -        udp_lib_checksum_complete(skb))
> >> +    if (udp_lib_checksum_complete(skb))
> >>              goto csum_error;
> >>
> >>      if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
> >> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> >> index 73f1112..2d6703d 100644
> >> --- a/net/ipv6/udp.c
> >> +++ b/net/ipv6/udp.c
> >> @@ -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))
> >> -                    goto csum_error;
> >> -    }
> >> +    if (udp_lib_checksum_complete(skb))
> >> +            goto csum_error;
> >>
> >>      if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
> >>              UDP6_INC_STATS_BH(sock_net(sk),
> >> --
> >> 1.8.3.1
> >
> >+Sasha
> >
> >This patch is targeted to the linux-4.4.y branch of the stable tree.
>
> Eric, will you ack this (or have a missed a previous ack)?

Sure, although I have already a Signed-off-by: tag on this one, since
I wrote this simpler fix for stable.

If needed :
Acked-by: Eric Dumazet <edumazet@google.com>

Thanks.

Re: [PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data corruption

[Sasha Levin]

On Wed, Aug 12, 2020 at 05:09:37PM -0700, Eric Dumazet wrote:
>On Wed, Aug 12, 2020 at 5:06 PM Sasha Levin <sashal@kernel.org> wrote:
>>
>> On Fri, Aug 07, 2020 at 06:03:00PM +0000, Dexuan Cui wrote:
>> >> From: Dexuan Cui <decui@microsoft.com>
>> >> Sent: Monday, July 27, 2020 6:55 PM
>> >> To: gregkh@linuxfoundation.org; edumazet@google.com;
>> >> stable@vger.kernel.org
>> >> Cc: w@1wt.eu; Dexuan Cui <decui@microsoft.com>; Joseph Salisbury
>> >> <Joseph.Salisbury@microsoft.com>; Michael Kelley <mikelley@microsoft.com>;
>> >> viro@zeniv.linux.org.uk; netdev@vger.kernel.org; davem@davemloft.net;
>> >> ohering@suse.com
>> >> Subject: [PATCH][for v4.4 only] udp: drop corrupt packets earlier to avoid data
>> >> corruption
>> >>
>> >> The v4.4 stable kernel lacks this bugfix:
>> >> commit 327868212381 ("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 327868212381,
>> >> but unluckily the bugfix is only backported to v4.9+. It turns out
>> >> backporting 327868212381 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: Eric Dumazet <edumazet@google.com>
>> >> Signed-off-by: Dexuan Cui <decui@microsoft.com>
>> >> ---
>> >>  net/ipv4/udp.c | 3 +--
>> >>  net/ipv6/udp.c | 6 ++----
>> >>  2 files changed, 3 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> >> index bb30699..49ab587 100644
>> >> --- a/net/ipv4/udp.c
>> >> +++ b/net/ipv4/udp.c
>> >> @@ -1589,8 +1589,7 @@ int udp_queue_rcv_skb(struct sock *sk, struct
>> >> sk_buff *skb)
>> >>              }
>> >>      }
>> >>
>> >> -    if (rcu_access_pointer(sk->sk_filter) &&
>> >> -        udp_lib_checksum_complete(skb))
>> >> +    if (udp_lib_checksum_complete(skb))
>> >>              goto csum_error;
>> >>
>> >>      if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
>> >> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> >> index 73f1112..2d6703d 100644
>> >> --- a/net/ipv6/udp.c
>> >> +++ b/net/ipv6/udp.c
>> >> @@ -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))
>> >> -                    goto csum_error;
>> >> -    }
>> >> +    if (udp_lib_checksum_complete(skb))
>> >> +            goto csum_error;
>> >>
>> >>      if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
>> >>              UDP6_INC_STATS_BH(sock_net(sk),
>> >> --
>> >> 1.8.3.1
>> >
>> >+Sasha
>> >
>> >This patch is targeted to the linux-4.4.y branch of the stable tree.
>>
>> Eric, will you ack this (or have a missed a previous ack)?
>
>Sure, although I have already a Signed-off-by: tag on this one, since
>I wrote this simpler fix for stable.
>
>If needed :
>Acked-by: Eric Dumazet <edumazet@google.com>

Ah, I see. Queued up, thanks!

-- 
Thanks,
Sasha