* [PATCH net] sctp: move the format error check out of __sctp_sf_do_9_1_abort
@ 2020-02-17 16:34 Xin Long
2020-02-18 3:48 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 2+ messages in thread
From: Xin Long @ 2020-02-17 16:34 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Neil Horman, Hangbin Liu
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 */
+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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH net] sctp: move the format error check out of __sctp_sf_do_9_1_abort
2020-02-17 16:34 [PATCH net] sctp: move the format error check out of __sctp_sf_do_9_1_abort Xin Long
@ 2020-02-18 3:48 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 2+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-02-18 3:48 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, Hangbin Liu
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-02-18 3:48 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-17 16:34 [PATCH net] sctp: move the format error check out of __sctp_sf_do_9_1_abort Xin Long
2020-02-18 3:48 ` Marcelo Ricardo Leitner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).