| Message ID | 20171220180855.GB22908@ziepe.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/20/2017 07:08 PM, Jason Gunthorpe wrote: > On Wed, Dec 20, 2017 at 12:35:35PM +0100, Javier Martinez Canillas wrote: >> The driver maps the I/O memory address to control the LPC bus CLKRUN_EN, >> but on the error path the memory is accessed by the .clk_enable handler >> after this was already unmapped. So only unmap the I/O memory region if >> it will not be used anymore. >> >> Also, the correct thing to do is to cleanup the resources in the inverse >> order that were acquired to prevent issues like these. >> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> >> drivers/char/tpm/tpm_tis_core.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >> index c2227983ed88..3455abbb2035 100644 >> +++ b/drivers/char/tpm/tpm_tis_core.c > > Yoiks. This patch is helping but the more I look at this the wronger > everything looks.. > > 1) tpm_chip_unregister makes chip->ops == NULL, so this sequence: > > static int tpm_tis_plat_remove(struct platform_device *pdev) > tpm_chip_unregister(chip); > tpm_tis_remove(chip); > void tpm_tis_remove(struct tpm_chip *chip) > if (chip->ops->clk_enable != NULL) > > Will oops > > 2) tpm_chip_register can also NULL ops in error cases, so this > sequence can oops: > > rc = tpm_chip_register(chip); > if (rc && is_bsw()) > iounmap(priv->ilb_base_addr); > > if (chip->ops->clk_enable != NULL) > chip->ops->clk_enable(chip, false); > > 3) iounmap should not be split between tpm_tis and tpm_tis_core > Put it at the end of tpm_tis_remove. > > 4) This sequence: > > + return tpm_chip_register(chip); > +out_err: > + tpm_tis_remove(chip); > + return rc; > > Doesn't look right. If tpm_chip_register fails then > tpm_tis_remove will never be called. This was sort of OK when > tpm_tis_remove didn't manage any resources, but now that it does > the above needs fixing too. > Right, I only noticed the issue this patch fixes and (wrongly) assumed the rest was correct. > The below draft fixes everything except #1. That needs a more thoughtful > idea.. > I'll just drop this patch from the series and you can fix all the issues in the error / driver removal paths. It's not a dependency anyways, I included it just because noticed the issue while reading the code. Best regards,
On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote: > > The below draft fixes everything except #1. That needs a more thoughtful > > idea.. > > > > I'll just drop this patch from the series and you can fix all the issues in > the error / driver removal paths. It's not a dependency anyways, I included > it just because noticed the issue while reading the code. Azhar, this means it becomes your problem :) Since the patches Jarkko has queued from you introduce oops's you need to fix them.. Jason
>-----Original Message----- >From: Jason Gunthorpe [mailto:jgg@ziepe.ca] >Sent: Wednesday, December 20, 2017 10:55 AM >To: Javier Martinez Canillas <javierm@redhat.com> >Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de >Goede <hdegoede@redhat.com>; Shaikh, Azhar <azhar.shaikh@intel.com>; >Arnd Bergmann <arnd@arndb.de>; Jarkko Sakkinen ><jarkko.sakkinen@linux.intel.com>; Peter Huewe <peterhuewe@gmx.de>; >Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux- >integrity@vger.kernel.org >Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O >memory region > >On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote: > >> > The below draft fixes everything except #1. That needs a more >> > thoughtful idea.. >> > >> >> I'll just drop this patch from the series and you can fix all the >> issues in the error / driver removal paths. It's not a dependency >> anyways, I included it just because noticed the issue while reading the code. > >Azhar, this means it becomes your problem :) > Sure, I will fix this. Javier you can drop this patch. >Since the patches Jarkko has queued from you introduce oops's you need to >fix them.. > >Jason Regards, Azhar Shaikh
Hello Azhar, On 12/20/2017 08:15 PM, Shaikh, Azhar wrote: > > >> -----Original Message----- >> From: Jason Gunthorpe [mailto:jgg@ziepe.ca] >> Sent: Wednesday, December 20, 2017 10:55 AM >> To: Javier Martinez Canillas <javierm@redhat.com> >> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de >> Goede <hdegoede@redhat.com>; Shaikh, Azhar <azhar.shaikh@intel.com>; >> Arnd Bergmann <arnd@arndb.de>; Jarkko Sakkinen >> <jarkko.sakkinen@linux.intel.com>; Peter Huewe <peterhuewe@gmx.de>; >> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux- >> integrity@vger.kernel.org >> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O >> memory region >> >> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote: >> >>>> The below draft fixes everything except #1. That needs a more >>>> thoughtful idea.. >>>> >>> >>> I'll just drop this patch from the series and you can fix all the >>> issues in the error / driver removal paths. It's not a dependency >>> anyways, I included it just because noticed the issue while reading the code. >> >> Azhar, this means it becomes your problem :) >> > > Sure, I will fix this. > Javier you can drop this patch. > On a second though, we should just merge $SUBJECT since is an obvious fix for one of the issues. Then you could fix the remaining bugs in error and driver removal paths. >> Since the patches Jarkko has queued from you introduce oops's you need to >> fix them.. >> >> Jason > > Regards, > Azhar Shaikh > Best regards,
HI Javier, >-----Original Message----- >From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- >owner@vger.kernel.org] On Behalf Of Javier Martinez Canillas >Sent: Thursday, December 21, 2017 4:49 AM >To: Shaikh, Azhar <azhar.shaikh@intel.com>; Jason Gunthorpe ><jgg@ziepe.ca> >Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de >Goede <hdegoede@redhat.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko >Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe ><peterhuewe@gmx.de>; Greg Kroah-Hartman ><gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org >Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O >memory region > >Hello Azhar, > >On 12/20/2017 08:15 PM, Shaikh, Azhar wrote: >> >> >>> -----Original Message----- >>> From: Jason Gunthorpe [mailto:jgg@ziepe.ca] >>> Sent: Wednesday, December 20, 2017 10:55 AM >>> To: Javier Martinez Canillas <javierm@redhat.com> >>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; >>> Hans de Goede <hdegoede@redhat.com>; Shaikh, Azhar >>> <azhar.shaikh@intel.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko >>> Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe >>> <peterhuewe@gmx.de>; Greg Kroah-Hartman ><gregkh@linuxfoundation.org>; >>> linux- integrity@vger.kernel.org >>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already >>> unmapped I/O memory region >>> >>> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote: >>> >>>>> The below draft fixes everything except #1. That needs a more >>>>> thoughtful idea.. >>>>> >>>> >>>> I'll just drop this patch from the series and you can fix all the >>>> issues in the error / driver removal paths. It's not a dependency >>>> anyways, I included it just because noticed the issue while reading the >code. >>> >>> Azhar, this means it becomes your problem :) >>> >> >> Sure, I will fix this. >> Javier you can drop this patch. >> > >On a second though, we should just merge $SUBJECT since is an obvious fix >for one of the issues. Then you could fix the remaining bugs in error and >driver removal paths. I think you are asking to keep this patch as a part of this series, itself and not upload separately. Is this correct? Please correct me if I am wrong. > >>> Since the patches Jarkko has queued from you introduce oops's you >>> need to fix them.. >>> >>> Jason >> >> Regards, >> Azhar Shaikh >> > >Best regards, >-- >Javier Martinez Canillas >Software Engineer - Desktop Hardware Enablement Red Hat Regards, Azhar Shaikh
On 12/21/2017 04:39 PM, Shaikh, Azhar wrote: > HI Javier, > > >> -----Original Message----- >> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- >> owner@vger.kernel.org] On Behalf Of Javier Martinez Canillas >> Sent: Thursday, December 21, 2017 4:49 AM >> To: Shaikh, Azhar <azhar.shaikh@intel.com>; Jason Gunthorpe >> <jgg@ziepe.ca> >> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de >> Goede <hdegoede@redhat.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko >> Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe >> <peterhuewe@gmx.de>; Greg Kroah-Hartman >> <gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org >> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O >> memory region >> >> Hello Azhar, >> >> On 12/20/2017 08:15 PM, Shaikh, Azhar wrote: >>> >>> >>>> -----Original Message----- >>>> From: Jason Gunthorpe [mailto:jgg@ziepe.ca] >>>> Sent: Wednesday, December 20, 2017 10:55 AM >>>> To: Javier Martinez Canillas <javierm@redhat.com> >>>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; >>>> Hans de Goede <hdegoede@redhat.com>; Shaikh, Azhar >>>> <azhar.shaikh@intel.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko >>>> Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe >>>> <peterhuewe@gmx.de>; Greg Kroah-Hartman >> <gregkh@linuxfoundation.org>; >>>> linux- integrity@vger.kernel.org >>>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already >>>> unmapped I/O memory region >>>> >>>> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote: >>>> >>>>>> The below draft fixes everything except #1. That needs a more >>>>>> thoughtful idea.. >>>>>> >>>>> >>>>> I'll just drop this patch from the series and you can fix all the >>>>> issues in the error / driver removal paths. It's not a dependency >>>>> anyways, I included it just because noticed the issue while reading the >> code. >>>> >>>> Azhar, this means it becomes your problem :) >>>> >>> >>> Sure, I will fix this. >>> Javier you can drop this patch. >>> >> >> On a second though, we should just merge $SUBJECT since is an obvious fix >> for one of the issues. Then you could fix the remaining bugs in error and >> driver removal paths. > > I think you are asking to keep this patch as a part of this series, itself and not upload separately. Is this correct? Please correct me if I am wrong. > Yes, what I meant is that we should just keep this patch in the series since it fixes one of the resource cleanup bugs. Then you can fix the other issues as a follow-up. Best regards,
>-----Original Message----- >From: Javier Martinez Canillas [mailto:javierm@redhat.com] >Sent: Thursday, December 21, 2017 7:54 AM >To: Shaikh, Azhar <azhar.shaikh@intel.com>; Jason Gunthorpe ><jgg@ziepe.ca> >Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de >Goede <hdegoede@redhat.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko >Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe ><peterhuewe@gmx.de>; Greg Kroah-Hartman ><gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org >Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O >memory region > >On 12/21/2017 04:39 PM, Shaikh, Azhar wrote: >> HI Javier, >> >> >>> -----Original Message----- >>> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- >>> owner@vger.kernel.org] On Behalf Of Javier Martinez Canillas >>> Sent: Thursday, December 21, 2017 4:49 AM >>> To: Shaikh, Azhar <azhar.shaikh@intel.com>; Jason Gunthorpe >>> <jgg@ziepe.ca> >>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; >>> Hans de Goede <hdegoede@redhat.com>; Arnd Bergmann ><arnd@arndb.de>; >>> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe >>> <peterhuewe@gmx.de>; Greg Kroah-Hartman ><gregkh@linuxfoundation.org>; >>> linux-integrity@vger.kernel.org >>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already >>> unmapped I/O memory region >>> >>> Hello Azhar, >>> >>> On 12/20/2017 08:15 PM, Shaikh, Azhar wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Jason Gunthorpe [mailto:jgg@ziepe.ca] >>>>> Sent: Wednesday, December 20, 2017 10:55 AM >>>>> To: Javier Martinez Canillas <javierm@redhat.com> >>>>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; >>>>> Hans de Goede <hdegoede@redhat.com>; Shaikh, Azhar >>>>> <azhar.shaikh@intel.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko >>>>> Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe >>>>> <peterhuewe@gmx.de>; Greg Kroah-Hartman >>> <gregkh@linuxfoundation.org>; >>>>> linux- integrity@vger.kernel.org >>>>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already >>>>> unmapped I/O memory region >>>>> >>>>> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas >wrote: >>>>> >>>>>>> The below draft fixes everything except #1. That needs a more >>>>>>> thoughtful idea.. >>>>>>> >>>>>> >>>>>> I'll just drop this patch from the series and you can fix all the >>>>>> issues in the error / driver removal paths. It's not a dependency >>>>>> anyways, I included it just because noticed the issue while >>>>>> reading the >>> code. >>>>> >>>>> Azhar, this means it becomes your problem :) >>>>> >>>> >>>> Sure, I will fix this. >>>> Javier you can drop this patch. >>>> >>> >>> On a second though, we should just merge $SUBJECT since is an obvious >>> fix for one of the issues. Then you could fix the remaining bugs in >>> error and driver removal paths. >> >> I think you are asking to keep this patch as a part of this series, itself and not >upload separately. Is this correct? Please correct me if I am wrong. >> > >Yes, what I meant is that we should just keep this patch in the series since it >fixes one of the resource cleanup bugs. Then you can fix the other issues as a >follow-up. > Ok, but even the patch which I will be submitting will be doing the cleanup. So it will just split the change. >Best regards, >-- >Javier Martinez Canillas >Software Engineer - Desktop Hardware Enablement Red Hat Regards, Azhar Shaikh
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index d29add49b03388..09f18e2e644774 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -275,9 +275,6 @@ static void tpm_tis_pnp_remove(struct pnp_dev *dev) tpm_chip_unregister(chip); tpm_tis_remove(chip); - if (is_bsw()) - iounmap(priv->ilb_base_addr); - } static struct pnp_driver tis_pnp_driver = { @@ -328,10 +325,6 @@ static int tpm_tis_plat_remove(struct platform_device *pdev) tpm_chip_unregister(chip); tpm_tis_remove(chip); - - if (is_bsw()) - iounmap(priv->ilb_base_addr); - return 0; } diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index c2227983ed88d4..ffda1694a6aba3 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -727,6 +727,9 @@ void tpm_tis_remove(struct tpm_chip *chip) if (chip->ops->clk_enable != NULL) chip->ops->clk_enable(chip, false); + + if (priv->ilb_base_addr) + iounmap(priv->ilb_base_addr); } EXPORT_SYMBOL_GPL(tpm_tis_remove); @@ -921,22 +924,15 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, } } - rc = tpm_chip_register(chip); - if (rc && is_bsw()) - iounmap(priv->ilb_base_addr); - if (chip->ops->clk_enable != NULL) chip->ops->clk_enable(chip, false); - return rc; + rc = tpm_chip_register(chip); + if (rc): + goto out_err; + return 0; out_err: tpm_tis_remove(chip); - if (is_bsw()) - iounmap(priv->ilb_base_addr); - - if (chip->ops->clk_enable != NULL) - chip->ops->clk_enable(chip, false); - return rc; } EXPORT_SYMBOL_GPL(tpm_tis_core_init);