[PATCH] mISDN: hfcpci: Fix use-after-free bug in hfcpci_softirq

[PATCH] mISDN: hfcpci: Fix use-after-free bug in hfcpci_softirq

[Duoming Zhou]

The function hfcpci_softirq() is a timer handler. If it
is running, the timer_pending() will return 0 and the
del_timer_sync() in HFC_cleanup() will not be executed.
As a result, the use-after-free bug will happen. The
process is shown below:

    (cleanup routine)          |        (timer handler)
HFC_cleanup()                  | hfcpci_softirq()
 if (timer_pending(&hfc_tl))   |
   del_timer_sync()            |
 ...                           | ...
 pci_unregister_driver(hc)     |
  driver_unregister            |  driver_for_each_device
   bus_remove_driver           |   _hfcpci_softirq
    driver_detach              |   ...
     put_device(dev) //[1]FREE |
                               |    dev_get_drvdata(dev) //[2]USE

The device is deallocated is position [1] and used in
position [2].

Fix by removing the "timer_pending" check in HFC_cleanup(),
which makes sure that the hfcpci_softirq() have finished
before the resource is deallocated.

Fixes: 009fc857c5f6 ("mISDN: fix possible use-after-free in HFC_cleanup()")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 drivers/isdn/hardware/mISDN/hfcpci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
index af17459c1a5..e964a8dd851 100644
--- a/drivers/isdn/hardware/mISDN/hfcpci.c
+++ b/drivers/isdn/hardware/mISDN/hfcpci.c
@@ -2345,8 +2345,7 @@ HFC_init(void)
 static void __exit
 HFC_cleanup(void)
 {
-	if (timer_pending(&hfc_tl))
-		del_timer_sync(&hfc_tl);
+	del_timer_sync(&hfc_tl);
 
 	pci_unregister_driver(&hfc_driver);
 }
-- 
2.17.1

Re: [PATCH] mISDN: hfcpci: Fix use-after-free bug in hfcpci_softirq

[patchwork-bot+netdevbpf]

Hello:

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

On Sun,  9 Oct 2022 14:37:31 +0800 you wrote:
> The function hfcpci_softirq() is a timer handler. If it
> is running, the timer_pending() will return 0 and the
> del_timer_sync() in HFC_cleanup() will not be executed.
> As a result, the use-after-free bug will happen. The
> process is shown below:
> 
>     (cleanup routine)          |        (timer handler)
> HFC_cleanup()                  | hfcpci_softirq()
>  if (timer_pending(&hfc_tl))   |
>    del_timer_sync()            |
>  ...                           | ...
>  pci_unregister_driver(hc)     |
>   driver_unregister            |  driver_for_each_device
>    bus_remove_driver           |   _hfcpci_softirq
>     driver_detach              |   ...
>      put_device(dev) //[1]FREE |
>                                |    dev_get_drvdata(dev) //[2]USE
> 
> [...]

Here is the summary with links:
  - mISDN: hfcpci: Fix use-after-free bug in hfcpci_softirq
    https://git.kernel.org/netdev/net/c/175302f6b79e

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

Re: [PATCH] mISDN: hfcpci: Fix use-after-free bug in hfcpci_softirq

[Greg KH]

On Sun, Oct 09, 2022 at 02:37:31PM +0800, Duoming Zhou wrote:
> The function hfcpci_softirq() is a timer handler. If it
> is running, the timer_pending() will return 0 and the
> del_timer_sync() in HFC_cleanup() will not be executed.
> As a result, the use-after-free bug will happen. The
> process is shown below:
> 
>     (cleanup routine)          |        (timer handler)
> HFC_cleanup()                  | hfcpci_softirq()
>  if (timer_pending(&hfc_tl))   |
>    del_timer_sync()            |
>  ...                           | ...
>  pci_unregister_driver(hc)     |
>   driver_unregister            |  driver_for_each_device
>    bus_remove_driver           |   _hfcpci_softirq
>     driver_detach              |   ...
>      put_device(dev) //[1]FREE |
>                                |    dev_get_drvdata(dev) //[2]USE
> 
> The device is deallocated is position [1] and used in
> position [2].
> 
> Fix by removing the "timer_pending" check in HFC_cleanup(),
> which makes sure that the hfcpci_softirq() have finished
> before the resource is deallocated.
> 
> Fixes: 009fc857c5f6 ("mISDN: fix possible use-after-free in HFC_cleanup()")
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
>  drivers/isdn/hardware/mISDN/hfcpci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
> index af17459c1a5..e964a8dd851 100644
> --- a/drivers/isdn/hardware/mISDN/hfcpci.c
> +++ b/drivers/isdn/hardware/mISDN/hfcpci.c
> @@ -2345,8 +2345,7 @@ HFC_init(void)
>  static void __exit
>  HFC_cleanup(void)
>  {
> -	if (timer_pending(&hfc_tl))
> -		del_timer_sync(&hfc_tl);
> +	del_timer_sync(&hfc_tl);

How was this tested?  Do you have this hardware?

thanks,

greg k-h