netdev - Re: [PATCH v2] sctp: do trace_sctp_probe after SACK validation and check

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] [day] [month] [year] [list]

Date:   Tue, 24 Dec 2019 10:16:42 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Kevin Kou <qdkevin.kou@...il.com>
Cc:     linux-sctp@...r.kernel.org, netdev@...r.kernel.org,
        vyasevich@...il.com, nhorman@...driver.com, davem@...emloft.net
Subject: Re: [PATCH v2] sctp: do trace_sctp_probe after SACK validation and
 check

On Tue, Dec 24, 2019 at 10:40:40AM +0000, Kevin Kou wrote:
> The function sctp_sf_eat_sack_6_2 now performs the Verification
> Tag validation, Chunk length validation, Bogu check, and also
> the detection of out-of-order SACK based on the RFC2960
> Section 6.2 at the beginning, and finally performs the further
> processing of SACK. The trace_sctp_probe now triggered before
> the above necessary validation and check.
> 
> this patch is to do the trace_sctp_probe after the chunk sanity
> tests, but keep doing trace if the SACK received is out of order,
> for the out-of-order SACK is valuable to congestion control
> debugging.
> 
> v1->v2:
>  - keep doing SCTP trace if the SACK is out of order as Marcelo's
>    suggestion.

Thanks, but you generated the v2 on top of v1. You need to generate
the patch as if v1 didn't get applied (because it is not going in).

For future reference, if it would be a follow-up, then it wouldn't be
a "v2"..

And you need to explicit tag the patch with 'net' or 'net-next',
indicating which tree you target. net is for fixes, net-next for
cleanups/new features. net-next is good here.

Thanks,
Marcelo

> 
> Signed-off-by: Kevin Kou <qdkevin.kou@...il.com>
> ---
>  net/sctp/sm_statefuns.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index b4a54df..d302a78 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -3298,6 +3298,15 @@ enum sctp_disposition sctp_sf_eat_sack_6_2(struct net *net,
>  	chunk->subh.sack_hdr = sackh;
>  	ctsn = ntohl(sackh->cum_tsn_ack);
>  
> +	/* If Cumulative TSN Ack beyond the max tsn currently
> +	 * send, terminating the association and respond to the
> +	 * sender with an ABORT.
> +	 */
> +	if (TSN_lte(asoc->next_tsn, ctsn))
> +		return sctp_sf_violation_ctsn(net, ep, asoc, type, arg, commands);
> +
> +	trace_sctp_probe(ep, asoc, chunk);
> +
>  	/* i) If Cumulative TSN Ack is less than the Cumulative TSN
>  	 *     Ack Point, then drop the SACK.  Since Cumulative TSN
>  	 *     Ack is monotonically increasing, a SACK whose
> @@ -3311,15 +3320,6 @@ enum sctp_disposition sctp_sf_eat_sack_6_2(struct net *net,
>  		return SCTP_DISPOSITION_DISCARD;
>  	}
>  
> -	/* If Cumulative TSN Ack beyond the max tsn currently
> -	 * send, terminating the association and respond to the
> -	 * sender with an ABORT.
> -	 */
> -	if (!TSN_lt(ctsn, asoc->next_tsn))
> -		return sctp_sf_violation_ctsn(net, ep, asoc, type, arg, commands);
> -
> -	trace_sctp_probe(ep, asoc, chunk);
> -
>  	/* Return this SACK for further processing.  */
>  	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_CHUNK(chunk));
>  
> -- 
> 1.8.3.1
>