Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Oct 2022 09:07:07 +0300
From:   Leon Romanovsky <leon@kernel.org>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Duoming Zhou <duoming@zju.edu.cn>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, isdn@linux-pingi.de, kuba@kernel.org,
        andrii@kernel.org, davem@davemloft.net, axboe@kernel.dk
Subject: Re: [PATCH] mISDN: hfcpci: Fix use-after-free bug in hfcpci_Timer
Message-ID: <Y0j8ixwitdWKuUoM@unreal>
References: <20221013125729.105652-1-duoming@zju.edu.cn>
 <Y0gQhe6EL6nDstlL@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y0gQhe6EL6nDstlL@kroah.com>
Precedence: bulk

On Thu, Oct 13, 2022 at 03:20:05PM +0200, Greg KH wrote:
> On Thu, Oct 13, 2022 at 08:57:29PM +0800, Duoming Zhou wrote:
> > If the timer handler hfcpci_Timer() is running, the
> > del_timer(&hc->hw.timer) in release_io_hfcpci() could
> > not stop it. As a result, the use-after-free bug will
> > happen. The process is shown below:
> > 
> >     (cleanup routine)          |        (timer handler)
> > release_card()                 | hfcpci_Timer()
> >   release_io_hfcpci            |
> >     del_timer(&hc->hw.timer)   |
> >   ...                          |  ...
> >   kfree(hc) //[1]FREE          |
> >                                |   hc->hw.timer.expires //[2]USE
> > 
> > The hfc_pci is deallocated in position [1] and used in
> > position [2].
> > 
> > Fix by changing del_timer() in release_io_hfcpci() to
> > del_timer_sync(), which makes sure the hfcpci_Timer()
> > have finished before the hfc_pci is deallocated.
> > 
> > Fixes: 1700fe1a10dc ("Add mISDN HFC PCI driver")
> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > ---
> >  drivers/isdn/hardware/mISDN/hfcpci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
> > index af17459c1a5..5cf37fe7de2 100644
> > --- a/drivers/isdn/hardware/mISDN/hfcpci.c
> > +++ b/drivers/isdn/hardware/mISDN/hfcpci.c
> > @@ -157,7 +157,7 @@ release_io_hfcpci(struct hfc_pci *hc)
> >  {
> >  	/* disable memory mapped ports + busmaster */
> >  	pci_write_config_word(hc->pdev, PCI_COMMAND, 0);
> > -	del_timer(&hc->hw.timer);
> > +	del_timer_sync(&hc->hw.timer);
> 
> Nice, how did you test that this will work properly?  Do you have this
> hardware for testing?  How was this issue found and verified that this
> is the correct resolution?

According to his previous response [1], the answer will be no. I'm not
super-excited that this unmaintained and old driver chosen as playground
for new tool.

[1] https://lore.kernel.org/all/17ad6913.ff8e0.1838933840d.Coremail.duoming@zju.edu.cn/#t

> 
> thanks,
> 
> greg k-h