| Message ID | 20190621153828.20699-1-albeu@free.fr |
|---|---|
| State | Accepted |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [OpenWrt-Devel] runqueue: Fix the callbacks order in runqueue_task_kill() | expand |
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
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
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
Alban <albeu@free.fr> [2019-07-01 16:23:42]: Hi, sorry for the late response, but I've just noticed, that it's related to libubox and got interested. I would use "PATCH libubox" subject next time so it's more clear at first sight. > 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. Can I kindly ask you for additional favor, adding unit test case which would expose this bug? It's going to help us in the future with possible regressions etc. It's not mandatory (yet), but really nice and really helps merging/reviewing the changes much faster (at least for me). libubox contains unit tests already, tests are run on CI[1] automatically, one of the test runs happens under Valgrind, another under various clang's sanitizers, which should hopefully catch this use-after-free bugs. You can find basic unit test for runqueue component in `tests/test-runqueue.c`, perhaps you could adjust this test directly (or write new one) in order to expose the bug. Thanks! 1. http://lists.infradead.org/pipermail/openwrt-devel/2019-December/020831.html -- ynezz
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)
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(-)