netdev - Re: [4.9.13] use after free in ipv4

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMVG2styKvhUkVnhkFJ-B427sgnbxsBN8rJfE=wFd3wtXYXvrg@mail.gmail.com>
Date:   Mon, 6 Mar 2017 14:33:19 +0800
From:   Daniel J Blueman <daniel@...ra.org>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Stephen Hemminger <shemminger@...ux-foundation.org>
Subject: Re: [4.9.13] use after free in ipv4_mtu

On 2 March 2017 at 21:28, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Thu, 2017-03-02 at 05:08 -0800, Eric Dumazet wrote:
>
>> Thanks for the report !
>>
>> This patch should solve this precise issue, but we need more work.
>>
>> We need to audit all __sk_dst_get() and make sure they are inside an
>> rcu_read_lock()/rcu_read_unlock() section.
>>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> index 22548b5f05cbe5a655e0c53df2d31c5cc2e8a702..517963e1cb6eb9d70fcd71f44262813c3378759f 100644
>> --- a/net/ipv4/tcp_output.c
>> +++ b/net/ipv4/tcp_output.c
>> @@ -1459,7 +1459,7 @@ EXPORT_SYMBOL(tcp_sync_mss);
>>  unsigned int tcp_current_mss(struct sock *sk)
>>  {
>>       const struct tcp_sock *tp = tcp_sk(sk);
>> -     const struct dst_entry *dst = __sk_dst_get(sk);
>> +     const struct dst_entry *dst;
>>       u32 mss_now;
>>       unsigned int header_len;
>>       struct tcp_out_options opts;
>> @@ -1467,11 +1467,14 @@ unsigned int tcp_current_mss(struct sock *sk)
>>
>>       mss_now = tp->mss_cache;
>>
>> +     rcu_read_lock();
>> +     dst = __sk_dst_get(sk);
>>       if (dst) {
>>               u32 mtu = dst_mtu(dst);
>>               if (mtu != inet_csk(sk)->icsk_pmtu_cookie)
>>                       mss_now = tcp_sync_mss(sk, mtu);
>>       }
>> +     rcu_read_unlock();
>>
>>       header_len = tcp_established_options(sk, NULL, &opts, &md5) +
>>                    sizeof(struct tcphdr);
>
> Normally TCP sockets sk_dst_cache can only be changed if the thread
> doing the change owns the socket.
>
> I have not yet understood which point was breaking the rule yet.

Great work Eric! I have been unable to reproduce the KASAN warning
with this patch.

Reported-by: Daniel J Blueman <daniel@...ra.org>
Tested-by: Daniel J Blueman <daniel@...ra.org>

I do change the network queueing discipline and related at runtime [1]
which may be triggering this, though I did think I saw the KASAN
report only after resuming from suspend. rf(un)kill and other tweaking
may have been involved too.

Thanks,
  Dan

[1] /etc/sysctl.d/90-tcp.conf

net.core.default_qdisc = fq_codel
net.ipv4.tcp_congestion_control = bbr
net.ipv4.tcp_slow_start_after_idle = 0
net.ipv4.tcp_ecn = 1
-- 
Daniel J Blueman