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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <748522b6.ec147.183699a6dfb.Coremail.duoming@zju.edu.cn>
Date:   Fri, 23 Sep 2022 17:07:51 +0800 (GMT+08:00)
From:   duoming@....edu.cn
To:     "Jakub Kicinski" <kuba@...nel.org>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        isdn@...ux-pingi.de
Subject: Re: [PATCH] mISDN: fix use-after-free bugs in l1oip timer handlers

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