netdev - Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT

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: <20201106102106.GB3556@localhost.localdomain>
Date:   Fri, 6 Nov 2020 07:21:06 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Petr Malat <oss@...at.biz>
Cc:     linux-sctp@...r.kernel.org, Vlad Yasevich <vyasevich@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sctp: Fix sending when PMTU is less than
 SCTP_DEFAULT_MINSEGMENT

On Fri, Nov 06, 2020 at 10:48:24AM +0100, Petr Malat wrote:
> On Fri, Nov 06, 2020 at 05:46:34AM -0300, Marcelo Ricardo Leitner wrote:
> > On Thu, Nov 05, 2020 at 11:39:47AM +0100, Petr Malat wrote:
> > > Function sctp_dst_mtu() never returns lower MTU than
> > > SCTP_TRUNC4(SCTP_DEFAULT_MINSEGMENT) even when the actual MTU is less,
> > > in which case we rely on the IP fragmentation and must enable it.
> > 
> > This should be being handled at sctp_packet_will_fit():
> 
> sctp_packet_will_fit() does something a little bit different, it
> allows fragmentation, if the packet must be longer than the pathmtu
> set in SCTP structures, which is never less than 512 (see
> sctp_dst_mtu()) even when the actual mtu is less than 512.
> 
> One can test it by setting mtu of an interface to e.g. 300,
> and sending a longer packet (e.g. 400B):
> >           psize = packet->size;
> >           if (packet->transport->asoc)
> >                   pmtu = packet->transport->asoc->pathmtu;
> >           else
> >                   pmtu = packet->transport->pathmtu;
> here the returned pmtu will be 512

Thing is, your patch is using the same vars to check for it:
+       pmtu = tp->asoc ? tp->asoc->pathmtu : tp->pathmtu;

> 
> > 
> >           /* Decide if we need to fragment or resubmit later. */
> >           if (psize + chunk_len > pmtu) {
> This branch will not be taken as the packet length is less then 512

Right, ok. While then your patch will catch it because pmtu will be
SCTP_DEFAULT_MINSEGMENT, as it is checking with '<='.

> 
> >            }
> > 
> And the whole function will return SCTP_XMIT_OK without setting
> ipfragok.
> 
> I think the idea of never going bellow 512 in sctp_dst_mtu() is to
> reduce overhead of SCTP headers, which is fine, but when we do that,
> we must be sure to allow the IP fragmentation, which is currently
> missing.

Hmm. ip frag is probably just worse than higher header/payload
overhead.

> 
> The other option would be to keep track of the real MTU in pathmtu
> and perform max(512, pathmtu) in sctp_packet_will_fit() function.

I need to check where this 512 came from. I don't recall it from top
of my head and it's from before git history. Maybe we should just drop
this limit, if it's artificial. IPV4_MIN_MTU is 68.

> 
> Not sure when exactly this got broken, but using MTU less than 512
> used to work in 4.9.

Uhh, that's a bit old already. If you could narrow it down, that would
be nice.

  Marcelo