netdev - Re: [PATCH net] tcp: Always cleanup skb before sending

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]

Date:   Thu, 2 Nov 2017 11:26:21 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Christoph Paasch <cpaasch@...le.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] tcp: Always cleanup skb before sending

On Thu, Nov 2, 2017 at 11:24 AM, Christoph Paasch <cpaasch@...le.com> wrote:
> Hi Eric,
>
> On 01/11/17 - 19:21:31, Eric Dumazet wrote:
>> On Wed, 2017-11-01 at 18:00 -0700, Eric Dumazet wrote:
>> > On Wed, 2017-11-01 at 17:10 -0700, Christoph Paasch wrote:
>> >
>> > > Yes, that looks good to me. Thanks!
>> > >
>> > > But we still need to clean up the skb in tcp_v4_send_reset and
>> > > tcp_v4_send_ack, as the fields (e.g., tcp_skb_cb->seq) have been set when
>> > > coming from tcp_v4_rcv.
>> >
>> > You might be confused : ip_send_unicast_reply() does not send back the
>> > incoming skb.
>> >
>> > A fresh skb is allocated, then appended/sent.
>> >
>> > And commit 24a2d43d8886f5a29c did the changes to provide to
>> > __ip_options_echo() the proper IPCB header location.
>> >
>>
>> More details :
>>
>> Fields written by tcp_init_nondata_skb() on the synack packet :
>>
>> ->seq          (32bits) at offset 0 of skb->cb[]
>> ->end_seq      (32bits) at offset 4 of skb->cb[]
>> ->tcp_gso_segs (16bits) at offset 8
>> ->tcp_flags    (8bits) at offset 12 value (TCPHDR_SYN | TCPHDR_ACK ->
>> 0x12)
>
> In my build I see tcp_flags at offset 16, because ktime_t is 64 bits.

Yes, but this ktime was removed in net-next for my rb-tree work.

>
>> ->sacked       (8bits) at offset 13
>>
>> IPCB fields sharing these 14 bytes :
>>
>> iif  /* 32bits, offset 0 */
>> opt.faddr    (32bits) offset 4
>> opt.nexthop  (32bits) offset 8 /* value 1 */
>> opt.optlen   (8bits) offset 12 /* value 0x12 */
>> opt.srr      (8bits) offset 13
>>
>> IP6CB fields sharing these 14 bytes :
>>
>> iif   /* 32bits, offset 0 */
>> ra    /* 16 bits, offset 4 */
>> dst0  /* 16 bits offset 6 */
>> srcrt /* 16 bits offset 8 */  -> 0x0001
>> dst1  /* 16 bits offset 10 */ (not mangled -> 0)
>> lastopt /* 16 bits offset 12 */  -> 0x12
>
> Thus, because what I mention above, we are writing here into flags which sits
> at offset 16.
>
> So, IP6SKB_FORWARDED and IP6SKB_FRAGMENTED will be set on the outgoing skb.
> Especially for IP6SKB_FORWARDED, which is checked at ip6_finish_output2() I
> am concerned. Even in general, having these fields set looks brittle to me.
>
> What do you think?

I think I will submit my patch, which should clear the issue, without
breaking IPv4 options handling as your patch did ;)

Thanks !