* [PATCHv2 net] sctp: move the format error check out of __sctp_sf_do_9_1_abort
@ 2020-02-18 4:07 Xin Long
2020-02-18 4:18 ` Marcelo Ricardo Leitner
2020-02-18 5:57 ` David Miller
0 siblings, 2 replies; 3+ messages in thread
From: Xin Long @ 2020-02-18 4:07 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.
v1->v2:
- improve the comment in the code as Marcelo's suggestion.
Fixes: 96ca468b86b0 ("sctp: check invalid value of length parameter in error cause")
Reported-by: Hangbin Liu <liuhangbin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@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..6a16af4 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 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] 3+ messages in thread
* Re: [PATCHv2 net] sctp: move the format error check out of __sctp_sf_do_9_1_abort
2020-02-18 4:07 [PATCHv2 net] sctp: move the format error check out of __sctp_sf_do_9_1_abort Xin Long
@ 2020-02-18 4:18 ` Marcelo Ricardo Leitner
2020-02-18 5:57 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-02-18 4:18 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, Hangbin Liu
On Tue, Feb 18, 2020 at 12:07:53PM +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.
>
> v1->v2:
> - improve the comment in the code as Marcelo's suggestion.
>
> Fixes: 96ca468b86b0 ("sctp: check invalid value of length parameter in error cause")
> Reported-by: Hangbin Liu <liuhangbin@gmail.com>
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Thanks :-)
> 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..6a16af4 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 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 [flat|nested] 3+ messages in thread
* Re: [PATCHv2 net] sctp: move the format error check out of __sctp_sf_do_9_1_abort
2020-02-18 4:07 [PATCHv2 net] sctp: move the format error check out of __sctp_sf_do_9_1_abort Xin Long
2020-02-18 4:18 ` Marcelo Ricardo Leitner
@ 2020-02-18 5:57 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2020-02-18 5:57 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, liuhangbin
From: Xin Long <lucien.xin@gmail.com>
Date: Tue, 18 Feb 2020 12:07:53 +0800
> 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.
>
> v1->v2:
> - improve the comment in the code as Marcelo's suggestion.
>
> Fixes: 96ca468b86b0 ("sctp: check invalid value of length parameter in error cause")
> Reported-by: Hangbin Liu <liuhangbin@gmail.com>
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-18 5:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 4:07 [PATCHv2 net] sctp: move the format error check out of __sctp_sf_do_9_1_abort Xin Long
2020-02-18 4:18 ` Marcelo Ricardo Leitner
2020-02-18 5:57 ` David Miller
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).