[PATCH net] ipv4: remove warning in ip_recv_error

[PATCH net] ipv4: remove warning in ip_recv_error

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

A precondition check in ip_recv_error triggered on an otherwise benign
race. Remove the warning.

The warning triggers when passing an ipv6 socket to this ipv4 error
handling function. RaceFuzzer was able to trigger it due to a race
in setsockopt IPV6_ADDRFORM.

  ---
  CPU0
    do_ipv6_setsockopt
      sk->sk_socket->ops = &inet_dgram_ops;

  ---
  CPU1
    sk->sk_prot->recvmsg
      udp_recvmsg
        ip_recv_error
          WARN_ON_ONCE(sk->sk_family == AF_INET6);

  ---
  CPU0
    do_ipv6_setsockopt
      sk->sk_family = PF_INET;

This socket option converts a v6 socket that is connected to a v4 peer
to an v4 socket. It updates the socket on the fly, changing fields in
sk as well as other structs. This is inherently non-atomic. It races
with the lockless udp_recvmsg path.

No other code makes an assumption that these fields are updated
atomically. It is benign here, too, as ip_recv_error cares only about
the protocol of the skbs enqueued on the error queue, for which
sk_family is not a precise predictor (thanks to another isue with
IPV6_ADDRFORM).

Link: http://lkml.kernel.org/r/20180518120826.GA19515@dragonet.kaist.ac.kr
Fixes: ("7ce875e5ecb8 ipv4: warn once on passing AF_INET6 socket to ip_recv_error")
Reported-by: DaeRyong Jeong <threeearcat@gmail.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/ipv4/ip_sockglue.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 5ad2d8ed3a3f..57bbb060faaf 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -505,8 +505,6 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
 	int err;
 	int copied;
 
-	WARN_ON_ONCE(sk->sk_family == AF_INET6);
-
 	err = -EAGAIN;
 	skb = sock_dequeue_err_skb(sk);
 	if (!skb)
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH net] ipv4: remove warning in ip_recv_error

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed, 23 May 2018 14:29:52 -0400

> From: Willem de Bruijn <willemb@google.com>
> 
> A precondition check in ip_recv_error triggered on an otherwise benign
> race. Remove the warning.
> 
> The warning triggers when passing an ipv6 socket to this ipv4 error
> handling function. RaceFuzzer was able to trigger it due to a race
> in setsockopt IPV6_ADDRFORM.
 ...
> This socket option converts a v6 socket that is connected to a v4 peer
> to an v4 socket. It updates the socket on the fly, changing fields in
> sk as well as other structs. This is inherently non-atomic. It races
> with the lockless udp_recvmsg path.
> 
> No other code makes an assumption that these fields are updated
> atomically. It is benign here, too, as ip_recv_error cares only about
> the protocol of the skbs enqueued on the error queue, for which
> sk_family is not a precise predictor (thanks to another isue with
> IPV6_ADDRFORM).
> 
> Link: http://lkml.kernel.org/r/20180518120826.GA19515@dragonet.kaist.ac.kr
> Fixes: ("7ce875e5ecb8 ipv4: warn once on passing AF_INET6 socket to ip_recv_error")
> Reported-by: DaeRyong Jeong <threeearcat@gmail.com>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Applied and queued up for -stable.

The SHA1_ID doesn't go inside the (" ") of the Fixes tag, I fixed
it up this time.

Re: [PATCH net] ipv4: remove warning in ip_recv_error

[Willem de Bruijn]

On Thu, May 24, 2018 at 10:18 PM, David Miller <davem@davemloft.net> wrote:
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Wed, 23 May 2018 14:29:52 -0400
>
>> From: Willem de Bruijn <willemb@google.com>
>>
>> A precondition check in ip_recv_error triggered on an otherwise benign
>> race. Remove the warning.
>>
>> The warning triggers when passing an ipv6 socket to this ipv4 error
>> handling function. RaceFuzzer was able to trigger it due to a race
>> in setsockopt IPV6_ADDRFORM.
>  ...
>> This socket option converts a v6 socket that is connected to a v4 peer
>> to an v4 socket. It updates the socket on the fly, changing fields in
>> sk as well as other structs. This is inherently non-atomic. It races
>> with the lockless udp_recvmsg path.
>>
>> No other code makes an assumption that these fields are updated
>> atomically. It is benign here, too, as ip_recv_error cares only about
>> the protocol of the skbs enqueued on the error queue, for which
>> sk_family is not a precise predictor (thanks to another isue with
>> IPV6_ADDRFORM).
>>
>> Link: http://lkml.kernel.org/r/20180518120826.GA19515@dragonet.kaist.ac.kr
>> Fixes: ("7ce875e5ecb8 ipv4: warn once on passing AF_INET6 socket to ip_recv_error")
>> Reported-by: DaeRyong Jeong <threeearcat@gmail.com>
>> Suggested-by: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> Applied and queued up for -stable.
>
> The SHA1_ID doesn't go inside the (" ") of the Fixes tag, I fixed
> it up this time.

Thanks David. Sorry about that.

I'll send a checkpatch.pl patch to catch such typos myself
in the future. Something like

+# Check format of Fixes line
+               if ($in_commit_log && $line =~ /^\s*fixes:/i) {
+                       if ($line !~ /^fixes:\s[0-9a-f]{12,40}\s\(".*"\)$/i) {
+                               WARN("BAD_FIXES",
+                                    "Fixes tag is not of form
\"Fixes: <12+ chars of sha1> \(\"title line\"\)");
+                       }
+               }
+

though it seems that Fixes lines are expressly omitted from strict
commit style checking at the moment. I don't immediately see why.