Re: use-after-free in sctp_do_sm

[Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>]

Em 11-12-2015 12:30, Dmitry Vyukov escreveu:
> On Fri, Dec 11, 2015 at 3:03 PM, Marcelo Ricardo Leitner
> <marcelo.leitner@xxxxxxxxx> wrote:
> > On Fri, Dec 11, 2015 at 11:51:21AM -0200, Marcelo Ricardo Leitner wrote:
> > > Em 11-12-2015 11:35, Dmitry Vyukov escreveu:
> > > > On Wed, Dec 9, 2015 at 5:41 PM, Marcelo Ricardo Leitner
> > > > <marcelo.leitner@xxxxxxxxx> wrote:
> > > > > On Wed, Dec 09, 2015 at 01:03:56PM -0200, Marcelo Ricardo Leitner wrote:
> > > > > > On Wed, Dec 09, 2015 at 03:41:29PM +0100, Dmitry Vyukov wrote:
> > > > > > > On Tue, Dec 8, 2015 at 8:22 PM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
> > > > > > > > On Tue, Dec 8, 2015 at 6:40 PM, Marcelo Ricardo Leitner
> > > > > > > > <marcelo.leitner@xxxxxxxxx> wrote:
> > > > > > ...
> > > > > > > > > The patches were combined already, but this last pick by Vlad is just
> > > > > > > > > not yet patched. It's not necessary for your testing and I didn't want
> > > > > > > > > to interrupt it in case you were already testing it.
> > > > > > > > >
> > > > > > > > > You can use my last patch here, from 2 emails ago, the one which
> > > > > > > > > contains this line:
> > > > > > > > > -       case SCTP_DISPOSITION_ABORT:
> > > > > > > >
> > > > > > > >
> > > > > > > > You are right. I missed that they are combined. Testing with it now.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Use-after-free still happens.
> > > > > > > I am on commit aa53685549a2cfb5f175b0c4a20bc9aa1e5a1b85 (Dec 8) plus
> > > > > > > the following sctp-related changes:
> > > > > >
> > > > > > Changes are fine.  Ugh. Ok, I'll try your new reproducer here.
> > > > >
> > > > > Heh I wasn't going to reproduce this by myself anytime soon, I think.
> > > > > It's using the same socket to connect to itself, and only happens if the
> > > > > connect() gets there before the listen() call. Figured this out because
> > > > > I could only reproduce it under strace at first.
> > > > >
> > > > > Please give this other patch a try. A state command
> > > > > (sctp_sf_cookie_wait_prm_abort) was issuing SCTP_CMD_INIT_FAILED, which
> > > > > leads to SCTP_CMD_DELETE_TCB, but returning SCTP_DISPOSITION_CONSUME,
> > > > > which fooled the patch.
> > > > >
> > > > > ---8<---
> > > > > commit 9f84d50e36cee0ce66e4ce9b3b1665e0a1dbcdd3
> > > > > Author: Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>
> > > > > Date:   Fri Dec 4 15:30:23 2015 -0200
> > > > >
> > > > >      sctp: fix use-after-free in pr_debug statement
> > > > >
> > > > >      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 was a place issuing SCTP_CMD_INIT_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@xxxxxxxxxx>
> > > > >
> > > > > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > > > > index 6098d4c42fa9..be23d5c2074f 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,
> > > > > @@ -1123,7 +1123,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();
> > > > >
> > > > > @@ -1136,7 +1136,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,
> > > > > @@ -1151,7 +1151,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;
> > > > > @@ -1174,11 +1174,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 6f46aa16cb76..d801e151498a 100644
> > > > > --- a/net/sctp/sm_statefuns.c
> > > > > +++ b/net/sctp/sm_statefuns.c
> > > > > @@ -4959,12 +4959,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;
> > > > >
> > > > >          sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> > > > >
> > > > > @@ -4983,7 +4981,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;
> > > > >   }
> > > > >
> > > > >   /*
> > > >
> > > >
> > > > Still happens...
> > > > I am on commit aa53685549a2cfb5f175b0c4a20bc9aa1e5a1b85 with your
> > > > latest patch applied.
> > > > Can you figure out what happens now from the report below? If not I
> > > > can create a repro, it's just somewhat time consuming.
> > >
> > > I can imagine. I don't know how this fuzzer works, but it would be nice if
> > > this reproducer extractor could be executed easier. So far, we have
>
> It would be very nice, but it is not always trivial.
>
> Fuzzer pretty much tried to trigger everything that is triggerable
> from user-space. Sometimes what it does can make no sense. But it is
> still super-important for contexts like Android, where programs can be
> as malicious as you can imagine and the system heavily relies on
> kernel for protection.
>
> > > identified 3 different issues already leading to this bug:
> > > - 1st, the handling on DELETE_TCB
> > > - 2nd, the handling on DISPOSITION_ABORT
> > > - 3rd, the bad combination on internal state-machine command to a return
> > > value
> > >
> > > I can and will review it again, but it's doing nasty stuff like using the
> > > same socket to connect to itself. It's hard to imagine all those
> > > combinations in mind that might lead to that use-after-free.
> > >
> > > Keep you posted.. thanks.
> >
> > Found a similar place in abort primitive handling like in this last
> > patch update, it's probably the issue you're still triggering.
> >
> > Also found another place that may lead to this use after free, in case
> > we receive a packet with a chunk that has no data.
>
> I see that sctp_cmd_interpreter does:
>
>      sctp_cmd_delete_tcb(commands, asoc);
>      asoc = NULL;
>
> Won't it be simpler to pass sctp_association ** to this function and
> let it clear it whenever it decides to free the objects, rather than
> try to duplicate its logic on higher level. Just a blind thought.

That's like a short-circuit between the two logics, it's already 
somewhat duplicated. I'm afraid that these other still returning 
DISPOSITION_CONSUME may not be aware that the assoc is going away in 
short term, maybe we have some other bug there too.

If/when we simplify sctp_side_effects() and get ride of that switch 
case, that's probably how it will work, though.

  Marcelo

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html