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

[Eric Dumazet <eric.dumazet@gmail.com>]

On 3/19/20 3:52 AM, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> On Thu, Mar 19, 2020 at 11:34:38AM +0100, Florian Westphal wrote:
>>> Martin Zaharinov <micron10@gmail.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);