netdev - Re: Bug URGENT Report with new kernel 5.5.10-5.6-rc6

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] [day] [month] [year] [list]

Message-ID: <4d9f339c-b0a7-1861-7d76-e0f2cee92b8c@gmail.com>
Date:   Thu, 19 Mar 2020 09:45:38 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Florian Westphal <fw@...len.de>,
        Pablo Neira Ayuso <pablo@...filter.org>
Cc:     Martin Zaharinov <micron10@...il.com>,
        netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: Bug URGENT Report with new kernel 5.5.10-5.6-rc6



On 3/19/20 9:40 AM, Eric Dumazet wrote:
> 
> 
> On 3/19/20 3:52 AM, Florian Westphal wrote:
>> Pablo Neira Ayuso <pablo@...filter.org> wrote:
>>> On Thu, Mar 19, 2020 at 11:34:38AM +0100, Florian Westphal wrote:
>>>> Martin Zaharinov <micron10@...il.com> wrote:
>>>>
>>>> [ trimming CC ]
>>>>
>>>> Please revert
>>>>
>>>> commit 28f8bfd1ac948403ebd5c8070ae1e25421560059
>>>> netfilter: Support iif matches in POSTROUTING
>>>
>>> Please, specify a short description to append to the revert.
>>
>> TCP makes use of the rb_node in sk_buff for its retransmit queue,
>> amongst others.
> 
> 
> Only for master skbs kept in TCP internal queues (rtx rb tree)
> 
> However the packets leaving TCP stack are clones.
> 
>   skb->dev aliases to this storage, i.e., passing
>> skb->dev as the input interface in postrouting may point to another
>> sk_buff instead.
>> This will cause crashes and data corruption with nf_queue, as we will
>> attempt to increment a random pcpu variable when calling dev_hold().
>>
>> Also, the memory address may also be free'd, which gives UAF splat.
>>
> 
> This seems to suggest clones skb->dev should be cleared before leaving TCP stack,
> if some layer is confused because skb->dev has not yet been set by IP layer ?
> 
> Untested patch :
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 306e25d743e8de1bfe23d6e3b3a9fb0f23664912..c40fb3880307aa3156d01a8b49f1296657346cfd 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1228,6 +1228,7 @@ static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb,
>         /* Cleanup our debris for IP stacks */
>         memset(skb->cb, 0, max(sizeof(struct inet_skb_parm),
>                                sizeof(struct inet6_skb_parm)));
> +       skb->dev = NULL;
>  
>         tcp_add_tx_delay(skb, tp);
>  
> 

Or clear the field only after cloning :

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 306e25d743e8de1bfe23d6e3b3a9fb0f23664912..13dd0d8003baee3febcfb85df84421f8f91132ef 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1109,6 +1109,7 @@ static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb,
 
                if (unlikely(!skb))
                        return -ENOBUFS;
+               skb->dev = NULL;
        }
 
        inet = inet_sk(sk);