OpenWrt/LEDE Project

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bug Report
  • Category Base system
  • Assigned To
    Petr Štetiar
  • Operating System All
  • Severity Low
  • Priority Very Low
  • Reported Version Trunk
  • Due in Version Undecided
  • Due Date Undecided
  • Private
Attached to Project: OpenWrt/LEDE Project
Opened by Chris Nisbet - 18.04.2020
Last edited by Petr Štetiar - 21.05.2020

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

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.

Closed by  Petr Štetiar
21.05.2020 14:06
Reason for closing:  Fixed
Additional comments about closing:  

Fixed via https:/ /git.openwrt.org/89fb6136ad7484e4e8f9b61 8e530e098cf573665

Admin
Petr Štetiar commented on 20.04.2020 07:00
Chris Nisbet commented on 21.04.2020 02:14

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.
Admin
Petr Štetiar commented on 22.04.2020 08:12
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) ?

Admin
Petr Štetiar commented on 22.04.2020 08:13

Original patch author seems to lost interest. Thanks.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing