Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FS#3016 - libubox: read of freed memory in runqueue_task_kill() #7866

Closed
openwrt-bot opened this issue Apr 18, 2020 · 4 comments
Closed

FS#3016 - libubox: read of freed memory in runqueue_task_kill() #7866

openwrt-bot opened this issue Apr 18, 2020 · 4 comments
Labels

Comments

@openwrt-bot
Copy link

@openwrt-bot openwrt-bot commented Apr 18, 2020

ChrisNisbet01:

I believe there is a use-after-free bug in runqueue.c:runqueue_task_kill().

If a task is killed by calling runqueue_task_kill(), the 'complete' callback will be called when runqueue_task_complete() is called, which, in the case of test-runqueue.c at least, is where resources allocated for the task would normally be freed (see q_sleep_complete()).

So, after runqueue_task_kill() calls runqueue_task_complete(), the pointer 't' points to freed memory. However, the very next line in runqueue_task_kill() reads from this memory to see if there is a 'kill' callback.

I have confirmed that this occurs by modifying test-runqueue.c slightly so that a 20ms timer is started when the task is added. When the timer expires and the timer callback is called it calls runqueue_task_kill() to kill the task. When running 'make test' a failed test occurs and generates the following output...

[0/1] finish 'sleep 1' ==19171== Invalid read of size 8 ==19171== at 0x4850F47: runqueue_task_kill (runqueue.c:200) ==19171== by 0x484EDB1: uloop_process_timeouts (uloop.c:505) ==19171== by 0x484EDB1: uloop_run_timeout (uloop.c:542) ==19171== by 0x10933E: uloop_run (uloop.h:111) ==19171== by 0x10933E: main (test-runqueue.c:126) ==19171== Address 0x4a5f058 is 24 bytes inside a block of size 208 free'd ==19171== at 0x483BA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==19171== by 0x4850EAA: runqueue_task_complete (runqueue.c:234) ==19171== by 0x4850F42: runqueue_task_kill (runqueue.c:199) ==19171== by 0x484EDB1: uloop_process_timeouts (uloop.c:505) ==19171== by 0x484EDB1: uloop_run_timeout (uloop.c:542) ==19171== by 0x10933E: uloop_run (uloop.h:111) ==19171== by 0x10933E: main (test-runqueue.c:126) ==19171== Block was alloc'd at ==19171== at 0x483CD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==19171== by 0x10961D: add_sleeper.constprop.0 (test-runqueue.c:101) ==19171== by 0x10932C: main (test-runqueue.c:123) ==19171== Please find attached the modified test-runqueue.c file I used to generate the test output.
@openwrt-bot
Copy link
Author

@openwrt-bot openwrt-bot commented Apr 20, 2020

@openwrt-bot
Copy link
Author

@openwrt-bot openwrt-bot commented Apr 21, 2020

ChrisNisbet01:

That patch seems to sort things out for me. Valgrind is happy.
I've given the change a review with my inexpert eye, and it seems to be doing all the right things.
One change I'd suggest is to remove the variable 'running'.
Then

if (running && t->type->kill)

becomes

if (t->running && t->type->kill)

One other change I'd suggest is to fix up the comment above the 'kill' field of struct runqueue_task_type in runqueue.h.

/* * called to kill a task. must not make any calls to runqueue_task_complete, * it has already been removed from the list. */

With this patch applied, it has not already been removed from the list.
I'd suggest something like ...

/* * called to kill a task. must not make any calls to runqueue_task_complete, * which will be called after this returns. */

On the subject of these comments, the comment relating to the 'cancel' field seems a bit unclear. It might be better if the final sentence says

Should call runqueue_task_complete when done.

@openwrt-bot
Copy link
Author

@openwrt-bot openwrt-bot commented Apr 22, 2020

ynezz:

One change I'd suggest is to remove the variable 'running'.

Can you take over that original patch, add your proposed changes including your nice reproducer/testcase in tests/test-runqueue.c (I've just verified it, works fine here as well), add your Signed-off-by: and send it as described in https://openwrt.org/submitting-patches (adding original author to Cc: loop) ?

@openwrt-bot
Copy link
Author

@openwrt-bot openwrt-bot commented Apr 22, 2020

ynezz:

Original patch author seems to lost interest. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant