netdev - Re: [PATCHv2 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]

Date:   Tue, 27 Nov 2018 22:39:44 +0100
From:   Jakub Audykowicz <jakub.audykowicz@...il.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem@...emloft.net,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCHv2 net] sctp: update frag_point when stream_interleave is
 set

On 2018-11-27 12:11, 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 after asoc->pathmtu is
> set as sp->pathmtu in sctp_association_init(). Note that it moved them
> after sctp_stream_init(), as stream->si needs to be set first.
>
> Frag_point's calculation is also related with datachunk's type, so it
> needs to update frag_point when stream->si may be changed in
> sctp_process_init().
>
> v1->v2:
>   - call sctp_assoc_update_frag_point() separately in sctp_process_init
>     and sctp_association_init, per Marcelo's suggestion.
>
> 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/associola.c     | 7 ++++---
>  net/sctp/sm_make_chunk.c | 3 +++
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 6a28b96..dd77ec3 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -118,9 +118,6 @@ static struct sctp_association *sctp_association_init(
>  	asoc->flowlabel = sp->flowlabel;
>  	asoc->dscp = sp->dscp;
>  
> -	/* Initialize default path MTU. */
> -	asoc->pathmtu = sp->pathmtu;
> -
>  	/* Set association default SACK delay */
>  	asoc->sackdelay = msecs_to_jiffies(sp->sackdelay);
>  	asoc->sackfreq = sp->sackfreq;
> @@ -252,6 +249,10 @@ static struct sctp_association *sctp_association_init(
>  			     0, gfp))
>  		goto fail_init;
>  
> +	/* Initialize default path MTU. */
> +	asoc->pathmtu = sp->pathmtu;
> +	sctp_assoc_update_frag_point(asoc);
> +
>  	/* Assume that peer would support both address types unless we are
>  	 * told otherwise.
>  	 */
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 4a4fd19..f4ac6c5 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2462,6 +2462,9 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
>  			     asoc->c.sinit_max_instreams, gfp))
>  		goto clean_up;
>  
> +	/* Update frag_point when stream_interleave may get changed. */
> +	sctp_assoc_update_frag_point(asoc);
> +
>  	if (!asoc->temp && sctp_assoc_set_id(asoc, gfp))
>  		goto clean_up;
>  

For what it's worth, I can confirm this works as a fix for my issue:)