On 3/19/20 9:40 AM, Eric Dumazet wrote:
>
>
> On 3/19/20 3:52 AM, Florian Westphal wrote:
>> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>>> On Thu, Mar 19, 2020 at 11:34:38AM +0100, Florian Westphal wrote:
>>>> Martin Zaharinov <micron10@xxxxxxxxx> 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);
