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, 02 Nov 2017 11:24:06 -0700
From:   Christoph Paasch <cpaasch@...le.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] tcp: Always cleanup skb before sending

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.

> ->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?

> At xmit :
> 
> IPV4 uses ip_build_and_send_pkt() to transmit the SYNACK, so skb->cb[]
> is not used.
> 
> IPv6 uses other fields.
> 
> So I really wonder what exact issue you observed, please share your
> drugs ;)

No drugs, only belgian beer :)


Christoph