From: Vlad Yasevich <vyasevich@gmail•com>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail•com>,
Dmitry Vyukov <dvyukov@google•com>
Cc: netdev <netdev@vger•kernel.org>,
Eric Dumazet <eric.dumazet@gmail•com>,
syzkaller <syzkaller@googlegroups•com>,
linux-sctp@vger•kernel.org, Kostya Serebryany <kcc@google•com>,
Alexander Potapenko <glider@google•com>,
Sasha Levin <sasha.levin@oracle•com>
Subject: Re: use-after-free in sctp_do_sm
Date: Fri, 11 Dec 2015 13:37:52 -0500 [thread overview]
Message-ID: <566B1800.60100@gmail.com> (raw)
In-Reply-To: <20151211140333.GE3886@mrl.redhat.com>
On 12/11/2015 09:03 AM, Marcelo Ricardo Leitner 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@gmail•com> 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@google•com> wrote:
>>>>>>> On Tue, Dec 8, 2015 at 6:40 PM, Marcelo Ricardo Leitner
>>>>>>> <marcelo.leitner@gmail•com> 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@gmail•com>
>>>> 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@google•com>
>>>>
>>>> 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
>> 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.
>
> Oh my.. :)
Yes. This is what I was worried about... Anything that triggers
a DELTE_TCB command has to return a code that we can trap.
The other way is to do what Dmitri suggested, but even there, we
need to be very careful.
-vlad
>
> Marcelo
>
next prev parent reply other threads:[~2015-12-11 18:37 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
2016-01-11 22:13 ` David Miller
2016-01-12 8:41 ` Dmitry Vyukov
2015-12-11 18:37 ` Vlad Yasevich [this message]
2015-12-14 9:50 ` use-after-free in sctp_do_sm 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=566B1800.60100@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