| Message ID | 20180306092503.11568-4-tomas.winkler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-03-06 at 11:25 +0200, Tomas Winkler wrote: > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation > of crypto keys which can be a computationally intensive task. > The timeout is set to 3min. > Rather than increasing default timeout a new constant is > added, to not stall for too long on regular commands failures. > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> Why are you radically chaging the default timeout? The commit message does not tell anything about that change. Why couldn't we just have two timeouts: one default and one long that would be at least as long as the longest timeout defined in the spec? Then we would not have to ever update LAST_CC constant, we could actually remove it completely. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiBPbiBUdWUsIDIwMTgtMDMtMDYgYXQgMTE6MjUgKzAyMDAsIFRvbWFzIFdpbmtsZXIgd3JvdGU6 DQo+ID4gVFBNMl9DQ19DcmVhdGUoMHgxNTMpIGFuZCBUUE0yX0NDX0NyZWF0ZVByaW1hcnkgKDB4 MTMxKSBpbnZvbHZlDQo+ID4gZ2VuZXJhdGlvbiBvZiBjcnlwdG8ga2V5cyB3aGljaCBjYW4gYmUg YSBjb21wdXRhdGlvbmFsbHkgaW50ZW5zaXZlIHRhc2suDQo+ID4gVGhlIHRpbWVvdXQgaXMgc2V0 IHRvIDNtaW4uDQo+ID4gUmF0aGVyIHRoYW4gaW5jcmVhc2luZyBkZWZhdWx0IHRpbWVvdXQgYSBu ZXcgY29uc3RhbnQgaXMgYWRkZWQsIHRvIG5vdA0KPiA+IHN0YWxsIGZvciB0b28gbG9uZyBvbiBy ZWd1bGFyIGNvbW1hbmRzIGZhaWx1cmVzLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogVG9tYXMg V2lua2xlciA8dG9tYXMud2lua2xlckBpbnRlbC5jb20+DQo+IA0KPiBXaHkgYXJlIHlvdSByYWRp Y2FsbHkgY2hhZ2luZyB0aGUgZGVmYXVsdCB0aW1lb3V0PyBUaGUgY29tbWl0IG1lc3NhZ2UNCj4g ZG9lcyBub3QgdGVsbCBhbnl0aGluZyBhYm91dCB0aGF0IGNoYW5nZS4NCj4NCkxldCBtZSwgcmVj aGVjayBidXQgaXQgc2hvdWxkIGJlIHNhbWUgdmFsdWUganVzdCBjb252ZXJ0ZWQgdG8gbXNlY3Mu DQogDQo+IFdoeSBjb3VsZG4ndCB3ZSBqdXN0IGhhdmUgdHdvIHRpbWVvdXRzOiBvbmUgZGVmYXVs dCBhbmQgb25lIGxvbmcgdGhhdA0KPiB3b3VsZCBiZSBhdCBsZWFzdCBhcyBsb25nIGFzIHRoZSBs b25nZXN0IHRpbWVvdXQgZGVmaW5lZCBpbiB0aGUgc3BlYz8NCg0KSSd2ZSB0cmllZCB0byBleHBs YWluIGl0IGluIHRoZSBjb21taXQgbWVzc2FnZSBidXQgYXBwYXJlbnRseSBoYXMgZmFpbGVkLiAN Cg0KV2UgaGF2ZSBhIGRlZmF1bHQgb3IgdW5kZWZpbmVkIHdoaWNoIHNob3VsZCBiZSB0aGUgc2Ft ZSBhcyBpdCB3YXMgdW5sZXNzIEkgZGlkIHNvbWUgc2lsbHkgbWlzdGFrZSBpbiBjb252ZXJzaW9u IHRvIG1zZWNzICh3aWxsIGNoZWNrKSwgYXNzIGFsbCBvdGhlcnMgYXJlIGluIG1zZWNzLg0KSXQg d2FzIDIgbWluID0gMiAqIDYwICogSFogKGluIGppZmZpZXMpIHdoaWNoIHdvdWxkIGJlIG1zZWNz X3RvX2ppZmZpZXMoMiAqIDYwICogMTAwMCA9IDEyMDAwMCkgDQpUUE0yX0RVUkFUSU9OX0RFRkFV TFQgICA9IDEyMDAwMA0KDQpUaGVuIHdlIGhhdmUgYSBuZXcgb25lIHdoaWNoIGlzIDMgbWluIGp1 c3QgZm9yIGtleSBnZW5lcmF0aW9uIChQcmltZSBudW1iZXIgZ2VuZXJhdGlvbiBpcyBoZWF2eSBs aWZmdGluZykgDQoNCj4gVGhlbiB3ZSB3b3VsZCBub3QgaGF2ZSB0byBldmVyIHVwZGF0ZSBMQVNU X0NDIGNvbnN0YW50LCB3ZSBjb3VsZCBhY3R1YWxseQ0KPiByZW1vdmUgaXQgY29tcGxldGVseS4N Cg0KSSBiZWxpZXZlIEkndmUgcmVtb3ZlZCBpdCBuZXh0IHBhdGNoLCBJIGRpZG7igJl0IHdhbnQg ZG8gdG9vIG11Y2ggaW4gdGhpcyBvbmUuDQoNClRoYW5rcw0KVG9tYXMNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2018-03-06 at 15:19 +0000, Winkler, Tomas wrote: > > On Tue, 2018-03-06 at 11:25 +0200, Tomas Winkler wrote: > > > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve > > > generation of crypto keys which can be a computationally intensive task. > > > The timeout is set to 3min. > > > Rather than increasing default timeout a new constant is added, to not > > > stall for too long on regular commands failures. > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > > > Why are you radically chaging the default timeout? The commit message > > does not tell anything about that change. > > > > Let me, recheck but it should be same value just converted to msecs. > > > Why couldn't we just have two timeouts: one default and one long that > > would be at least as long as the longest timeout defined in the spec? > > I've tried to explain it in the commit message but apparently has failed. > > We have a default or undefined which should be the same as it was unless I did > some silly mistake in conversion to msecs (will check), ass all others are in > msecs. > It was 2 min = 2 * 60 * HZ (in jiffies) which would be msecs_to_jiffies(2 * 60 > * 1000 = 120000) > TPM2_DURATION_DEFAULT = 120000 Aah, of course :-) The problem was that I had somehow a blid spot with seeing the msec_to_jiffies() conversion. TPM_NUM_DURATIONS would a better name than TPM_DURATION_MAX because TPM_DURATION_MAX is easy to confuse with TPM_DURATION_* constants. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 402e54252b22..40d1770f6b38 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -421,6 +421,8 @@ int tpm_get_timeouts(struct tpm_chip *chip) msecs_to_jiffies(TPM2_DURATION_MEDIUM); chip->duration[TPM_LONG] = msecs_to_jiffies(TPM2_DURATION_LONG); + chip->duration[TPM_LONG_LONG] = + msecs_to_jiffies(TPM2_DURATION_LONG_LONG); chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS; return 0; @@ -509,6 +511,7 @@ int tpm_get_timeouts(struct tpm_chip *chip) usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_medium)); chip->duration[TPM_LONG] = usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long)); + chip->duration[TPM_LONG_LONG] = 0; /* not used under 1.2 */ /* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above * value wrong and apparently reports msecs rather than usecs. So we diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 38197a30ad7b..30610d97d30c 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -67,7 +67,9 @@ enum tpm_duration { TPM_SHORT = 0, TPM_MEDIUM = 1, TPM_LONG = 2, + TPM_LONG_LONG = 3, TPM_UNDEFINED, + TPM_DURATION_MAX = TPM_UNDEFINED, }; #define TPM_WARN_RETRY 0x800 @@ -79,15 +81,20 @@ enum tpm_duration { #define TPM_HEADER_SIZE 10 enum tpm2_const { - TPM2_PLATFORM_PCR = 24, - TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8), - TPM2_TIMEOUT_A = 750, - TPM2_TIMEOUT_B = 2000, - TPM2_TIMEOUT_C = 200, - TPM2_TIMEOUT_D = 30, - TPM2_DURATION_SHORT = 20, - TPM2_DURATION_MEDIUM = 750, - TPM2_DURATION_LONG = 2000, + TPM2_PLATFORM_PCR = 24, + TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8), +}; + +enum tpm2_timeouts { + TPM2_TIMEOUT_A = 750, + TPM2_TIMEOUT_B = 2000, + TPM2_TIMEOUT_C = 200, + TPM2_TIMEOUT_D = 30, + TPM2_DURATION_SHORT = 20, + TPM2_DURATION_MEDIUM = 750, + TPM2_DURATION_LONG = 2000, + TPM2_DURATION_LONG_LONG = 300000, + TPM2_DURATION_DEFAULT = 120000, }; enum tpm2_structures { @@ -123,6 +130,7 @@ enum tpm2_algorithms { enum tpm2_command_codes { TPM2_CC_FIRST = 0x011F, + TPM2_CC_CREATE_PRIMARY = 0x0131, TPM2_CC_SELF_TEST = 0x0143, TPM2_CC_STARTUP = 0x0144, TPM2_CC_SHUTDOWN = 0x0145, @@ -227,7 +235,7 @@ struct tpm_chip { unsigned long timeout_c; /* jiffies */ unsigned long timeout_d; /* jiffies */ bool timeout_adjusted; - unsigned long duration[3]; /* jiffies */ + unsigned long duration[TPM_DURATION_MAX]; /* jiffies */ bool duration_adjusted; struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index a700f8f9ead7..c1ddbbba406e 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -90,6 +90,8 @@ static struct tpm2_hash tpm2_hash_map[] = { * of time the chip could take to return the result. The values * of the SHORT, MEDIUM, and LONG durations are taken from the * PC Client Profile (PTP) specification. + * LONG_LONG is for commands that generates keys which empirically + * takes longer time on some systems. */ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { TPM_UNDEFINED, /* 11F */ @@ -110,7 +112,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { TPM_UNDEFINED, /* 12e */ TPM_UNDEFINED, /* 12f */ TPM_UNDEFINED, /* 130 */ - TPM_UNDEFINED, /* 131 */ + TPM_LONG_LONG, /* 131 */ TPM_UNDEFINED, /* 132 */ TPM_UNDEFINED, /* 133 */ TPM_UNDEFINED, /* 134 */ @@ -144,7 +146,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { TPM_UNDEFINED, /* 150 */ TPM_UNDEFINED, /* 151 */ TPM_UNDEFINED, /* 152 */ - TPM_UNDEFINED, /* 153 */ + TPM_LONG_LONG, /* 153 */ TPM_UNDEFINED, /* 154 */ TPM_UNDEFINED, /* 155 */ TPM_UNDEFINED, /* 156 */ @@ -821,7 +823,7 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal) duration = chip->duration[index]; if (duration <= 0) - duration = 2 * 60 * HZ; + duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT); return duration; }
TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation of crypto keys which can be a computationally intensive task. The timeout is set to 3min. Rather than increasing default timeout a new constant is added, to not stall for too long on regular commands failures. Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> --- V2: add more explanation to the commit message. drivers/char/tpm/tpm-interface.c | 3 +++ drivers/char/tpm/tpm.h | 28 ++++++++++++++++++---------- drivers/char/tpm/tpm2-cmd.c | 8 +++++--- 3 files changed, 26 insertions(+), 13 deletions(-)