netdev - Re: qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions

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]

Message-ID: <1517979275.3715.155.camel@gmail.com>
Date:   Tue, 06 Feb 2018 20:54:35 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Daniel Axtens <dja@...ens.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        netdev@...r.kernel.org
Subject: Re: qdisc_pkt_len_init: SCTP/GSO_BY_FRAGS and robustness questions

On Wed, 2018-02-07 at 12:15 +1100, Daniel Axtens wrote:
> Hi Marcelo and Eric,
> 
> I'm working on checking code that might be impacted by GSO_BY_FRAGS -
> after finding that the token bucket filter qdisc code doesn't handle it
> properly, DaveM said I should look for other places where this might be
> an issue [0].
> 
> I'm currently looking at qdisc_pkt_len_init in net/core/dev.c. This is
> called by __dev_queue_xmit, before validate_xmit_skb, so before an SCTP
> skb would be segmented if the hardware doesn't support SCTP offload.
> 
> There are two things I was hoping you two could offer some advice on:
> 
> 1) Eric, in 7c68d1a6b4db ("net: qdisc_pkt_len_init() should be more
>    robust") you replaced a chunk of code that is similar to the code
>    found in skb_gso_transport_seglen() and replaced it with more robust
>    code. Do we need to change skb_gso_transport_seglen() in a similar way?

I would prefer we get rid of DODGY ability to provide buggy packets.

It would be silly to 'fix' all the places in the kernel (like hundred
of drivers I guess), while we can simply validate packets at the time
they are provided by malicious sources (user space using
af_packet/tun/virtio_net ...)


> 
> 2) Marcelo, unlike skb_gso_transport_seglen(), where you added a case
>    for SCTP in 90017accff61 ("sctp: Add GSO support"), there doesn't
>    seem to be a GSO_BY_FRAGS or SCTP check in qdisc_pkt_len_init, so I
>    think the accounting is probably wrong for SCTP. I'm not 100% sure
>    how to fix this as it's now quite different from the calcuations in
>    skb_gso_transport_seglen() - so I was hoping that you might have an
>    idea.
> 
> Thanks in advance!
> 
> [0]: https://patchwork.ozlabs.org/patch/869145/#1852414
> 
> Regards,
> Daniel
> 
>