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 <[email protected]>
---
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
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.
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