| Message ID | 1513898019-164487-1-git-send-email-azhar.shaikh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 21, 2017 at 03:13:39PM -0800, Azhar Shaikh wrote: > Commit 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout > the duration of transmit_cmd()") added code which accessed > chip->ops, even after it was set to NULL in tpm_del_char_device(), > called from tpm_chip_unregister() in error / driver exit paths. > So fix this code. > > Fixes: 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout > the duration of transmit_cmd()") > > Suggested-by: Javier Martinez Canillas <javierm@redhat.com> > Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> > Changes in v2: > - Remove TPM_CHIP_FLAG_DO_NOT_CLEAR_OPS flag, instead call > tpm_tis_clkrun_enable() directly in tpm_tis_remove() > - Add comment to tpm_tis_clkrun_enable() Looks Ok to me Reviewed-by: Jason Gunthorpe <jgg@ziepe.ca> Jason
On Thu, Dec 21, 2017 at 03:13:39PM -0800, Azhar Shaikh wrote: > Commit 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout > the duration of transmit_cmd()") added code which accessed > chip->ops, even after it was set to NULL in tpm_del_char_device(), > called from tpm_chip_unregister() in error / driver exit paths. > So fix this code. > > Fixes: 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout > the duration of transmit_cmd()") > > Suggested-by: Javier Martinez Canillas <javierm@redhat.com> > Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> > Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> > --- > Changes in v2: > - Remove TPM_CHIP_FLAG_DO_NOT_CLEAR_OPS flag, instead call > tpm_tis_clkrun_enable() directly in tpm_tis_remove() > - Add comment to tpm_tis_clkrun_enable() > > drivers/char/tpm/tpm_tis.c | 6 ------ > drivers/char/tpm/tpm_tis_core.c | 29 +++++++++++++++++------------ > 2 files changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index d29add49b033..c847fc69a2fc 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 = { > @@ -329,9 +326,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 c2227983ed88..519e4a78c8f8 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -37,6 +37,8 @@ > */ > #define TPM_POLL_SLEEP 1 /* msec */ > > +static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); > + > static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, > bool check_cancel, bool *canceled) > { > @@ -716,8 +718,7 @@ void tpm_tis_remove(struct tpm_chip *chip) > u32 interrupt; > int rc; > > - if (chip->ops->clk_enable != NULL) > - chip->ops->clk_enable(chip, true); > + tpm_tis_clkrun_enable(chip, true); > > rc = tpm_tis_read32(priv, reg, &interrupt); > if (rc < 0) > @@ -725,8 +726,10 @@ void tpm_tis_remove(struct tpm_chip *chip) > > tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt); > > - if (chip->ops->clk_enable != NULL) > - chip->ops->clk_enable(chip, false); > + tpm_tis_clkrun_enable(chip, false); > + > + if (priv->ilb_base_addr) > + iounmap(priv->ilb_base_addr); > } > EXPORT_SYMBOL_GPL(tpm_tis_remove); > > @@ -736,6 +739,9 @@ void tpm_tis_remove(struct tpm_chip *chip) > * @chip: TPM chip to use > * @value: 1 - Disable CLKRUN protocol, so that clocks are free running > * 0 - Enable CLKRUN protocol > + * > + * Call this function directly in tpm_tis_remove() in error or driver removal > + * path, since the chip->ops is set to NULL in tpm_chip_unregister(). > */ > static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) > { > @@ -922,21 +928,20 @@ 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 (rc) > + goto out_err; > > if (chip->ops->clk_enable != NULL) > chip->ops->clk_enable(chip, false); > > - return rc; > -out_err: > - tpm_tis_remove(chip); > - if (is_bsw()) > - iounmap(priv->ilb_base_addr); > + return 0; > > - if (chip->ops->clk_enable != NULL) > +out_err: > + if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL)) > chip->ops->clk_enable(chip, false); > > + tpm_tis_remove(chip); > + > return rc; > } > EXPORT_SYMBOL_GPL(tpm_tis_core_init); > -- > 1.9.1 > Since the original commit is not in the mainline yet, you should rather send an updated version of that commit. /Jarkko
>-----Original Message----- >From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] >Sent: Friday, December 22, 2017 10:42 AM >To: Shaikh, Azhar <azhar.shaikh@intel.com> >Cc: jgg@ziepe.ca; javierm@redhat.com; peterhuewe@gmx.de; linux-security- >module@vger.kernel.org; linux-integrity@vger.kernel.org; linux- >kernel@vger.kernel.org >Subject: Re: [PATCH v2] tpm: Fix the driver cleanup code > >On Thu, Dec 21, 2017 at 03:13:39PM -0800, Azhar Shaikh wrote: >> Commit 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout the >> duration of transmit_cmd()") added code which accessed >> chip->ops, even after it was set to NULL in tpm_del_char_device(), >> called from tpm_chip_unregister() in error / driver exit paths. >> So fix this code. >> >> Fixes: 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout the >> duration of transmit_cmd()") >> >> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> >> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> >> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> >> --- >> Changes in v2: >> - Remove TPM_CHIP_FLAG_DO_NOT_CLEAR_OPS flag, instead call >> tpm_tis_clkrun_enable() directly in tpm_tis_remove() >> - Add comment to tpm_tis_clkrun_enable() >> >> drivers/char/tpm/tpm_tis.c | 6 ------ >> drivers/char/tpm/tpm_tis_core.c | 29 +++++++++++++++++------------ >> 2 files changed, 17 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c >> index d29add49b033..c847fc69a2fc 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 = { @@ -329,9 +326,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 c2227983ed88..519e4a78c8f8 >> 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -37,6 +37,8 @@ >> */ >> #define TPM_POLL_SLEEP 1 /* msec */ >> >> +static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); >> + >> static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, >> bool check_cancel, bool *canceled) { >@@ -716,8 +718,7 @@ void >> tpm_tis_remove(struct tpm_chip *chip) >> u32 interrupt; >> int rc; >> >> - if (chip->ops->clk_enable != NULL) >> - chip->ops->clk_enable(chip, true); >> + tpm_tis_clkrun_enable(chip, true); >> >> rc = tpm_tis_read32(priv, reg, &interrupt); >> if (rc < 0) >> @@ -725,8 +726,10 @@ void tpm_tis_remove(struct tpm_chip *chip) >> >> tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt); >> >> - if (chip->ops->clk_enable != NULL) >> - chip->ops->clk_enable(chip, false); >> + tpm_tis_clkrun_enable(chip, false); >> + >> + if (priv->ilb_base_addr) >> + iounmap(priv->ilb_base_addr); >> } >> EXPORT_SYMBOL_GPL(tpm_tis_remove); >> >> @@ -736,6 +739,9 @@ void tpm_tis_remove(struct tpm_chip *chip) >> * @chip: TPM chip to use >> * @value: 1 - Disable CLKRUN protocol, so that clocks are free running >> * 0 - Enable CLKRUN protocol >> + * >> + * Call this function directly in tpm_tis_remove() in error or driver >> + removal >> + * path, since the chip->ops is set to NULL in tpm_chip_unregister(). >> */ >> static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) >> { @@ -922,21 +928,20 @@ 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 (rc) >> + goto out_err; >> >> if (chip->ops->clk_enable != NULL) >> chip->ops->clk_enable(chip, false); >> >> - return rc; >> -out_err: >> - tpm_tis_remove(chip); >> - if (is_bsw()) >> - iounmap(priv->ilb_base_addr); >> + return 0; >> >> - if (chip->ops->clk_enable != NULL) >> +out_err: >> + if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL)) >> chip->ops->clk_enable(chip, false); >> >> + tpm_tis_remove(chip); >> + >> return rc; >> } >> EXPORT_SYMBOL_GPL(tpm_tis_core_init); >> -- >> 1.9.1 >> > >Since the original commit is not in the mainline yet, you should rather >send an updated version of that commit. > Ok, I will send an updated version of the original commit "tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()" >/Jarkko Regards, Azhar Shaikh
>-----Original Message----- >From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- >owner@vger.kernel.org] On Behalf Of Shaikh, Azhar >Sent: Friday, December 22, 2017 11:15 AM >To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >Cc: jgg@ziepe.ca; javierm@redhat.com; peterhuewe@gmx.de; linux-security- >module@vger.kernel.org; linux-integrity@vger.kernel.org; linux- >kernel@vger.kernel.org >Subject: RE: [PATCH v2] tpm: Fix the driver cleanup code > > > >>-----Original Message----- >>From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] >>Sent: Friday, December 22, 2017 10:42 AM >>To: Shaikh, Azhar <azhar.shaikh@intel.com> >>Cc: jgg@ziepe.ca; javierm@redhat.com; peterhuewe@gmx.de; >>linux-security- module@vger.kernel.org; >>linux-integrity@vger.kernel.org; linux- kernel@vger.kernel.org >>Subject: Re: [PATCH v2] tpm: Fix the driver cleanup code >> >>On Thu, Dec 21, 2017 at 03:13:39PM -0800, Azhar Shaikh wrote: >>> Commit 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout the >>> duration of transmit_cmd()") added code which accessed >>> chip->ops, even after it was set to NULL in tpm_del_char_device(), >>> called from tpm_chip_unregister() in error / driver exit paths. >>> So fix this code. >>> >>> Fixes: 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout the >>> duration of transmit_cmd()") >>> >>> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> >>> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> >>> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> >>> --- >>> Changes in v2: >>> - Remove TPM_CHIP_FLAG_DO_NOT_CLEAR_OPS flag, instead call >>> tpm_tis_clkrun_enable() directly in tpm_tis_remove() >>> - Add comment to tpm_tis_clkrun_enable() >>> >>> drivers/char/tpm/tpm_tis.c | 6 ------ >>> drivers/char/tpm/tpm_tis_core.c | 29 +++++++++++++++++------------ >>> 2 files changed, 17 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c >>> index d29add49b033..c847fc69a2fc 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 = { @@ -329,9 +326,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 c2227983ed88..519e4a78c8f8 >>> 100644 >>> --- a/drivers/char/tpm/tpm_tis_core.c >>> +++ b/drivers/char/tpm/tpm_tis_core.c >>> @@ -37,6 +37,8 @@ >>> */ >>> #define TPM_POLL_SLEEP 1 /* msec */ >>> >>> +static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool >>> +value); >>> + >>> static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, >>> bool check_cancel, bool *canceled) { >>@@ -716,8 +718,7 @@ void >>> tpm_tis_remove(struct tpm_chip *chip) >>> u32 interrupt; >>> int rc; >>> >>> - if (chip->ops->clk_enable != NULL) >>> - chip->ops->clk_enable(chip, true); >>> + tpm_tis_clkrun_enable(chip, true); >>> >>> rc = tpm_tis_read32(priv, reg, &interrupt); >>> if (rc < 0) >>> @@ -725,8 +726,10 @@ void tpm_tis_remove(struct tpm_chip *chip) >>> >>> tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt); >>> >>> - if (chip->ops->clk_enable != NULL) >>> - chip->ops->clk_enable(chip, false); >>> + tpm_tis_clkrun_enable(chip, false); >>> + >>> + if (priv->ilb_base_addr) >>> + iounmap(priv->ilb_base_addr); >>> } >>> EXPORT_SYMBOL_GPL(tpm_tis_remove); >>> >>> @@ -736,6 +739,9 @@ void tpm_tis_remove(struct tpm_chip *chip) >>> * @chip: TPM chip to use >>> * @value: 1 - Disable CLKRUN protocol, so that clocks are free running >>> * 0 - Enable CLKRUN protocol >>> + * >>> + * Call this function directly in tpm_tis_remove() in error or >>> + driver removal >>> + * path, since the chip->ops is set to NULL in tpm_chip_unregister(). >>> */ >>> static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) >>> { @@ -922,21 +928,20 @@ 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 (rc) >>> + goto out_err; >>> >>> if (chip->ops->clk_enable != NULL) >>> chip->ops->clk_enable(chip, false); >>> >>> - return rc; >>> -out_err: >>> - tpm_tis_remove(chip); >>> - if (is_bsw()) >>> - iounmap(priv->ilb_base_addr); >>> + return 0; >>> >>> - if (chip->ops->clk_enable != NULL) >>> +out_err: >>> + if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL)) >>> chip->ops->clk_enable(chip, false); >>> >>> + tpm_tis_remove(chip); >>> + >>> return rc; >>> } >>> EXPORT_SYMBOL_GPL(tpm_tis_core_init); >>> -- >>> 1.9.1 >>> >> >>Since the original commit is not in the mainline yet, you should rather >>send an updated version of that commit. >> > >Ok, I will send an updated version of the original commit "tpm: Keep CLKRUN >enabled throughout the duration of transmit_cmd()" Should I send the updated commit on top of these 4? https://patchwork.kernel.org/patch/10125543/ https://patchwork.kernel.org/patch/10125535/ https://patchwork.kernel.org/patch/10125509/ https://patchwork.kernel.org/patch/10125511/ > >>/Jarkko > >Regards, >Azhar Shaikh Regards, Azhar Shaikh
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index d29add49b033..c847fc69a2fc 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 = { @@ -329,9 +326,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 c2227983ed88..519e4a78c8f8 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -37,6 +37,8 @@ */ #define TPM_POLL_SLEEP 1 /* msec */ +static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); + static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, bool check_cancel, bool *canceled) { @@ -716,8 +718,7 @@ void tpm_tis_remove(struct tpm_chip *chip) u32 interrupt; int rc; - if (chip->ops->clk_enable != NULL) - chip->ops->clk_enable(chip, true); + tpm_tis_clkrun_enable(chip, true); rc = tpm_tis_read32(priv, reg, &interrupt); if (rc < 0) @@ -725,8 +726,10 @@ void tpm_tis_remove(struct tpm_chip *chip) tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt); - if (chip->ops->clk_enable != NULL) - chip->ops->clk_enable(chip, false); + tpm_tis_clkrun_enable(chip, false); + + if (priv->ilb_base_addr) + iounmap(priv->ilb_base_addr); } EXPORT_SYMBOL_GPL(tpm_tis_remove); @@ -736,6 +739,9 @@ void tpm_tis_remove(struct tpm_chip *chip) * @chip: TPM chip to use * @value: 1 - Disable CLKRUN protocol, so that clocks are free running * 0 - Enable CLKRUN protocol + * + * Call this function directly in tpm_tis_remove() in error or driver removal + * path, since the chip->ops is set to NULL in tpm_chip_unregister(). */ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) { @@ -922,21 +928,20 @@ 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 (rc) + goto out_err; if (chip->ops->clk_enable != NULL) chip->ops->clk_enable(chip, false); - return rc; -out_err: - tpm_tis_remove(chip); - if (is_bsw()) - iounmap(priv->ilb_base_addr); + return 0; - if (chip->ops->clk_enable != NULL) +out_err: + if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL)) chip->ops->clk_enable(chip, false); + tpm_tis_remove(chip); + return rc; } EXPORT_SYMBOL_GPL(tpm_tis_core_init);
Commit 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()") added code which accessed chip->ops, even after it was set to NULL in tpm_del_char_device(), called from tpm_chip_unregister() in error / driver exit paths. So fix this code. Fixes: 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()") Suggested-by: Javier Martinez Canillas <javierm@redhat.com> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> --- Changes in v2: - Remove TPM_CHIP_FLAG_DO_NOT_CLEAR_OPS flag, instead call tpm_tis_clkrun_enable() directly in tpm_tis_remove() - Add comment to tpm_tis_clkrun_enable() drivers/char/tpm/tpm_tis.c | 6 ------ drivers/char/tpm/tpm_tis_core.c | 29 +++++++++++++++++------------ 2 files changed, 17 insertions(+), 18 deletions(-)