Re: [PATCH] mISDN: fix use-after-free bugs in l1oip timer handlers

[duoming]

Hello,

On Thu, 22 Sep 2022 06:35:10 -0700 Jakub Kicinski wrote:

> On Tue, 20 Sep 2022 19:57:16 +0800 Duoming Zhou wrote:
> > -	if (timer_pending(&hc->keep_tl))
> > -		del_timer(&hc->keep_tl);
> > +	del_timer_sync(&hc->keep_tl);
> >  
> > -	if (timer_pending(&hc->timeout_tl))
> > -		del_timer(&hc->timeout_tl);
> > +	del_timer_sync(&hc->timeout_tl);
> >  
> >  	cancel_work_sync(&hc->workq);
> 
> There needs to be some more cleverness here.
> hc->workq and hc->socket_thread can kick those timers right back in.

In order to stop hc->keep_tl timer, I think adding del_timer_sync(&hc->keep_tl)
and cancel_work_sync(&hc->workq) again behind cancel_work_sync(&hc->workq) and
move the del_timer_sync(&hc->timeout_tl) behind l1oip_socket_close(hc) is a
better solution. The detail is shown below:

diff --git a/drivers/isdn/mISDN/l1oip_core.c b/drivers/isdn/mISDN/l1oip_core.c
index 2c40412466e..7b89d98a781 100644
--- a/drivers/isdn/mISDN/l1oip_core.c
+++ b/drivers/isdn/mISDN/l1oip_core.c
@@ -1232,17 +1232,16 @@ release_card(struct l1oip *hc)
 {
        int     ch;

-       if (timer_pending(&hc->keep_tl))
-               del_timer(&hc->keep_tl);
-
-       if (timer_pending(&hc->timeout_tl))
-               del_timer(&hc->timeout_tl);
-
+       del_timer_sync(&hc->keep_tl);
+       cancel_work_sync(&hc->workq);
+       del_timer_sync(&hc->keep_tl);
        cancel_work_sync(&hc->workq);

        if (hc->socket_thread)
                l1oip_socket_close(hc);

+       del_timer_sync(&hc->timeout_tl);
+
        if (hc->registered && hc->chan[hc->d_idx].dch)
                mISDN_unregister_device(&hc->chan[hc->d_idx].dch->dev);
        for (ch = 0; ch < 128; ch++) {

The hc->workq is scheduled in keep_tl timer:

static void
l1oip_keepalive(struct timer_list *t)
{
	struct l1oip *hc = from_timer(hc, t, keep_tl);

	schedule_work(&hc->workq);
}

So we should use del_timer_sync() to stop the timers and use
cancel_work_sync() to stop the hc->workq.

If the hc->workq has completed, the hc->keep_tl.expires is setted to
jiffies + L1OIP_KEEPALIVE * HZ and the keep_tl will restart. The detail
is shown below:

static int
l1oip_socket_send(struct l1oip *hc, u8 localcodec, u8 channel, u32 chanmask,
		  u16 timebase, u8 *buf, int len)
{
        ....

	/* restart timer */
	if (time_before(hc->keep_tl.expires, jiffies + 5 * HZ))
		mod_timer(&hc->keep_tl, jiffies + L1OIP_KEEPALIVE * HZ);
	else
		hc->keep_tl.expires = jiffies + L1OIP_KEEPALIVE * HZ;
        ...
}

So we need add del_timer_sync(&hc->keep_tl) again after cancel_work_sync(&hc->workq)
to stop hc->keep_t1 timer. The hc->workq could also be rescheduled, but the keep_tl 
timer will not be restarted again. Because the hc->keep_tl.expires equals to 
L1OIP_KEEPALIVE * HZ that is larger than jiffies + 5 * HZ. 

Finally, we use cancel_work_sync() to cancel the hc->workq. Now, Both the hc->workq 
and hc->keep_tl could be stopped.

In order to stop timeout_tl timer, we move del_timer_sync(&hc->timeout_tl) behind
l1oip_socket_close(). Because the hc->socket_thread could start the timeout_tl timer.

Best regards,
Duoming Zhou