| Message ID | 20210814222519.42476-1-hao.wu@rubrik.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] tpm: fix Atmel TPM crash caused by too frequent queries | expand |
> On Aug 14, 2021, at 3:25 PM, Hao Wu <hao.wu@rubrik.com> wrote: > > The Atmel TPM 1.2 chips crash with error > `tpm_try_transmit: send(): error -62` since kernel 4.14. > It is observed from the kernel log after running `tpm_sealdata -z`. > The error thrown from the command is as follows > ``` > $ tpm_sealdata -z > Tspi_Key_LoadKey failed: 0x00001087 - layer=tddl, > code=0087 (135), I/O error > ``` > > The issue was reproduced with the following Atmel TPM chip: > ``` > $ tpm_version > T0 TPM 1.2 Version Info: > Chip Version: 1.2.66.1 > Spec Level: 2 > Errata Revision: 3 > TPM Vendor ID: ATML > TPM Version: 01010000 > Manufacturer Info: 41544d4c > ``` > > The root cause of the issue is due to the TPM calls to msleep() > were replaced with usleep_range() [1], which reduces > the actual timeout. Via experiments, it is observed that > the original msleep(5) actually sleeps for 15ms. > Because of a known timeout issue in Atmel TPM 1.2 chip, > the shorter timeout than 15ms can cause the error described above. > > A few further changes in kernel 4.16 [2] and 4.18 [3, 4] further > reduced the timeout to less than 1ms. With experiments, > the problematic timeout in the latest kernel is the one > for `wait_for_tpm_stat`. > > To fix it, the patch reverts the timeout of `wait_for_tpm_stat` > to 15ms for all Atmel TPM 1.2 chips, but leave it untouched > for Ateml TPM 2.0 chip, and chips from other vendors. > As explained above, the chosen 15ms timeout is > the actual timeout before this issue introduced, > thus the old value is used here. > Particularly, TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 14700us, > TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 15000us according to > the existing TPM_TIMEOUT_RANGE_US (300us). > The fixed has been tested in the system with the affected Atmel chip > with no issues observed after boot up. > > References: > [1] 9f3fc7bcddcb tpm: replace msleep() with usleep_range() in TPM > 1.2/2.0 generic drivers > [2] cf151a9a44d5 tpm: reduce tpm polling delay in tpm_tis_core > [3] 59f5a6b07f64 tpm: reduce poll sleep time in tpm_transmit() > [4] 424eaf910c32 tpm: reduce polling time to usecs for even finer > granularity > > Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers") > Link: https://patchwork.kernel.org/project/linux-integrity/patch/20200926223150.109645-1-hao.wu@rubrik.com/ > Signed-off-by: Hao Wu <hao.wu@rubrik.com> > --- > v4: > - Move timeout constants to drivers/char/tpm/tpm_tis_core.h > - Cleanup unnecessary inline comment > > v3: > - removes unnecessary condition check in `wait_for_tpm_stat` > > v2: > - follow the existing way to define two timeouts (min and max) > for ATMEL chip, thus keep the exact timeout logic for > non-ATEML chips. > - limit the timeout increase to only ATMEL TPM 1.2 chips, > because it is not an issue for TPM 2.0 chips yet. > > Test Plan: > - Run fixed kernel with ATMEL TPM chips and see crash > has been fixed. > - Run fixed kernel with non-ATMEL TPM chips, and confirm > the timeout has not been changed. > > drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++-- > drivers/char/tpm/tpm_tis_core.h | 2 ++ > include/linux/tpm.h | 3 +++ > 3 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 55b9d3965ae1..24605f100e96 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -80,8 +80,8 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > } > } else { > do { > - usleep_range(TPM_TIMEOUT_USECS_MIN, > - TPM_TIMEOUT_USECS_MAX); > + usleep_range(chip->timeout_wait_stat_min, > + chip->timeout_wait_stat_max); > status = chip->ops->status(chip); > if ((status & mask) == mask) > return 0; > @@ -934,6 +934,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX); > chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX); > chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX); > + chip->timeout_wait_stat_min = TPM_TIMEOUT_USECS_MIN; > + chip->timeout_wait_stat_max = TPM_TIMEOUT_USECS_MAX; > priv->phy_ops = phy_ops; > dev_set_drvdata(&chip->dev, priv); > > @@ -983,6 +985,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > > priv->manufacturer_id = vendor; > > + if (priv->manufacturer_id == TPM_VID_ATML && > + !(chip->flags & TPM_CHIP_FLAG_TPM2)) { > + /* If TPM chip is 1.2 ATMEL chip, timeout need to be relaxed*/ > + chip->timeout_wait_stat_min = TPM_ATML_TIMEOUT_WAIT_STAT_MIN; > + chip->timeout_wait_stat_max = TPM_ATML_TIMEOUT_WAIT_STAT_MAX; > + } > + > rc = tpm_tis_read8(priv, TPM_RID(0), &rid); > if (rc < 0) > goto out_err; > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > index 9b2d32a59f67..2e431beb44f7 100644 > --- a/drivers/char/tpm/tpm_tis_core.h > +++ b/drivers/char/tpm/tpm_tis_core.h > @@ -54,6 +54,8 @@ enum tis_defaults { > TIS_MEM_LEN = 0x5000, > TIS_SHORT_TIMEOUT = 750, /* ms */ > TIS_LONG_TIMEOUT = 2000, /* 2 sec */ > + TPM_ATML_TIMEOUT_WAIT_STAT_MIN = 14700, /* usecs */ > + TPM_ATML_TIMEOUT_WAIT_STAT_MAX = 15000, /* usecs */ > }; > > /* Some timeout values are needed before it is known whether the chip is > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index aa11fe323c56..171b9102c976 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -150,6 +150,8 @@ struct tpm_chip { > bool timeout_adjusted; > unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */ > bool duration_adjusted; > + unsigned int timeout_wait_stat_min; /* usecs */ > + unsigned int timeout_wait_stat_max; /* usecs */ > > struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; > > @@ -269,6 +271,7 @@ enum tpm2_cc_attrs { > #define TPM_VID_INTEL 0x8086 > #define TPM_VID_WINBOND 0x1050 > #define TPM_VID_STM 0x104A > +#define TPM_VID_ATML 0x1114 > > enum tpm_chip_flags { > TPM_CHIP_FLAG_TPM2 = BIT(1), > -- > 2.29.0.vfs.0.0 > Just kindly remind this code review in case it has been missed somehow Thanks Hao
On Wed, 2021-08-25 at 22:38 -0700, Hao Wu wrote: > > On Aug 14, 2021, at 3:25 PM, Hao Wu <hao.wu@rubrik.com> wrote: > > > > The Atmel TPM 1.2 chips crash with error > > `tpm_try_transmit: send(): error -62` since kernel 4.14. > > It is observed from the kernel log after running `tpm_sealdata -z`. > > The error thrown from the command is as follows > > ``` > > $ tpm_sealdata -z > > Tspi_Key_LoadKey failed: 0x00001087 - layer=tddl, > > code=0087 (135), I/O error > > ``` > > > > The issue was reproduced with the following Atmel TPM chip: > > ``` > > $ tpm_version > > T0 TPM 1.2 Version Info: > > Chip Version: 1.2.66.1 > > Spec Level: 2 > > Errata Revision: 3 > > TPM Vendor ID: ATML > > TPM Version: 01010000 > > Manufacturer Info: 41544d4c > > ``` > > > > The root cause of the issue is due to the TPM calls to msleep() > > were replaced with usleep_range() [1], which reduces > > the actual timeout. Via experiments, it is observed that > > the original msleep(5) actually sleeps for 15ms. > > Because of a known timeout issue in Atmel TPM 1.2 chip, > > the shorter timeout than 15ms can cause the error described above. > > > > A few further changes in kernel 4.16 [2] and 4.18 [3, 4] further > > reduced the timeout to less than 1ms. With experiments, > > the problematic timeout in the latest kernel is the one > > for `wait_for_tpm_stat`. > > > > To fix it, the patch reverts the timeout of `wait_for_tpm_stat` > > to 15ms for all Atmel TPM 1.2 chips, but leave it untouched > > for Ateml TPM 2.0 chip, and chips from other vendors. > > As explained above, the chosen 15ms timeout is > > the actual timeout before this issue introduced, > > thus the old value is used here. > > Particularly, TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 14700us, > > TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 15000us according to > > the existing TPM_TIMEOUT_RANGE_US (300us). > > The fixed has been tested in the system with the affected Atmel chip > > with no issues observed after boot up. > > > > References: > > [1] 9f3fc7bcddcb tpm: replace msleep() with usleep_range() in TPM > > 1.2/2.0 generic drivers > > [2] cf151a9a44d5 tpm: reduce tpm polling delay in tpm_tis_core > > [3] 59f5a6b07f64 tpm: reduce poll sleep time in tpm_transmit() > > [4] 424eaf910c32 tpm: reduce polling time to usecs for even finer > > granularity > > > > Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers") > > Link: https://patchwork.kernel.org/project/linux-integrity/patch/20200926223150.109645-1-hao.wu@rubrik.com/ > > Signed-off-by: Hao Wu <hao.wu@rubrik.com> > > --- > > v4: > > - Move timeout constants to drivers/char/tpm/tpm_tis_core.h > > - Cleanup unnecessary inline comment > > > > v3: > > - removes unnecessary condition check in `wait_for_tpm_stat` > > > > v2: > > - follow the existing way to define two timeouts (min and max) > > for ATMEL chip, thus keep the exact timeout logic for > > non-ATEML chips. > > - limit the timeout increase to only ATMEL TPM 1.2 chips, > > because it is not an issue for TPM 2.0 chips yet. > > > > Test Plan: > > - Run fixed kernel with ATMEL TPM chips and see crash > > has been fixed. > > - Run fixed kernel with non-ATMEL TPM chips, and confirm > > the timeout has not been changed. > > > > drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++-- > > drivers/char/tpm/tpm_tis_core.h | 2 ++ > > include/linux/tpm.h | 3 +++ > > 3 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > > index 55b9d3965ae1..24605f100e96 100644 > > --- a/drivers/char/tpm/tpm_tis_core.c > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -80,8 +80,8 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > > } > > } else { > > do { > > - usleep_range(TPM_TIMEOUT_USECS_MIN, > > - TPM_TIMEOUT_USECS_MAX); > > + usleep_range(chip->timeout_wait_stat_min, > > + chip->timeout_wait_stat_max); > > status = chip->ops->status(chip); > > if ((status & mask) == mask) > > return 0; > > @@ -934,6 +934,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > > chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX); > > chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX); > > chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX); > > + chip->timeout_wait_stat_min = TPM_TIMEOUT_USECS_MIN; > > + chip->timeout_wait_stat_max = TPM_TIMEOUT_USECS_MAX; > > priv->phy_ops = phy_ops; > > dev_set_drvdata(&chip->dev, priv); > > > > @@ -983,6 +985,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > > > > priv->manufacturer_id = vendor; > > > > + if (priv->manufacturer_id == TPM_VID_ATML && > > + !(chip->flags & TPM_CHIP_FLAG_TPM2)) { > > + /* If TPM chip is 1.2 ATMEL chip, timeout need to be relaxed*/ > > + chip->timeout_wait_stat_min = TPM_ATML_TIMEOUT_WAIT_STAT_MIN; > > + chip->timeout_wait_stat_max = TPM_ATML_TIMEOUT_WAIT_STAT_MAX; > > + } > > + > > rc = tpm_tis_read8(priv, TPM_RID(0), &rid); > > if (rc < 0) > > goto out_err; > > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > > index 9b2d32a59f67..2e431beb44f7 100644 > > --- a/drivers/char/tpm/tpm_tis_core.h > > +++ b/drivers/char/tpm/tpm_tis_core.h > > @@ -54,6 +54,8 @@ enum tis_defaults { > > TIS_MEM_LEN = 0x5000, > > TIS_SHORT_TIMEOUT = 750, /* ms */ > > TIS_LONG_TIMEOUT = 2000, /* 2 sec */ > > + TPM_ATML_TIMEOUT_WAIT_STAT_MIN = 14700, /* usecs */ > > + TPM_ATML_TIMEOUT_WAIT_STAT_MAX = 15000, /* usecs */ > > }; I'd prefer TIS_TIMEOUT_{MIN, MAX}_ATML. I.e. no "WAIT_STAT" and without "TPM_" to be consistent with other constants here. > > > > /* Some timeout values are needed before it is known whether the chip is > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > > index aa11fe323c56..171b9102c976 100644 > > --- a/include/linux/tpm.h > > +++ b/include/linux/tpm.h > > @@ -150,6 +150,8 @@ struct tpm_chip { > > bool timeout_adjusted; > > unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */ > > bool duration_adjusted; > > + unsigned int timeout_wait_stat_min; /* usecs */ > > + unsigned int timeout_wait_stat_max; /* usecs */ Please rename as timeout_{min, max}. And I think tpm_chip is wrong place to put them as they are TIS specific, i.e. they should be in tpm_tis_data. > > > > struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; > > > > @@ -269,6 +271,7 @@ enum tpm2_cc_attrs { > > #define TPM_VID_INTEL 0x8086 > > #define TPM_VID_WINBOND 0x1050 > > #define TPM_VID_STM 0x104A > > +#define TPM_VID_ATML 0x1114 > > > > enum tpm_chip_flags { > > TPM_CHIP_FLAG_TPM2 = BIT(1), > > -- > > 2.29.0.vfs.0.0 > > > > Just kindly remind this code review in case it has been missed somehow I'm sorry, my bad. I managed to somehow miss this. Might be because I've been recently reorganizing my email accounts. And thanks for pinging so that I spotted it. > Thanks > Hao /Jarkko
> On Aug 26, 2021, at 9:24 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Wed, 2021-08-25 at 22:38 -0700, Hao Wu wrote: >>> On Aug 14, 2021, at 3:25 PM, Hao Wu <hao.wu@rubrik.com> wrote: >>> >>> The Atmel TPM 1.2 chips crash with error >>> `tpm_try_transmit: send(): error -62` since kernel 4.14. >>> It is observed from the kernel log after running `tpm_sealdata -z`. >>> The error thrown from the command is as follows >>> ``` >>> $ tpm_sealdata -z >>> Tspi_Key_LoadKey failed: 0x00001087 - layer=tddl, >>> code=0087 (135), I/O error >>> ``` >>> >>> The issue was reproduced with the following Atmel TPM chip: >>> ``` >>> $ tpm_version >>> T0 TPM 1.2 Version Info: >>> Chip Version: 1.2.66.1 >>> Spec Level: 2 >>> Errata Revision: 3 >>> TPM Vendor ID: ATML >>> TPM Version: 01010000 >>> Manufacturer Info: 41544d4c >>> ``` >>> >>> The root cause of the issue is due to the TPM calls to msleep() >>> were replaced with usleep_range() [1], which reduces >>> the actual timeout. Via experiments, it is observed that >>> the original msleep(5) actually sleeps for 15ms. >>> Because of a known timeout issue in Atmel TPM 1.2 chip, >>> the shorter timeout than 15ms can cause the error described above. >>> >>> A few further changes in kernel 4.16 [2] and 4.18 [3, 4] further >>> reduced the timeout to less than 1ms. With experiments, >>> the problematic timeout in the latest kernel is the one >>> for `wait_for_tpm_stat`. >>> >>> To fix it, the patch reverts the timeout of `wait_for_tpm_stat` >>> to 15ms for all Atmel TPM 1.2 chips, but leave it untouched >>> for Ateml TPM 2.0 chip, and chips from other vendors. >>> As explained above, the chosen 15ms timeout is >>> the actual timeout before this issue introduced, >>> thus the old value is used here. >>> Particularly, TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 14700us, >>> TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 15000us according to >>> the existing TPM_TIMEOUT_RANGE_US (300us). >>> The fixed has been tested in the system with the affected Atmel chip >>> with no issues observed after boot up. >>> >>> References: >>> [1] 9f3fc7bcddcb tpm: replace msleep() with usleep_range() in TPM >>> 1.2/2.0 generic drivers >>> [2] cf151a9a44d5 tpm: reduce tpm polling delay in tpm_tis_core >>> [3] 59f5a6b07f64 tpm: reduce poll sleep time in tpm_transmit() >>> [4] 424eaf910c32 tpm: reduce polling time to usecs for even finer >>> granularity >>> >>> Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers") >>> Link: https://patchwork.kernel.org/project/linux-integrity/patch/20200926223150.109645-1-hao.wu@rubrik.com/ >>> Signed-off-by: Hao Wu <hao.wu@rubrik.com> >>> --- >>> v4: >>> - Move timeout constants to drivers/char/tpm/tpm_tis_core.h >>> - Cleanup unnecessary inline comment >>> >>> v3: >>> - removes unnecessary condition check in `wait_for_tpm_stat` >>> >>> v2: >>> - follow the existing way to define two timeouts (min and max) >>> for ATMEL chip, thus keep the exact timeout logic for >>> non-ATEML chips. >>> - limit the timeout increase to only ATMEL TPM 1.2 chips, >>> because it is not an issue for TPM 2.0 chips yet. >>> >>> Test Plan: >>> - Run fixed kernel with ATMEL TPM chips and see crash >>> has been fixed. >>> - Run fixed kernel with non-ATMEL TPM chips, and confirm >>> the timeout has not been changed. >>> >>> drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++-- >>> drivers/char/tpm/tpm_tis_core.h | 2 ++ >>> include/linux/tpm.h | 3 +++ >>> 3 files changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >>> index 55b9d3965ae1..24605f100e96 100644 >>> --- a/drivers/char/tpm/tpm_tis_core.c >>> +++ b/drivers/char/tpm/tpm_tis_core.c >>> @@ -80,8 +80,8 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, >>> } >>> } else { >>> do { >>> - usleep_range(TPM_TIMEOUT_USECS_MIN, >>> - TPM_TIMEOUT_USECS_MAX); >>> + usleep_range(chip->timeout_wait_stat_min, >>> + chip->timeout_wait_stat_max); >>> status = chip->ops->status(chip); >>> if ((status & mask) == mask) >>> return 0; >>> @@ -934,6 +934,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >>> chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX); >>> chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX); >>> chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX); >>> + chip->timeout_wait_stat_min = TPM_TIMEOUT_USECS_MIN; >>> + chip->timeout_wait_stat_max = TPM_TIMEOUT_USECS_MAX; >>> priv->phy_ops = phy_ops; >>> dev_set_drvdata(&chip->dev, priv); >>> >>> @@ -983,6 +985,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >>> >>> priv->manufacturer_id = vendor; >>> >>> + if (priv->manufacturer_id == TPM_VID_ATML && >>> + !(chip->flags & TPM_CHIP_FLAG_TPM2)) { >>> + /* If TPM chip is 1.2 ATMEL chip, timeout need to be relaxed*/ >>> + chip->timeout_wait_stat_min = TPM_ATML_TIMEOUT_WAIT_STAT_MIN; >>> + chip->timeout_wait_stat_max = TPM_ATML_TIMEOUT_WAIT_STAT_MAX; >>> + } >>> + >>> rc = tpm_tis_read8(priv, TPM_RID(0), &rid); >>> if (rc < 0) >>> goto out_err; >>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h >>> index 9b2d32a59f67..2e431beb44f7 100644 >>> --- a/drivers/char/tpm/tpm_tis_core.h >>> +++ b/drivers/char/tpm/tpm_tis_core.h >>> @@ -54,6 +54,8 @@ enum tis_defaults { >>> TIS_MEM_LEN = 0x5000, >>> TIS_SHORT_TIMEOUT = 750, /* ms */ >>> TIS_LONG_TIMEOUT = 2000, /* 2 sec */ >>> + TPM_ATML_TIMEOUT_WAIT_STAT_MIN = 14700, /* usecs */ >>> + TPM_ATML_TIMEOUT_WAIT_STAT_MAX = 15000, /* usecs */ >>> }; > > I'd prefer TIS_TIMEOUT_{MIN, MAX}_ATML. I.e. no "WAIT_STAT" and without "TPM_" > to be consistent with other constants here. Ok will do > >>> >>> /* Some timeout values are needed before it is known whether the chip is >>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h >>> index aa11fe323c56..171b9102c976 100644 >>> --- a/include/linux/tpm.h >>> +++ b/include/linux/tpm.h >>> @@ -150,6 +150,8 @@ struct tpm_chip { >>> bool timeout_adjusted; >>> unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */ >>> bool duration_adjusted; >>> + unsigned int timeout_wait_stat_min; /* usecs */ >>> + unsigned int timeout_wait_stat_max; /* usecs */ > > Please rename as timeout_{min, max}. Ok will do > > And I think tpm_chip is wrong place to put them as they are TIS > specific, i.e. they should be in tpm_tis_data. Sorry, I am not familiar with tpm_tis_data, could tell the the place that you want me to put the var? I think I may have hard time to move forward according toward this comment due to bandwidth constraints. Some helps would be appreciated. Is tpm_tis_data something specific to a chip instance ? Given the values are tied to chip, we need chip specific instance to make this work. > >>> >>> struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; >>> >>> @@ -269,6 +271,7 @@ enum tpm2_cc_attrs { >>> #define TPM_VID_INTEL 0x8086 >>> #define TPM_VID_WINBOND 0x1050 >>> #define TPM_VID_STM 0x104A >>> +#define TPM_VID_ATML 0x1114 >>> >>> enum tpm_chip_flags { >>> TPM_CHIP_FLAG_TPM2 = BIT(1), >>> -- >>> 2.29.0.vfs.0.0 >>> >> >> Just kindly remind this code review in case it has been missed somehow > > I'm sorry, my bad. I managed to somehow miss this. Might be because > I've been recently reorganizing my email accounts. And thanks for > pinging so that I spotted it. No worries, thanks for quick response! > >> Thanks >> Hao > > /Jarkko Hao
> On Aug 26, 2021, at 5:35 PM, Hao Wu <hao.wu@rubrik.com> wrote: > >> On Aug 26, 2021, at 9:24 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: >> >> On Wed, 2021-08-25 at 22:38 -0700, Hao Wu wrote: >>>> On Aug 14, 2021, at 3:25 PM, Hao Wu <hao.wu@rubrik.com> wrote: >>>> >>>> The Atmel TPM 1.2 chips crash with error >>>> `tpm_try_transmit: send(): error -62` since kernel 4.14. >>>> It is observed from the kernel log after running `tpm_sealdata -z`. >>>> The error thrown from the command is as follows >>>> ``` >>>> $ tpm_sealdata -z >>>> Tspi_Key_LoadKey failed: 0x00001087 - layer=tddl, >>>> code=0087 (135), I/O error >>>> ``` >>>> >>>> The issue was reproduced with the following Atmel TPM chip: >>>> ``` >>>> $ tpm_version >>>> T0 TPM 1.2 Version Info: >>>> Chip Version: 1.2.66.1 >>>> Spec Level: 2 >>>> Errata Revision: 3 >>>> TPM Vendor ID: ATML >>>> TPM Version: 01010000 >>>> Manufacturer Info: 41544d4c >>>> ``` >>>> >>>> The root cause of the issue is due to the TPM calls to msleep() >>>> were replaced with usleep_range() [1], which reduces >>>> the actual timeout. Via experiments, it is observed that >>>> the original msleep(5) actually sleeps for 15ms. >>>> Because of a known timeout issue in Atmel TPM 1.2 chip, >>>> the shorter timeout than 15ms can cause the error described above. >>>> >>>> A few further changes in kernel 4.16 [2] and 4.18 [3, 4] further >>>> reduced the timeout to less than 1ms. With experiments, >>>> the problematic timeout in the latest kernel is the one >>>> for `wait_for_tpm_stat`. >>>> >>>> To fix it, the patch reverts the timeout of `wait_for_tpm_stat` >>>> to 15ms for all Atmel TPM 1.2 chips, but leave it untouched >>>> for Ateml TPM 2.0 chip, and chips from other vendors. >>>> As explained above, the chosen 15ms timeout is >>>> the actual timeout before this issue introduced, >>>> thus the old value is used here. >>>> Particularly, TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 14700us, >>>> TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 15000us according to >>>> the existing TPM_TIMEOUT_RANGE_US (300us). >>>> The fixed has been tested in the system with the affected Atmel chip >>>> with no issues observed after boot up. >>>> >>>> References: >>>> [1] 9f3fc7bcddcb tpm: replace msleep() with usleep_range() in TPM >>>> 1.2/2.0 generic drivers >>>> [2] cf151a9a44d5 tpm: reduce tpm polling delay in tpm_tis_core >>>> [3] 59f5a6b07f64 tpm: reduce poll sleep time in tpm_transmit() >>>> [4] 424eaf910c32 tpm: reduce polling time to usecs for even finer >>>> granularity >>>> >>>> Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers") >>>> Link: https://patchwork.kernel.org/project/linux-integrity/patch/20200926223150.109645-1-hao.wu@rubrik.com/ >>>> Signed-off-by: Hao Wu <hao.wu@rubrik.com> >>>> --- >>>> v4: >>>> - Move timeout constants to drivers/char/tpm/tpm_tis_core.h >>>> - Cleanup unnecessary inline comment >>>> >>>> v3: >>>> - removes unnecessary condition check in `wait_for_tpm_stat` >>>> >>>> v2: >>>> - follow the existing way to define two timeouts (min and max) >>>> for ATMEL chip, thus keep the exact timeout logic for >>>> non-ATEML chips. >>>> - limit the timeout increase to only ATMEL TPM 1.2 chips, >>>> because it is not an issue for TPM 2.0 chips yet. >>>> >>>> Test Plan: >>>> - Run fixed kernel with ATMEL TPM chips and see crash >>>> has been fixed. >>>> - Run fixed kernel with non-ATMEL TPM chips, and confirm >>>> the timeout has not been changed. >>>> >>>> drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++-- >>>> drivers/char/tpm/tpm_tis_core.h | 2 ++ >>>> include/linux/tpm.h | 3 +++ >>>> 3 files changed, 16 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >>>> index 55b9d3965ae1..24605f100e96 100644 >>>> --- a/drivers/char/tpm/tpm_tis_core.c >>>> +++ b/drivers/char/tpm/tpm_tis_core.c >>>> @@ -80,8 +80,8 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, >>>> } >>>> } else { >>>> do { >>>> - usleep_range(TPM_TIMEOUT_USECS_MIN, >>>> - TPM_TIMEOUT_USECS_MAX); >>>> + usleep_range(chip->timeout_wait_stat_min, >>>> + chip->timeout_wait_stat_max); >>>> status = chip->ops->status(chip); >>>> if ((status & mask) == mask) >>>> return 0; >>>> @@ -934,6 +934,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >>>> chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX); >>>> chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX); >>>> chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX); >>>> + chip->timeout_wait_stat_min = TPM_TIMEOUT_USECS_MIN; >>>> + chip->timeout_wait_stat_max = TPM_TIMEOUT_USECS_MAX; >>>> priv->phy_ops = phy_ops; >>>> dev_set_drvdata(&chip->dev, priv); >>>> >>>> @@ -983,6 +985,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >>>> >>>> priv->manufacturer_id = vendor; >>>> >>>> + if (priv->manufacturer_id == TPM_VID_ATML && >>>> + !(chip->flags & TPM_CHIP_FLAG_TPM2)) { >>>> + /* If TPM chip is 1.2 ATMEL chip, timeout need to be relaxed*/ >>>> + chip->timeout_wait_stat_min = TPM_ATML_TIMEOUT_WAIT_STAT_MIN; >>>> + chip->timeout_wait_stat_max = TPM_ATML_TIMEOUT_WAIT_STAT_MAX; >>>> + } >>>> + >>>> rc = tpm_tis_read8(priv, TPM_RID(0), &rid); >>>> if (rc < 0) >>>> goto out_err; >>>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h >>>> index 9b2d32a59f67..2e431beb44f7 100644 >>>> --- a/drivers/char/tpm/tpm_tis_core.h >>>> +++ b/drivers/char/tpm/tpm_tis_core.h >>>> @@ -54,6 +54,8 @@ enum tis_defaults { >>>> TIS_MEM_LEN = 0x5000, >>>> TIS_SHORT_TIMEOUT = 750, /* ms */ >>>> TIS_LONG_TIMEOUT = 2000, /* 2 sec */ >>>> + TPM_ATML_TIMEOUT_WAIT_STAT_MIN = 14700, /* usecs */ >>>> + TPM_ATML_TIMEOUT_WAIT_STAT_MAX = 15000, /* usecs */ >>>> }; >> >> I'd prefer TIS_TIMEOUT_{MIN, MAX}_ATML. I.e. no "WAIT_STAT" and without "TPM_" >> to be consistent with other constants here. > Ok will do >> >>>> >>>> /* Some timeout values are needed before it is known whether the chip is >>>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h >>>> index aa11fe323c56..171b9102c976 100644 >>>> --- a/include/linux/tpm.h >>>> +++ b/include/linux/tpm.h >>>> @@ -150,6 +150,8 @@ struct tpm_chip { >>>> bool timeout_adjusted; >>>> unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */ >>>> bool duration_adjusted; >>>> + unsigned int timeout_wait_stat_min; /* usecs */ >>>> + unsigned int timeout_wait_stat_max; /* usecs */ >> >> Please rename as timeout_{min, max}. > Ok will do >> >> And I think tpm_chip is wrong place to put them as they are TIS >> specific, i.e. they should be in tpm_tis_data. > Sorry, I am not familiar with tpm_tis_data, could tell the the place that you want me to put the var? > I think I may have hard time to move forward according toward this comment due to bandwidth constraints. > Some helps would be appreciated. > > Is tpm_tis_data something specific to a chip instance ? Given the values are tied to chip, > we need chip specific instance to make this work. Hi Jarkko, I have checked about your proposal a bit. It look slike we need to Run “struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev)” in every wait_for_tpm_stat call. Would this be a performance concern ? If we cache this in tpm_chip instance, it is not the case. Please let me know your thought. Hao >> >>>> >>>> struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; >>>> >>>> @@ -269,6 +271,7 @@ enum tpm2_cc_attrs { >>>> #define TPM_VID_INTEL 0x8086 >>>> #define TPM_VID_WINBOND 0x1050 >>>> #define TPM_VID_STM 0x104A >>>> +#define TPM_VID_ATML 0x1114 >>>> >>>> enum tpm_chip_flags { >>>> TPM_CHIP_FLAG_TPM2 = BIT(1), >>>> -- >>>> 2.29.0.vfs.0.0 >>>> >>> >>> Just kindly remind this code review in case it has been missed somehow >> >> I'm sorry, my bad. I managed to somehow miss this. Might be because >> I've been recently reorganizing my email accounts. And thanks for >> pinging so that I spotted it. > No worries, thanks for quick response! > >> >>> Thanks >>> Hao >> >> /Jarkko > > Hao
> On Sep 4, 2021, at 2:14 PM, Hao Wu <hao.wu@rubrik.com> wrote: > > > >> On Aug 26, 2021, at 5:35 PM, Hao Wu <hao.wu@rubrik.com> wrote: >> >>> On Aug 26, 2021, at 9:24 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: >>> >>> On Wed, 2021-08-25 at 22:38 -0700, Hao Wu wrote: >>>>> On Aug 14, 2021, at 3:25 PM, Hao Wu <hao.wu@rubrik.com> wrote: >>>>> >>>>> The Atmel TPM 1.2 chips crash with error >>>>> `tpm_try_transmit: send(): error -62` since kernel 4.14. >>>>> It is observed from the kernel log after running `tpm_sealdata -z`. >>>>> The error thrown from the command is as follows >>>>> ``` >>>>> $ tpm_sealdata -z >>>>> Tspi_Key_LoadKey failed: 0x00001087 - layer=tddl, >>>>> code=0087 (135), I/O error >>>>> ``` >>>>> >>>>> The issue was reproduced with the following Atmel TPM chip: >>>>> ``` >>>>> $ tpm_version >>>>> T0 TPM 1.2 Version Info: >>>>> Chip Version: 1.2.66.1 >>>>> Spec Level: 2 >>>>> Errata Revision: 3 >>>>> TPM Vendor ID: ATML >>>>> TPM Version: 01010000 >>>>> Manufacturer Info: 41544d4c >>>>> ``` >>>>> >>>>> The root cause of the issue is due to the TPM calls to msleep() >>>>> were replaced with usleep_range() [1], which reduces >>>>> the actual timeout. Via experiments, it is observed that >>>>> the original msleep(5) actually sleeps for 15ms. >>>>> Because of a known timeout issue in Atmel TPM 1.2 chip, >>>>> the shorter timeout than 15ms can cause the error described above. >>>>> >>>>> A few further changes in kernel 4.16 [2] and 4.18 [3, 4] further >>>>> reduced the timeout to less than 1ms. With experiments, >>>>> the problematic timeout in the latest kernel is the one >>>>> for `wait_for_tpm_stat`. >>>>> >>>>> To fix it, the patch reverts the timeout of `wait_for_tpm_stat` >>>>> to 15ms for all Atmel TPM 1.2 chips, but leave it untouched >>>>> for Ateml TPM 2.0 chip, and chips from other vendors. >>>>> As explained above, the chosen 15ms timeout is >>>>> the actual timeout before this issue introduced, >>>>> thus the old value is used here. >>>>> Particularly, TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 14700us, >>>>> TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 15000us according to >>>>> the existing TPM_TIMEOUT_RANGE_US (300us). >>>>> The fixed has been tested in the system with the affected Atmel chip >>>>> with no issues observed after boot up. >>>>> >>>>> References: >>>>> [1] 9f3fc7bcddcb tpm: replace msleep() with usleep_range() in TPM >>>>> 1.2/2.0 generic drivers >>>>> [2] cf151a9a44d5 tpm: reduce tpm polling delay in tpm_tis_core >>>>> [3] 59f5a6b07f64 tpm: reduce poll sleep time in tpm_transmit() >>>>> [4] 424eaf910c32 tpm: reduce polling time to usecs for even finer >>>>> granularity >>>>> >>>>> Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers") >>>>> Link: https://patchwork.kernel.org/project/linux-integrity/patch/20200926223150.109645-1-hao.wu@rubrik.com/ >>>>> Signed-off-by: Hao Wu <hao.wu@rubrik.com> >>>>> --- >>>>> v4: >>>>> - Move timeout constants to drivers/char/tpm/tpm_tis_core.h >>>>> - Cleanup unnecessary inline comment >>>>> >>>>> v3: >>>>> - removes unnecessary condition check in `wait_for_tpm_stat` >>>>> >>>>> v2: >>>>> - follow the existing way to define two timeouts (min and max) >>>>> for ATMEL chip, thus keep the exact timeout logic for >>>>> non-ATEML chips. >>>>> - limit the timeout increase to only ATMEL TPM 1.2 chips, >>>>> because it is not an issue for TPM 2.0 chips yet. >>>>> >>>>> Test Plan: >>>>> - Run fixed kernel with ATMEL TPM chips and see crash >>>>> has been fixed. >>>>> - Run fixed kernel with non-ATMEL TPM chips, and confirm >>>>> the timeout has not been changed. >>>>> >>>>> drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++-- >>>>> drivers/char/tpm/tpm_tis_core.h | 2 ++ >>>>> include/linux/tpm.h | 3 +++ >>>>> 3 files changed, 16 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >>>>> index 55b9d3965ae1..24605f100e96 100644 >>>>> --- a/drivers/char/tpm/tpm_tis_core.c >>>>> +++ b/drivers/char/tpm/tpm_tis_core.c >>>>> @@ -80,8 +80,8 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, >>>>> } >>>>> } else { >>>>> do { >>>>> - usleep_range(TPM_TIMEOUT_USECS_MIN, >>>>> - TPM_TIMEOUT_USECS_MAX); >>>>> + usleep_range(chip->timeout_wait_stat_min, >>>>> + chip->timeout_wait_stat_max); >>>>> status = chip->ops->status(chip); >>>>> if ((status & mask) == mask) >>>>> return 0; >>>>> @@ -934,6 +934,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >>>>> chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX); >>>>> chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX); >>>>> chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX); >>>>> + chip->timeout_wait_stat_min = TPM_TIMEOUT_USECS_MIN; >>>>> + chip->timeout_wait_stat_max = TPM_TIMEOUT_USECS_MAX; >>>>> priv->phy_ops = phy_ops; >>>>> dev_set_drvdata(&chip->dev, priv); >>>>> >>>>> @@ -983,6 +985,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >>>>> >>>>> priv->manufacturer_id = vendor; >>>>> >>>>> + if (priv->manufacturer_id == TPM_VID_ATML && >>>>> + !(chip->flags & TPM_CHIP_FLAG_TPM2)) { >>>>> + /* If TPM chip is 1.2 ATMEL chip, timeout need to be relaxed*/ >>>>> + chip->timeout_wait_stat_min = TPM_ATML_TIMEOUT_WAIT_STAT_MIN; >>>>> + chip->timeout_wait_stat_max = TPM_ATML_TIMEOUT_WAIT_STAT_MAX; >>>>> + } >>>>> + >>>>> rc = tpm_tis_read8(priv, TPM_RID(0), &rid); >>>>> if (rc < 0) >>>>> goto out_err; >>>>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h >>>>> index 9b2d32a59f67..2e431beb44f7 100644 >>>>> --- a/drivers/char/tpm/tpm_tis_core.h >>>>> +++ b/drivers/char/tpm/tpm_tis_core.h >>>>> @@ -54,6 +54,8 @@ enum tis_defaults { >>>>> TIS_MEM_LEN = 0x5000, >>>>> TIS_SHORT_TIMEOUT = 750, /* ms */ >>>>> TIS_LONG_TIMEOUT = 2000, /* 2 sec */ >>>>> + TPM_ATML_TIMEOUT_WAIT_STAT_MIN = 14700, /* usecs */ >>>>> + TPM_ATML_TIMEOUT_WAIT_STAT_MAX = 15000, /* usecs */ >>>>> }; >>> >>> I'd prefer TIS_TIMEOUT_{MIN, MAX}_ATML. I.e. no "WAIT_STAT" and without "TPM_" >>> to be consistent with other constants here. >> Ok will do >>> >>>>> >>>>> /* Some timeout values are needed before it is known whether the chip is >>>>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h >>>>> index aa11fe323c56..171b9102c976 100644 >>>>> --- a/include/linux/tpm.h >>>>> +++ b/include/linux/tpm.h >>>>> @@ -150,6 +150,8 @@ struct tpm_chip { >>>>> bool timeout_adjusted; >>>>> unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */ >>>>> bool duration_adjusted; >>>>> + unsigned int timeout_wait_stat_min; /* usecs */ >>>>> + unsigned int timeout_wait_stat_max; /* usecs */ >>> >>> Please rename as timeout_{min, max}. >> Ok will do To be honest, this naming could be misleading, because the timeout here only applies to wait_stat use case. But I will just follow you suggestion anyway. Hao >>> >>> And I think tpm_chip is wrong place to put them as they are TIS >>> specific, i.e. they should be in tpm_tis_data. >> Sorry, I am not familiar with tpm_tis_data, could tell the the place that you want me to put the var? >> I think I may have hard time to move forward according toward this comment due to bandwidth constraints. >> Some helps would be appreciated. >> >> Is tpm_tis_data something specific to a chip instance ? Given the values are tied to chip, >> we need chip specific instance to make this work. > > Hi Jarkko, I have checked about your proposal a bit. It look slike we need to > Run “struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev)” in every wait_for_tpm_stat call. Would this be a performance concern ? > If we cache this in tpm_chip instance, it is not the case. > > Please let me know your thought. > > Hao > >>> >>>>> >>>>> struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; >>>>> >>>>> @@ -269,6 +271,7 @@ enum tpm2_cc_attrs { >>>>> #define TPM_VID_INTEL 0x8086 >>>>> #define TPM_VID_WINBOND 0x1050 >>>>> #define TPM_VID_STM 0x104A >>>>> +#define TPM_VID_ATML 0x1114 >>>>> >>>>> enum tpm_chip_flags { >>>>> TPM_CHIP_FLAG_TPM2 = BIT(1), >>>>> -- >>>>> 2.29.0.vfs.0.0 >>>>> >>>> >>>> Just kindly remind this code review in case it has been missed somehow >>> >>> I'm sorry, my bad. I managed to somehow miss this. Might be because >>> I've been recently reorganizing my email accounts. And thanks for >>> pinging so that I spotted it. >> No worries, thanks for quick response! >> >>> >>>> Thanks >>>> Hao >>> >>> /Jarkko >> >> Hao >
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 55b9d3965ae1..24605f100e96 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -80,8 +80,8 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, } } else { do { - usleep_range(TPM_TIMEOUT_USECS_MIN, - TPM_TIMEOUT_USECS_MAX); + usleep_range(chip->timeout_wait_stat_min, + chip->timeout_wait_stat_max); status = chip->ops->status(chip); if ((status & mask) == mask) return 0; @@ -934,6 +934,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX); chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX); chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX); + chip->timeout_wait_stat_min = TPM_TIMEOUT_USECS_MIN; + chip->timeout_wait_stat_max = TPM_TIMEOUT_USECS_MAX; priv->phy_ops = phy_ops; dev_set_drvdata(&chip->dev, priv); @@ -983,6 +985,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, priv->manufacturer_id = vendor; + if (priv->manufacturer_id == TPM_VID_ATML && + !(chip->flags & TPM_CHIP_FLAG_TPM2)) { + /* If TPM chip is 1.2 ATMEL chip, timeout need to be relaxed*/ + chip->timeout_wait_stat_min = TPM_ATML_TIMEOUT_WAIT_STAT_MIN; + chip->timeout_wait_stat_max = TPM_ATML_TIMEOUT_WAIT_STAT_MAX; + } + rc = tpm_tis_read8(priv, TPM_RID(0), &rid); if (rc < 0) goto out_err; diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h index 9b2d32a59f67..2e431beb44f7 100644 --- a/drivers/char/tpm/tpm_tis_core.h +++ b/drivers/char/tpm/tpm_tis_core.h @@ -54,6 +54,8 @@ enum tis_defaults { TIS_MEM_LEN = 0x5000, TIS_SHORT_TIMEOUT = 750, /* ms */ TIS_LONG_TIMEOUT = 2000, /* 2 sec */ + TPM_ATML_TIMEOUT_WAIT_STAT_MIN = 14700, /* usecs */ + TPM_ATML_TIMEOUT_WAIT_STAT_MAX = 15000, /* usecs */ }; /* Some timeout values are needed before it is known whether the chip is diff --git a/include/linux/tpm.h b/include/linux/tpm.h index aa11fe323c56..171b9102c976 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -150,6 +150,8 @@ struct tpm_chip { bool timeout_adjusted; unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */ bool duration_adjusted; + unsigned int timeout_wait_stat_min; /* usecs */ + unsigned int timeout_wait_stat_max; /* usecs */ struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; @@ -269,6 +271,7 @@ enum tpm2_cc_attrs { #define TPM_VID_INTEL 0x8086 #define TPM_VID_WINBOND 0x1050 #define TPM_VID_STM 0x104A +#define TPM_VID_ATML 0x1114 enum tpm_chip_flags { TPM_CHIP_FLAG_TPM2 = BIT(1),
The Atmel TPM 1.2 chips crash with error `tpm_try_transmit: send(): error -62` since kernel 4.14. It is observed from the kernel log after running `tpm_sealdata -z`. The error thrown from the command is as follows ``` $ tpm_sealdata -z Tspi_Key_LoadKey failed: 0x00001087 - layer=tddl, code=0087 (135), I/O error ``` The issue was reproduced with the following Atmel TPM chip: ``` $ tpm_version T0 TPM 1.2 Version Info: Chip Version: 1.2.66.1 Spec Level: 2 Errata Revision: 3 TPM Vendor ID: ATML TPM Version: 01010000 Manufacturer Info: 41544d4c ``` The root cause of the issue is due to the TPM calls to msleep() were replaced with usleep_range() [1], which reduces the actual timeout. Via experiments, it is observed that the original msleep(5) actually sleeps for 15ms. Because of a known timeout issue in Atmel TPM 1.2 chip, the shorter timeout than 15ms can cause the error described above. A few further changes in kernel 4.16 [2] and 4.18 [3, 4] further reduced the timeout to less than 1ms. With experiments, the problematic timeout in the latest kernel is the one for `wait_for_tpm_stat`. To fix it, the patch reverts the timeout of `wait_for_tpm_stat` to 15ms for all Atmel TPM 1.2 chips, but leave it untouched for Ateml TPM 2.0 chip, and chips from other vendors. As explained above, the chosen 15ms timeout is the actual timeout before this issue introduced, thus the old value is used here. Particularly, TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 14700us, TPM_ATML_TIMEOUT_WAIT_STAT_MIN is set to 15000us according to the existing TPM_TIMEOUT_RANGE_US (300us). The fixed has been tested in the system with the affected Atmel chip with no issues observed after boot up. References: [1] 9f3fc7bcddcb tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers [2] cf151a9a44d5 tpm: reduce tpm polling delay in tpm_tis_core [3] 59f5a6b07f64 tpm: reduce poll sleep time in tpm_transmit() [4] 424eaf910c32 tpm: reduce polling time to usecs for even finer granularity Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers") Link: https://patchwork.kernel.org/project/linux-integrity/patch/20200926223150.109645-1-hao.wu@rubrik.com/ Signed-off-by: Hao Wu <hao.wu@rubrik.com> --- v4: - Move timeout constants to drivers/char/tpm/tpm_tis_core.h - Cleanup unnecessary inline comment v3: - removes unnecessary condition check in `wait_for_tpm_stat` v2: - follow the existing way to define two timeouts (min and max) for ATMEL chip, thus keep the exact timeout logic for non-ATEML chips. - limit the timeout increase to only ATMEL TPM 1.2 chips, because it is not an issue for TPM 2.0 chips yet. Test Plan: - Run fixed kernel with ATMEL TPM chips and see crash has been fixed. - Run fixed kernel with non-ATMEL TPM chips, and confirm the timeout has not been changed. drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++-- drivers/char/tpm/tpm_tis_core.h | 2 ++ include/linux/tpm.h | 3 +++ 3 files changed, 16 insertions(+), 2 deletions(-)