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

[duoming@zju.edu.cn]

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