Re: [PATCH net] sctp: move the format error check out of __sctp_sf_do_9_1_abort

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Tue, Feb 18, 2020 at 12:34:52AM +0800, Xin Long wrote:
> When T2 timer is to be stopped, the asoc should also be deleted,
> otherwise, there will be no chance to call sctp_association_free
> and the asoc could last in memory forever.
> 
> However, in sctp_sf_shutdown_sent_abort(), after adding the cmd
> SCTP_CMD_TIMER_STOP for T2 timer, it may return error due to the
> format error from __sctp_sf_do_9_1_abort() and miss adding
> SCTP_CMD_ASSOC_FAILED where the asoc will be deleted.
> 
> This patch is to fix it by moving the format error check out of
> __sctp_sf_do_9_1_abort(), and do it before adding the cmd
> SCTP_CMD_TIMER_STOP for T2 timer.
> 
> Thanks Hangbin for reporting this issue by the fuzz testing.
> 
> Fixes: 96ca468b86b0 ("sctp: check invalid value of length parameter in error cause")
> Reported-by: Hangbin Liu <liuhangbin@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/sm_statefuns.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 748e3b1..e2b2b41 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -170,6 +170,16 @@ static inline bool sctp_chunk_length_valid(struct sctp_chunk *chunk,
>  	return true;
>  }
>  
> +/* Check for the format error in an ABORT chunk */

Can we just change this comment to:
+/* Check for format error in an ABORT chunk */
(just s/the //)
It is slightly misleading otherwise.

Rest of the patch LGTM.

> +static inline bool sctp_err_chunk_valid(struct sctp_chunk *chunk)
> +{
> +	struct sctp_errhdr *err;
> +
> +	sctp_walk_errors(err, chunk->chunk_hdr);
> +
> +	return (void *)err == (void *)chunk->chunk_end;
> +}
> +
>  /**********************************************************
>   * These are the state functions for handling chunk events.
>   **********************************************************/
> @@ -2255,6 +2265,9 @@ enum sctp_disposition sctp_sf_shutdown_pending_abort(
>  		    sctp_bind_addr_state(&asoc->base.bind_addr, &chunk->dest))
>  		return sctp_sf_discard_chunk(net, ep, asoc, type, arg, commands);
>  
> +	if (!sctp_err_chunk_valid(chunk))
> +		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
> +
>  	return __sctp_sf_do_9_1_abort(net, ep, asoc, type, arg, commands);
>  }
>  
> @@ -2298,6 +2311,9 @@ enum sctp_disposition sctp_sf_shutdown_sent_abort(
>  		    sctp_bind_addr_state(&asoc->base.bind_addr, &chunk->dest))
>  		return sctp_sf_discard_chunk(net, ep, asoc, type, arg, commands);
>  
> +	if (!sctp_err_chunk_valid(chunk))
> +		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
> +
>  	/* Stop the T2-shutdown timer. */
>  	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
>  			SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
> @@ -2565,6 +2581,9 @@ enum sctp_disposition sctp_sf_do_9_1_abort(
>  		    sctp_bind_addr_state(&asoc->base.bind_addr, &chunk->dest))
>  		return sctp_sf_discard_chunk(net, ep, asoc, type, arg, commands);
>  
> +	if (!sctp_err_chunk_valid(chunk))
> +		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
> +
>  	return __sctp_sf_do_9_1_abort(net, ep, asoc, type, arg, commands);
>  }
>  
> @@ -2582,16 +2601,8 @@ static enum sctp_disposition __sctp_sf_do_9_1_abort(
>  
>  	/* See if we have an error cause code in the chunk.  */
>  	len = ntohs(chunk->chunk_hdr->length);
> -	if (len >= sizeof(struct sctp_chunkhdr) + sizeof(struct sctp_errhdr)) {
> -		struct sctp_errhdr *err;
> -
> -		sctp_walk_errors(err, chunk->chunk_hdr);
> -		if ((void *)err != (void *)chunk->chunk_end)
> -			return sctp_sf_pdiscard(net, ep, asoc, type, arg,
> -						commands);
> -
> +	if (len >= sizeof(struct sctp_chunkhdr) + sizeof(struct sctp_errhdr))
>  		error = ((struct sctp_errhdr *)chunk->skb->data)->cause;
> -	}
>  
>  	sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNRESET));
>  	/* ASSOC_FAILED will DELETE_TCB. */
> -- 
> 2.1.0
>