netdev - Re: [PATCH net] sctp: update frag_point when stream

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: <20181126122903.GI3601@localhost.localdomain>
Date:   Mon, 26 Nov 2018 10:29:03 -0200
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem@...emloft.net, Neil Horman <nhorman@...driver.com>,
        Jakub Audykowicz <jakub.audykowicz@...il.com>
Subject: Re: [PATCH net] sctp: update frag_point when stream_interleave is set

On Mon, Nov 26, 2018 at 05:02:11PM +0800, Xin Long wrote:
> sctp_assoc_update_frag_point() should be called whenever asoc->pathmtu
> changes, but we missed one place in sctp_association_init(). It would
> cause frag_point is zero when sending data.
> 
> As says in Jakub's reproducer, if sp->pathmtu is set by socketopt, the
> new asoc->pathmtu inherits it in sctp_association_init(). Later when
> transports are added and their pmtu >= asoc->pathmtu, it will never
> call sctp_assoc_update_frag_point() to set frag_point.
> 
> This patch is to fix it by updating frag_point when stream_interleave
> is set in sctp_stream_interleave_init(), which is also called in
> sctp_association_init(). We're doing this also because frag_point
> is affected by datachunk's type, namely stream_interleave_0/1.
> 
> Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> Reported-by: Jakub Audykowicz <jakub.audykowicz@...il.com>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
>  net/sctp/stream_interleave.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> index 0a78cdf..19d596d 100644
> --- a/net/sctp/stream_interleave.c
> +++ b/net/sctp/stream_interleave.c
> @@ -1327,4 +1327,5 @@ void sctp_stream_interleave_init(struct sctp_stream *stream)
>  	asoc = container_of(stream, struct sctp_association, stream);
>  	stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
>  				       : &sctp_stream_interleave_0;
> +	sctp_assoc_update_frag_point(asoc);

I get that by adding it here we avoid adding it twice, one in
sctp_association_init and another in sctp_process_init, but here it is
out of context.

The decision on data chunk format is not made on this function but
higher in the stack and we can leverage that for sctp_process_init,
and for sctp_association_init, we should have it as close as possible
to where it initialized pathmtu and did not update the frag point.

>  }
> -- 
> 2.1.0
>