[PATCH net v2] net: rose: fix UAF bug caused by rose_t0timer_expiry

[PATCH net v2] net: rose: fix UAF bug caused by rose_t0timer_expiry

[Duoming Zhou]

There are UAF bugs caused by rose_t0timer_expiry(). The
root cause is that del_timer() could not stop the timer
handler that is running and there is no synchronization.
One of the race conditions is shown below:

    (thread 1)             |        (thread 2)
                           | rose_device_event
                           |   rose_rt_device_down
                           |     rose_remove_neigh
rose_t0timer_expiry        |       rose_stop_t0timer(rose_neigh)
  ...                      |         del_timer(&neigh->t0timer)
                           |         kfree(rose_neigh) //[1]FREE
  neigh->dce_mode //[2]USE |

The rose_neigh is deallocated in position [1] and use in
position [2].

The crash trace triggered by POC is like below:

BUG: KASAN: use-after-free in expire_timers+0x144/0x320
Write of size 8 at addr ffff888009b19658 by task swapper/0/0
...
Call Trace:
 <IRQ>
 dump_stack_lvl+0xbf/0xee
 print_address_description+0x7b/0x440
 print_report+0x101/0x230
 ? expire_timers+0x144/0x320
 kasan_report+0xed/0x120
 ? expire_timers+0x144/0x320
 expire_timers+0x144/0x320
 __run_timers+0x3ff/0x4d0
 run_timer_softirq+0x41/0x80
 __do_softirq+0x233/0x544
 ...

This patch changes rose_stop_ftimer() and rose_stop_t0timer()
in rose_remove_neigh() to del_timer_sync() in order that the
timer handler could be finished before the resources such as
rose_neigh and so on are deallocated. As a result, the UAF
bugs could be mitigated.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in v2:
  - v2: Use del_timer_sync to stop timer in rose_remove_neigh.

 net/rose/rose_route.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
index fee6409c2bb..eb0b8197ac8 100644
--- a/net/rose/rose_route.c
+++ b/net/rose/rose_route.c
@@ -227,8 +227,8 @@ static void rose_remove_neigh(struct rose_neigh *rose_neigh)
 {
 	struct rose_neigh *s;
 
-	rose_stop_ftimer(rose_neigh);
-	rose_stop_t0timer(rose_neigh);
+	del_timer_sync(&rose_neigh->ftimer);
+	del_timer_sync(&rose_neigh->t0timer);
 
 	skb_queue_purge(&rose_neigh->queue);
 
-- 
2.17.1

Re: [PATCH net v2] net: rose: fix UAF bug caused by rose_t0timer_expiry

[Jakub Kicinski]

On Tue,  5 Jul 2022 20:56:10 +0800 Duoming Zhou wrote:
> +	del_timer_sync(&rose_neigh->t0timer);

/**
 * del_timer_sync - deactivate a timer and wait for the handler to finish.
[...]
 * Synchronization rules: Callers must prevent restarting of the timer,
 * otherwise this function is meaningless.

how is the restarting prevented? If I'm looking right 
rose_t0timer_expiry() rearms the timer.

Re: [PATCH net v2] net: rose: fix UAF bug caused by rose_t0timer_expiry

[duoming]

Hello,

On Wed, 6 Jul 2022 19:02:37 -0700 Jakub Kicinski:

> On Tue,  5 Jul 2022 20:56:10 +0800 Duoming Zhou wrote:
> > +	del_timer_sync(&rose_neigh->t0timer);
> 
> /**
>  * del_timer_sync - deactivate a timer and wait for the handler to finish.
> [...]
>  * Synchronization rules: Callers must prevent restarting of the timer,
>  * otherwise this function is meaningless.
> 
> how is the restarting prevented? If I'm looking right 
> rose_t0timer_expiry() rearms the timer.

The del_timer_sync() could stop the timer that restart itself in
its timer callback function.

The root cause is shown below which is a part of code in
del_timer_sync:

	do {
		ret = try_to_del_timer_sync(timer);

		if (unlikely(ret < 0)) {
			del_timer_wait_running(timer);
			cpu_relax();
		}
	} while (ret < 0);

https://elixir.bootlin.com/linux/latest/source/kernel/time/timer.c#L1381

If the timer callback function is running, the try_to_del_timer_sync
will return -1. Then, it will loop until the timer is not queued and
the handler is not running on any CPU.

Although the timer may restart itself in timer callback function, the
del_timer_sync could also stop it.

In order to further prove the del_timer_sync() could stop the timer that
restart itself in its timer handler, I wrote the following kernel module
whoes part of code is shown below:

=================================================================

struct timer_list my_timer;
static void my_timer_callback(struct timer_list *timer);
static void start_timer(void);

static void start_timer(void){
    del_timer(&my_timer);
    my_timer.expires = jiffies+HZ;
    my_timer.function = my_timer_callback;
    add_timer(&my_timer);
}

static void my_timer_callback(struct timer_list *timer){
    printk("In my_timer_function");
    printk("the jiffies is %ld\n",jiffies);
    start_timer();
}

static int __init del_timer_sync_init(void)
{
    int result;
    printk("my_timer will be create.\n");
    printk("the jiffies is :%ld\n", jiffies);
    timer_setup(&my_timer,my_timer_callback,0);
    result = mod_timer(&my_timer,jiffies + SIXP_TXDELAY);
    printk("the mod_timer is :%d\n\n",result);
    return 0;
}

static void __exit del_timer_sync_exit(void)
{
    int result=del_timer_sync(&my_timer);
    printk("the del_timer_sync is :%d\n\n", result);
}

=================================================================

The timer handler is running from interrupts and del_timer_sync() could stop
the timer that rewind itself in its timer handler, the result is shown below:

# insmod del_timer_sync.ko 
[  103.505857] my_timer will be create.
[  103.505922] the jiffies is :4294770832
[  103.506845] the mod_timer is :0
[  103.506845] 
# [  103.532389] In my_timer_function
[  103.532452] the jiffies is 4294770859
[  104.576768] In my_timer_function
[  104.577096] the jiffies is 4294771904
[  105.600941] In my_timer_function
[  105.601072] the jiffies is 4294772928
[  106.625397] In my_timer_function
[  106.625573] the jiffies is 4294773952
[  107.648995] In my_timer_function
[  107.649212] the jiffies is 4294774976
[  108.673037] In my_timer_function
[  108.673787] the jiffies is 4294776001
rmmod del_timer_sync.ko
[  109.649482] the del_timer_sync is :1
[  109.649482] 
# 

If we call another thread such as a work_queue or the code in other places
to restart the timer instead of in its timer handler, the del_timer_sync()
could not stop it.

Best regards,
Duoming Zhou

Re: [PATCH net v2] net: rose: fix UAF bug caused by rose_t0timer_expiry

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  5 Jul 2022 20:56:10 +0800 you wrote:
> There are UAF bugs caused by rose_t0timer_expiry(). The
> root cause is that del_timer() could not stop the timer
> handler that is running and there is no synchronization.
> One of the race conditions is shown below:
> 
>     (thread 1)             |        (thread 2)
>                            | rose_device_event
>                            |   rose_rt_device_down
>                            |     rose_remove_neigh
> rose_t0timer_expiry        |       rose_stop_t0timer(rose_neigh)
>   ...                      |         del_timer(&neigh->t0timer)
>                            |         kfree(rose_neigh) //[1]FREE
>   neigh->dce_mode //[2]USE |
> 
> [...]

Here is the summary with links:
  - [net,v2] net: rose: fix UAF bug caused by rose_t0timer_expiry
    https://git.kernel.org/netdev/net/c/148ca0451807

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html