From: Vlad Yasevich <vyasevich@gmail•com>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail•com>,
netdev@vger•kernel.org
Cc: linux-sctp@vger•kernel.org, dvyukov@google•com,
eric.dumazet@gmail•com, syzkaller@googlegroups•com,
kcc@google•com, glider@google•com, sasha.levin@oracle•com
Subject: Re: [PATCH] sctp: fix use-after-free in pr_debug statement
Date: Mon, 11 Jan 2016 12:00:03 -0500 [thread overview]
Message-ID: <5693DF93.1070808@gmail.com> (raw)
In-Reply-To: <51dffdfdb37c240ff7e9b0b2a93433f217fa4d2c.1452257700.git.marcelo.leitner@gmail.com>
On 01/08/2016 08:00 AM, Marcelo Ricardo Leitner wrote:
> Couldn't get syzkaller working over here, so I still need your help on
> testing this. I expect this will be the last cycle, though.
>
> If it does generate another trace, I'll need the reproducer too because
> I can't find anything else just with code review.
>
> Thanks
Looks to me like you got all of them.
>
> --8<--
>
> Dmitry Vyukov reported a use-after-free in the code expanded by the
> macro debug_post_sfx, which is caused by the use of the asoc pointer
> after it was freed within sctp_side_effect() scope.
>
> This patch fixes it by allowing sctp_side_effect to clear that asoc
> pointer when the TCB is freed.
>
> As Vlad explained, we also have to cover the SCTP_DISPOSITION_ABORT case
> because it will trigger DELETE_TCB too on that same loop.
>
> Also, there were places issuing SCTP_CMD_INIT_FAILED and ASSOC_FAILED
> but returning SCTP_DISPOSITION_CONSUME, which would fool the scheme
> above. Fix it by returning SCTP_DISPOSITION_ABORT instead.
>
> The macro is already prepared to handle such NULL pointer.
>
> Reported-by: Dmitry Vyukov <dvyukov@google•com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail•com>
Acked-by: Vlad Yasevich <vyasevich@gmail•com>
Thanks
-vlad
> ---
> net/sctp/sm_sideeffect.c | 11 ++++++-----
> net/sctp/sm_statefuns.c | 17 ++++-------------
> 2 files changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 4f170ad38ff4f7d345d8e3a3fee7d691df64d9cb..2e21384697c2a6a5fd045142bcd9c39992d3867f 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -63,7 +63,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
> static int sctp_side_effects(sctp_event_t event_type, sctp_subtype_t subtype,
> sctp_state_t state,
> struct sctp_endpoint *ep,
> - struct sctp_association *asoc,
> + struct sctp_association **asoc,
> void *event_arg,
> sctp_disposition_t status,
> sctp_cmd_seq_t *commands,
> @@ -1125,7 +1125,7 @@ int sctp_do_sm(struct net *net, sctp_event_t event_type, sctp_subtype_t subtype,
> debug_post_sfn();
>
> error = sctp_side_effects(event_type, subtype, state,
> - ep, asoc, event_arg, status,
> + ep, &asoc, event_arg, status,
> &commands, gfp);
> debug_post_sfx();
>
> @@ -1138,7 +1138,7 @@ int sctp_do_sm(struct net *net, sctp_event_t event_type, sctp_subtype_t subtype,
> static int sctp_side_effects(sctp_event_t event_type, sctp_subtype_t subtype,
> sctp_state_t state,
> struct sctp_endpoint *ep,
> - struct sctp_association *asoc,
> + struct sctp_association **asoc,
> void *event_arg,
> sctp_disposition_t status,
> sctp_cmd_seq_t *commands,
> @@ -1153,7 +1153,7 @@ static int sctp_side_effects(sctp_event_t event_type, sctp_subtype_t subtype,
> * disposition SCTP_DISPOSITION_CONSUME.
> */
> if (0 != (error = sctp_cmd_interpreter(event_type, subtype, state,
> - ep, asoc,
> + ep, *asoc,
> event_arg, status,
> commands, gfp)))
> goto bail;
> @@ -1176,11 +1176,12 @@ static int sctp_side_effects(sctp_event_t event_type, sctp_subtype_t subtype,
> break;
>
> case SCTP_DISPOSITION_DELETE_TCB:
> + case SCTP_DISPOSITION_ABORT:
> /* This should now be a command. */
> + *asoc = NULL;
> break;
>
> case SCTP_DISPOSITION_CONSUME:
> - case SCTP_DISPOSITION_ABORT:
> /*
> * We should no longer have much work to do here as the
> * real work has been done as explicit commands above.
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 22c2bf367d7e8c7025065f33eabfd7e93a7f4021..f1f08c8f277bd8719299d1ed21eb23e36d55f7e2 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -2976,7 +2976,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
> SCTP_INC_STATS(net, SCTP_MIB_IN_DATA_CHUNK_DISCARDS);
> goto discard_force;
> case SCTP_IERROR_NO_DATA:
> - goto consume;
> + return SCTP_DISPOSITION_ABORT;
> case SCTP_IERROR_PROTO_VIOLATION:
> return sctp_sf_abort_violation(net, ep, asoc, chunk, commands,
> (u8 *)chunk->subh.data_hdr, sizeof(sctp_datahdr_t));
> @@ -3043,9 +3043,6 @@ discard_noforce:
> sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force);
>
> return SCTP_DISPOSITION_DISCARD;
> -consume:
> - return SCTP_DISPOSITION_CONSUME;
> -
> }
>
> /*
> @@ -3093,7 +3090,7 @@ sctp_disposition_t sctp_sf_eat_data_fast_4_4(struct net *net,
> case SCTP_IERROR_BAD_STREAM:
> break;
> case SCTP_IERROR_NO_DATA:
> - goto consume;
> + return SCTP_DISPOSITION_ABORT;
> case SCTP_IERROR_PROTO_VIOLATION:
> return sctp_sf_abort_violation(net, ep, asoc, chunk, commands,
> (u8 *)chunk->subh.data_hdr, sizeof(sctp_datahdr_t));
> @@ -3119,7 +3116,6 @@ sctp_disposition_t sctp_sf_eat_data_fast_4_4(struct net *net,
> SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
> }
>
> -consume:
> return SCTP_DISPOSITION_CONSUME;
> }
>
> @@ -4825,9 +4821,6 @@ sctp_disposition_t sctp_sf_do_9_1_prm_abort(
> * if necessary to fill gaps.
> */
> struct sctp_chunk *abort = arg;
> - sctp_disposition_t retval;
> -
> - retval = SCTP_DISPOSITION_CONSUME;
>
> if (abort)
> sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> @@ -4845,7 +4838,7 @@ sctp_disposition_t sctp_sf_do_9_1_prm_abort(
> SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
>
> - return retval;
> + return SCTP_DISPOSITION_ABORT;
> }
>
> /* We tried an illegal operation on an association which is closed. */
> @@ -4960,12 +4953,10 @@ sctp_disposition_t sctp_sf_cookie_wait_prm_abort(
> sctp_cmd_seq_t *commands)
> {
> struct sctp_chunk *abort = arg;
> - sctp_disposition_t retval;
>
> /* Stop T1-init timer */
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
> SCTP_TO(SCTP_EVENT_TIMEOUT_T1_INIT));
> - retval = SCTP_DISPOSITION_CONSUME;
>
> if (abort)
> sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> @@ -4985,7 +4976,7 @@ sctp_disposition_t sctp_sf_cookie_wait_prm_abort(
> sctp_add_cmd_sf(commands, SCTP_CMD_INIT_FAILED,
> SCTP_PERR(SCTP_ERROR_USER_ABORT));
>
> - return retval;
> + return SCTP_DISPOSITION_ABORT;
> }
>
> /*
>
next prev parent reply other threads:[~2016-01-11 17:00 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-24 9:15 use-after-free in sctp_do_sm Dmitry Vyukov
2015-11-24 9:31 ` Dmitry Vyukov
2015-11-24 10:10 ` Dmitry Vyukov
2015-11-24 20:45 ` Neil Horman
2015-11-24 21:08 ` Eric Dumazet
2015-11-24 21:12 ` David Miller
2015-11-25 15:12 ` Vlad Yasevich
2015-11-28 15:50 ` Dmitry Vyukov
2015-12-03 16:51 ` Marcelo Ricardo Leitner
2015-12-03 17:43 ` Marcelo Ricardo Leitner
2015-12-03 17:59 ` Eric Dumazet
2015-12-03 18:06 ` Marcelo
2015-12-03 18:35 ` Vlad Yasevich
2015-12-03 18:43 ` Marcelo
2015-12-04 17:14 ` [PATCH net 0/3] sctp: packet timestamp fixes Marcelo Ricardo Leitner
2015-12-04 17:14 ` [PATCH net 1/3] sctp: use the same clock as if sock source timestamps were on Marcelo Ricardo Leitner
2015-12-04 20:31 ` Vlad Yasevich
2015-12-04 17:14 ` [PATCH net 2/3] sctp: update the netstamp_needed counter when copying sockets Marcelo Ricardo Leitner
2015-12-04 20:33 ` Vlad Yasevich
2015-12-04 17:14 ` [PATCH net 3/3] sctp: also copy sk_tsflags when copying the socket Marcelo Ricardo Leitner
2015-12-04 20:33 ` Vlad Yasevich
2015-12-06 3:24 ` [PATCH net 0/3] sctp: packet timestamp fixes David Miller
2015-12-03 13:05 ` use-after-free in sctp_do_sm Marcelo Ricardo Leitner
2015-12-03 13:45 ` Dmitry Vyukov
2015-12-03 14:48 ` Eric Dumazet
2015-12-03 15:55 ` Dmitry Vyukov
2015-12-03 16:15 ` Marcelo Ricardo Leitner
2015-12-03 17:02 ` Eric Dumazet
2015-12-03 17:12 ` Dmitry Vyukov
2015-12-03 18:52 ` Aaron Conole
2015-12-03 19:06 ` Joe Perches
2015-12-03 19:32 ` Jason Baron
2015-12-03 20:03 ` Joe Perches
2015-12-03 20:10 ` Jason Baron
2015-12-03 20:24 ` Joe Perches
2015-12-03 20:42 ` Jason Baron
2015-12-03 20:51 ` Joe Perches
2015-12-04 10:40 ` Dmitry Vyukov
2015-12-04 12:55 ` Marcelo Ricardo Leitner
2015-12-04 15:37 ` Vlad Yasevich
2015-12-04 15:51 ` Aaron Conole
2015-12-04 16:12 ` Dmitry Vyukov
2015-12-04 16:47 ` Jason Baron
2015-12-04 17:03 ` Joe Perches
2015-12-04 17:11 ` Jason Baron
2015-12-04 10:41 ` Dmitry Vyukov
2015-12-04 17:48 ` Marcelo Ricardo Leitner
2015-12-04 20:25 ` Dmitry Vyukov
2015-12-04 21:34 ` Marcelo Ricardo Leitner
2015-12-04 21:38 ` Dmitry Vyukov
2015-12-05 16:39 ` Vlad Yasevich
2015-12-07 11:26 ` Dmitry Vyukov
2015-12-07 13:15 ` Marcelo Ricardo Leitner
2015-12-07 13:20 ` Dmitry Vyukov
2015-12-07 18:52 ` Marcelo Ricardo Leitner
2015-12-07 19:33 ` Vlad Yasevich
2015-12-07 19:50 ` Marcelo Ricardo Leitner
2015-12-07 20:37 ` Vlad Yasevich
2015-12-07 20:52 ` Marcelo Ricardo Leitner
2015-12-08 17:30 ` Dmitry Vyukov
2015-12-08 17:40 ` Marcelo Ricardo Leitner
2015-12-08 19:22 ` Dmitry Vyukov
2015-12-09 14:41 ` Dmitry Vyukov
2015-12-09 15:03 ` Marcelo Ricardo Leitner
2015-12-09 16:41 ` Marcelo Ricardo Leitner
2015-12-11 13:35 ` Dmitry Vyukov
2015-12-11 13:51 ` Marcelo Ricardo Leitner
2015-12-11 14:03 ` Marcelo Ricardo Leitner
2015-12-11 14:30 ` Dmitry Vyukov
2015-12-11 15:55 ` Marcelo Ricardo Leitner
2016-01-08 13:00 ` [PATCH] sctp: fix use-after-free in pr_debug statement Marcelo Ricardo Leitner
2016-01-11 17:00 ` Vlad Yasevich [this message]
2016-01-11 22:13 ` David Miller
2016-01-12 8:41 ` Dmitry Vyukov
2015-12-11 18:37 ` use-after-free in sctp_do_sm Vlad Yasevich
2015-12-14 9:50 ` David Laight
2015-12-14 14:25 ` Vlad Yasevich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5693DF93.1070808@gmail.com \
--to=vyasevich@gmail$(echo .)com \
--cc=dvyukov@google$(echo .)com \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=glider@google$(echo .)com \
--cc=kcc@google$(echo .)com \
--cc=linux-sctp@vger$(echo .)kernel.org \
--cc=marcelo.leitner@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=sasha.levin@oracle$(echo .)com \
--cc=syzkaller@googlegroups$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox