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);
