[OpenWrt-Devel] runqueue: Fix the callbacks order in runqueue_task_kill()
diff mbox series

Message ID 20190621153828.20699-1-albeu@free.fr
State Under Review
Delegated to: John Crispin
Headers show
Series
  • [OpenWrt-Devel] runqueue: Fix the callbacks order in runqueue_task_kill()
Related show

Commit Message

Alban June 21, 2019, 3:38 p.m. UTC
Since commit 11e8afea (runqueue should cal the complete handler from
more places) the call to the complete() callback has been moved to
runqueue_task_complete().  However in runqueue_task_kill()
runqueue_task_complete() is called before the kill() callback.
This will result in a use after free if the complete() callback free
the task struct.

Furthermore runqueue_start_next() is already called at the end of
runqueue_task_complete(), so there is no need to call it again in
runqueue_task_kill().

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 runqueue.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

John Crispin July 1, 2019, 1:48 p.m. UTC | #1
On 21/06/2019 17:38, Alban Bedel wrote:
> Since commit 11e8afea (runqueue should cal the complete handler from
> more places) the call to the complete() callback has been moved to
> runqueue_task_complete().  However in runqueue_task_kill()
> runqueue_task_complete() is called before the kill() callback.
> This will result in a use after free if the complete() callback free
> the task struct.
>
> Furthermore runqueue_start_next() is already called at the end of
> runqueue_task_complete(), so there is no need to call it again in
> runqueue_task_kill().
>
> Signed-off-by: Alban Bedel <albeu@free.fr>
> ---
>   runqueue.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/runqueue.c b/runqueue.c
> index a1d0133..4c621aa 100644
> --- a/runqueue.c
> +++ b/runqueue.c
> @@ -196,11 +196,9 @@ void runqueue_task_kill(struct runqueue_task *t)
>   	if (!t->queued)
>   		return;
>   
> -	runqueue_task_complete(t);
>   	if (running && t->type->kill)
>   		t->type->kill(q, t);
> -
> -	runqueue_start_next(q);
> +	runqueue_task_complete(t);
>   }
>   
>   void runqueue_stop(struct runqueue *q)

Hi,

runqueue_task_complete() will decrement running which, if called after the kill clause might not even trigger the kill() call. I am assuming you are running a custom runqueue_task_type ?

  John
Alban July 1, 2019, 2:23 p.m. UTC | #2
On Mon, 1 Jul 2019 15:48:47 +0200
John Crispin <john@phrozen.org> wrote:

> On 21/06/2019 17:38, Alban Bedel wrote:
> > Since commit 11e8afea (runqueue should cal the complete handler from
> > more places) the call to the complete() callback has been moved to
> > runqueue_task_complete().  However in runqueue_task_kill()
> > runqueue_task_complete() is called before the kill() callback.
> > This will result in a use after free if the complete() callback free
> > the task struct.
> >
> > Furthermore runqueue_start_next() is already called at the end of
> > runqueue_task_complete(), so there is no need to call it again in
> > runqueue_task_kill().
> >
> > Signed-off-by: Alban Bedel <albeu@free.fr>
> > ---
> >   runqueue.c | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/runqueue.c b/runqueue.c
> > index a1d0133..4c621aa 100644
> > --- a/runqueue.c
> > +++ b/runqueue.c
> > @@ -196,11 +196,9 @@ void runqueue_task_kill(struct runqueue_task
> > *t) if (!t->queued)
> >   		return;
> >   
> > -	runqueue_task_complete(t);
> >   	if (running && t->type->kill)
> >   		t->type->kill(q, t);
> > -
> > -	runqueue_start_next(q);
> > +	runqueue_task_complete(t);
> >   }
> >   
> >   void runqueue_stop(struct runqueue *q)  
> 
> Hi,
> 
> runqueue_task_complete() will decrement running which, if called
> after the kill clause might not even trigger the kill() call. I am
> assuming you are running a custom runqueue_task_type ?

No, TBH I haven't tested this, but as there is no documentation I had
to go through the code and noticed this suspicious construct. I then
saw commit 6a7fb7d8d (runqueue: fix use-after-free bug) which confirmed
that complete() is expected to free the task struct, which with the
above code will clearly lead to an access after free.

But I don't really see what you mean, 'running' is a boolean so it
can't be decremented. Did you mean 'running_tasks'?

Alban
John Crispin July 1, 2019, 2:54 p.m. UTC | #3
On 01/07/2019 16:23, Alban wrote:
> On Mon, 1 Jul 2019 15:48:47 +0200
> John Crispin <john@phrozen.org> wrote:
>
>> On 21/06/2019 17:38, Alban Bedel wrote:
>>> Since commit 11e8afea (runqueue should cal the complete handler from
>>> more places) the call to the complete() callback has been moved to
>>> runqueue_task_complete().  However in runqueue_task_kill()
>>> runqueue_task_complete() is called before the kill() callback.
>>> This will result in a use after free if the complete() callback free
>>> the task struct.
>>>
>>> Furthermore runqueue_start_next() is already called at the end of
>>> runqueue_task_complete(), so there is no need to call it again in
>>> runqueue_task_kill().
>>>
>>> Signed-off-by: Alban Bedel <albeu@free.fr>
>>> ---
>>>    runqueue.c | 4 +---
>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/runqueue.c b/runqueue.c
>>> index a1d0133..4c621aa 100644
>>> --- a/runqueue.c
>>> +++ b/runqueue.c
>>> @@ -196,11 +196,9 @@ void runqueue_task_kill(struct runqueue_task
>>> *t) if (!t->queued)
>>>    		return;
>>>    
>>> -	runqueue_task_complete(t);
>>>    	if (running && t->type->kill)
>>>    		t->type->kill(q, t);
>>> -
>>> -	runqueue_start_next(q);
>>> +	runqueue_task_complete(t);
>>>    }
>>>    
>>>    void runqueue_stop(struct runqueue *q)
>> Hi,
>>
>> runqueue_task_complete() will decrement running which, if called
>> after the kill clause might not even trigger the kill() call. I am
>> assuming you are running a custom runqueue_task_type ?
> No, TBH I haven't tested this, but as there is no documentation I had
> to go through the code and noticed this suspicious construct. I then
> saw commit 6a7fb7d8d (runqueue: fix use-after-free bug) which confirmed
> that complete() is expected to free the task struct, which with the
> above code will clearly lead to an access after free.
>
> But I don't really see what you mean, 'running' is a boolean so it
> can't be decremented. Did you mean 'running_tasks'?
>
> Alban


i did actually mean running_task, I'll have another look later on today

     John

Patch
diff mbox series

diff --git a/runqueue.c b/runqueue.c
index a1d0133..4c621aa 100644
--- a/runqueue.c
+++ b/runqueue.c
@@ -196,11 +196,9 @@  void runqueue_task_kill(struct runqueue_task *t)
 	if (!t->queued)
 		return;
 
-	runqueue_task_complete(t);
 	if (running && t->type->kill)
 		t->type->kill(q, t);
-
-	runqueue_start_next(q);
+	runqueue_task_complete(t);
 }
 
 void runqueue_stop(struct runqueue *q)