netdev - Re: [PATCH] ipv6: fix flow labels when the traffic class is non-0

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:   Mon, 30 Jan 2017 15:40:53 -0800
From:   Tom Herbert <tom@...bertland.com>
To:     Dimitris Michailidis <dmichail@...gle.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH] ipv6: fix flow labels when the traffic class is non-0

On Mon, Jan 30, 2017 at 2:09 PM, Dimitris Michailidis
<dmichail@...gle.com> wrote:
> ip6_make_flowlabel() determines the flow label for IPv6 packets. It's
> supposed to be passed a flow label, which it returns as is if non-0 and
> in some other cases, otherwise it calculates a new value.
>
> The problem is callers often pass a flowi6.flowlabel, which may also
> contain traffic class bits. If the traffic class is non-0
> ip6_make_flowlabel() mistakes the non-0 it gets as a flow label and
> returns the whole thing. Thus it can return a 'flow label' longer than
> 20b and the low 20b of that is typically 0 resulting in packets with 0
> label. Moreover, different packets of a flow may be labeled differently.
> For a TCP flow with ECN non-payload and payload packets get different
> labels as exemplified by this pair of consecutive packets:
>
> (pure ACK)
> Internet Protocol Version 6, Src: 2002:af5:11a3::, Dst: 2002:af5:11a2::
>     0110 .... = Version: 6
>     .... 0000 0000 .... .... .... .... .... = Traffic Class: 0x00 (DSCP: CS0, ECN: Not-ECT)
>         .... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
>         .... .... ..00 .... .... .... .... .... = Explicit Congestion Notification: Not ECN-Capable Transport (0)
>     .... .... .... 0001 1100 1110 0100 1001 = Flow Label: 0x1ce49
>     Payload Length: 32
>     Next Header: TCP (6)
>
> (payload)
> Internet Protocol Version 6, Src: 2002:af5:11a3::, Dst: 2002:af5:11a2::
>     0110 .... = Version: 6
>     .... 0000 0010 .... .... .... .... .... = Traffic Class: 0x02 (DSCP: CS0, ECN: ECT(0))
>         .... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
>         .... .... ..10 .... .... .... .... .... = Explicit Congestion Notification: ECN-Capable Transport codepoint '10' (2)
>     .... .... .... 0000 0000 0000 0000 0000 = Flow Label: 0x00000
>     Payload Length: 688
>     Next Header: TCP (6)
>
> This patch allows ip6_make_flowlabel() to be passed more than just a
> flow label and has it extract the part it really wants. This was simpler
> than modifying the callers. With this patch packets like the above become
>
> Internet Protocol Version 6, Src: 2002:af5:11a3::, Dst: 2002:af5:11a2::
>     0110 .... = Version: 6
>     .... 0000 0000 .... .... .... .... .... = Traffic Class: 0x00 (DSCP: CS0, ECN: Not-ECT)
>         .... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
>         .... .... ..00 .... .... .... .... .... = Explicit Congestion Notification: Not ECN-Capable Transport (0)
>     .... .... .... 1010 1111 1010 0101 1110 = Flow Label: 0xafa5e
>     Payload Length: 32
>     Next Header: TCP (6)
>
> Internet Protocol Version 6, Src: 2002:af5:11a3::, Dst: 2002:af5:11a2::
>     0110 .... = Version: 6
>     .... 0000 0010 .... .... .... .... .... = Traffic Class: 0x02 (DSCP: CS0, ECN: ECT(0))
>         .... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
>         .... .... ..10 .... .... .... .... .... = Explicit Congestion Notification: ECN-Capable Transport codepoint '10' (2)
>     .... .... .... 1010 1111 1010 0101 1110 = Flow Label: 0xafa5e
>     Payload Length: 688
>     Next Header: TCP (6)
>
> Signed-off-by: Dimitris Michailidis <dmichail@...gle.com>
> ---
>  include/net/ipv6.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 7afe991e900e..dbf0abba33b8 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -776,6 +776,11 @@ static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
>  {
>         u32 hash;
>
> +       /* @flowlabel may include more than a flow label, eg, the traffic class.
> +        * Here we want only the flow label value.
> +        */
> +       flowlabel &= IPV6_FLOWLABEL_MASK;
> +
Thanks for pointing out the issue, but I don't think this fix is quite
right. This would be discarding traffic class from being in the return
value. I can try to fix that.

Tom

>         if (flowlabel ||
>             net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
>             (!autolabel &&
> --
> 2.11.0.483.g087da7b7c-goog
>