netdev - Re: [PATCH net] sctp: fix handling of ICMP Frag Needed for too small MTUs

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:   Wed, 3 Jan 2018 15:31:00 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        Vlad Yasevich <vyasevich@...il.com>,
        Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCH net] sctp: fix handling of ICMP Frag Needed for too small MTUs

On Wed, Jan 3, 2018 at 5:44 AM, Marcelo Ricardo Leitner
<marcelo.leitner@...il.com> wrote:
> syzbot reported a hang involving SCTP, on which it kept flooding dmesg
> with the message:
> [  246.742374] sctp: sctp_transport_update_pmtu: Reported pmtu 508 too
> low, using default minimum of 512
>
> That happened because whenever SCTP hits an ICMP Frag Needed, it tries
> to adjust to the new MTU and triggers an immediate retransmission. But
> it didn't consider the fact that MTUs smaller than the SCTP minimum MTU
> allowed (512) would not cause the PMTU to change, and issued the
> retransmission anyway (thus leading to another ICMP Frag Needed, and so
> on).
>
> The fix is to disable Path MTU discovery for such transport and to skip
> the retransmission in such cases. By doing this, SCTP will do the
> backoff retransmissions as needed and will likely switch to another
> transport if available.
>
> See-also: https://lkml.org/lkml/2017/12/22/811
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> ---
>  net/sctp/input.c     | 5 ++++-
>  net/sctp/transport.c | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 621b5ca3fd1c17c3d7ef7bb1c7677ab98cebbe77..a24658c6f181e03d85f12dbe929c8bb4abaefcbd 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -412,8 +412,11 @@ void sctp_icmp_frag_needed(struct sock *sk, struct sctp_association *asoc,
>          * Needed will never be sent, but if a message was sent before
>          * PMTU discovery was disabled that was larger than the PMTU, it
>          * would not be fragmented, so it must be re-transmitted fragmented.
> +        * If the new PMTU is invalid, we will keep getting ICMP Frag
> +        * Needed. In this case, simply avoid the retransmit.
>          */
> -       sctp_retransmit(&asoc->outqueue, t, SCTP_RTXR_PMTUD);
> +       if (pmtu >= SCTP_DEFAULT_MINSEGMENT)
> +               sctp_retransmit(&asoc->outqueue, t, SCTP_RTXR_PMTUD);
>  }
>
>  void sctp_icmp_redirect(struct sock *sk, struct sctp_transport *t,
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 1e5a22430cf56e40a6f323081beb97836b506384..fbd9fe25764d4d98f93c60a48eccefd9cc6b4165 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -259,6 +259,8 @@ void sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu)
>                  * pmtu discovery on this transport.
>                  */
>                 t->pathmtu = SCTP_DEFAULT_MINSEGMENT;
> +               t->param_flags = (t->param_flags & ~SPP_PMTUD) |
> +                                SPP_PMTUD_DISABLE;
It seems that once it hits here,  this transport will have the minimum pmtu
forever, even after t->dst has expired. It means this tx path will not come
back to normal any more even when it gets a needfrag with reasonable
pmtu.  is it too harsh to this transport ?

Another thing is on sctp_sendmsg, it also checks pmtu_pending that may
be set by needfrag, and goes to sctp_assoc_sync_pmtu to trigger this
warning again.

>         } else {
>                 t->pathmtu = pmtu;
>         }
> --
> 2.14.3
>