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

[PATCH V4] 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.

What's more, the hc->workq and hc->socket_thread can kick
those timers right back in. We add a bool flag to show
if card is released. Then, check this flag in hc->workq
and hc->socket_thread.

Fixes: 3712b42d4b1b ("Add layer1 over IP support")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in v4:
  - Use bool flag to judge whether card is released.

 drivers/isdn/mISDN/l1oip.h      |  1 +
 drivers/isdn/mISDN/l1oip_core.c | 13 +++++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/isdn/mISDN/l1oip.h b/drivers/isdn/mISDN/l1oip.h
index 7ea10db20e3..48133d02281 100644
--- a/drivers/isdn/mISDN/l1oip.h
+++ b/drivers/isdn/mISDN/l1oip.h
@@ -59,6 +59,7 @@ struct l1oip {
 	int			bundle;		/* bundle channels in one frm */
 	int			codec;		/* codec to use for transmis. */
 	int			limit;		/* limit number of bchannels */
+	bool			shutdown;	/* if card is released */
 
 	/* timer */
 	struct timer_list	keep_tl;
diff --git a/drivers/isdn/mISDN/l1oip_core.c b/drivers/isdn/mISDN/l1oip_core.c
index 2c40412466e..a77195e378b 100644
--- a/drivers/isdn/mISDN/l1oip_core.c
+++ b/drivers/isdn/mISDN/l1oip_core.c
@@ -275,7 +275,7 @@ l1oip_socket_send(struct l1oip *hc, u8 localcodec, u8 channel, u32 chanmask,
 	p = frame;
 
 	/* restart timer */
-	if (time_before(hc->keep_tl.expires, jiffies + 5 * HZ))
+	if (time_before(hc->keep_tl.expires, jiffies + 5 * HZ) && !hc->shutdown)
 		mod_timer(&hc->keep_tl, jiffies + L1OIP_KEEPALIVE * HZ);
 	else
 		hc->keep_tl.expires = jiffies + L1OIP_KEEPALIVE * HZ;
@@ -601,7 +601,9 @@ l1oip_socket_parse(struct l1oip *hc, struct sockaddr_in *sin, u8 *buf, int len)
 		goto multiframe;
 
 	/* restart timer */
-	if (time_before(hc->timeout_tl.expires, jiffies + 5 * HZ) || !hc->timeout_on) {
+	if ((time_before(hc->timeout_tl.expires, jiffies + 5 * HZ) ||
+	     !hc->timeout_on) &&
+	    !hc->shutdown) {
 		hc->timeout_on = 1;
 		mod_timer(&hc->timeout_tl, jiffies + L1OIP_TIMEOUT * HZ);
 	} else /* only adjust timer */
@@ -1232,11 +1234,10 @@ release_card(struct l1oip *hc)
 {
 	int	ch;
 
-	if (timer_pending(&hc->keep_tl))
-		del_timer(&hc->keep_tl);
+	hc->shutdown = true;
 
-	if (timer_pending(&hc->timeout_tl))
-		del_timer(&hc->timeout_tl);
+	del_timer_sync(&hc->keep_tl);
+	del_timer_sync(&hc->timeout_tl);
 
 	cancel_work_sync(&hc->workq);
 
-- 
2.17.1

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

[Leon Romanovsky]

On Wed, Sep 28, 2022 at 09:39:38PM +0800, Duoming Zhou wrote:
> 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.
> 
> What's more, the hc->workq and hc->socket_thread can kick
> those timers right back in. We add a bool flag to show
> if card is released. Then, check this flag in hc->workq
> and hc->socket_thread.
> 
> Fixes: 3712b42d4b1b ("Add layer1 over IP support")
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
> Changes in v4:
>   - Use bool flag to judge whether card is released.
> 
>  drivers/isdn/mISDN/l1oip.h      |  1 +
>  drivers/isdn/mISDN/l1oip_core.c | 13 +++++++------
>  2 files changed, 8 insertions(+), 6 deletions(-)

It looks like it is ok now, but whole mISDN code doesn't look healthy,
so it is hard to say for sure.

Are you fixing real issue that you saw in field?

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

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

[duoming]

Hello,

On Thu, 29 Sep 2022 10:22:07 +0300 Leon Romanovsky wrote:

> On Wed, Sep 28, 2022 at 09:39:38PM +0800, Duoming Zhou wrote:
> > 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.
> > 
> > What's more, the hc->workq and hc->socket_thread can kick
> > those timers right back in. We add a bool flag to show
> > if card is released. Then, check this flag in hc->workq
> > and hc->socket_thread.
> > 
> > Fixes: 3712b42d4b1b ("Add layer1 over IP support")
> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > ---
> > Changes in v4:
> >   - Use bool flag to judge whether card is released.
> > 
> >  drivers/isdn/mISDN/l1oip.h      |  1 +
> >  drivers/isdn/mISDN/l1oip_core.c | 13 +++++++------
> >  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> It looks like it is ok now, but whole mISDN code doesn't look healthy,
> so it is hard to say for sure.
> 
> Are you fixing real issue that you saw in field?

Thank you for your time and reply! I found this issue through a 
static analysis tool wroten by myself.

> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Thank you!

Best regards,
Duoming Zhou

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 28 Sep 2022 21:39:38 +0800 you wrote:
> 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:
> 
> [...]

Here is the summary with links:
  - [V4] mISDN: fix use-after-free bugs in l1oip timer handlers
    https://git.kernel.org/netdev/net/c/2568a7e0832e

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