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

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

[Duoming Zhou]

The l1oip_cleanup() traverses the l1oip_ilist and calls
release_card() to cleanup module and stack. However,
release_card() calls del_timer() to delete the timers
such as keep_tl and timeout_tl. If the timer handler is
running, the del_timer() will not stop it and result in
UAF bugs. One of the processes is shown below:

    (cleanup routine)          |        (timer handler)
release_card()                 | l1oip_timeout()
 ...                           |
 del_timer()                   | ...
 ...                           |
 kfree(hc) //FREE              |
                               | hc->timeout_on = 0 //USE

Fix by calling del_timer_sync() in release_card(), which
makes sure the timer handlers have finished before the
resources, such as l1oip and so on, have been deallocated.

Fixes: 3712b42d4b1b ("Add layer1 over IP support")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 drivers/isdn/mISDN/l1oip_core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/isdn/mISDN/l1oip_core.c b/drivers/isdn/mISDN/l1oip_core.c
index 2c40412466e..ead2fdf8ae1 100644
--- a/drivers/isdn/mISDN/l1oip_core.c
+++ b/drivers/isdn/mISDN/l1oip_core.c
@@ -1232,11 +1232,9 @@ release_card(struct l1oip *hc)
 {
 	int	ch;
 
-	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);
 
-- 
2.17.1

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

[Jakub Kicinski]

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.

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