| Message ID | 20180228195819.22231-2-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-02-28 at 21:58 +0200, Jarkko Sakkinen wrote: > From: Alexander Steffen <Alexander.Steffen@infineon.com> > > My Nuvoton 6xx in a Dell XPS-13 has been intermittently failing to > work (necessitating a reboot). The problem seems to be that the TPM > gets into a state where the partial self-test doesn't return > TPM_RC_SUCCESS (meaning all tests have run to completion), but > instead returns TPM_RC_TESTING (meaning some tests are still running > in the background). There are various theories that resending the > self-test command actually causes the tests to restart and thus > triggers more TPM_RC_TESTING returns until the timeout is exceeded. > > There are several issues here: firstly being we shouldn't slow down > the boot sequence waiting for the self test to complete once the TPM > backgrounds them. It will actually make available all functions that > have passed and if it gets a failure return TPM_RC_FAILURE to every > subsequent command. So the fix is to kick off self tests once and if > they return TPM_RC_TESTING log that as a backgrounded self test I still think removing all logging traces is a mistake for something that can consume a significant amount of time in the boot sequence. It's going to cause lost of people doing boot timings to waste lots of effort. However, removing the log messages makes the above statement a lie, so one of the two needs fixing. [...] > @@ -852,27 +837,24 @@ static const struct tpm_input_header > tpm2_selftest_header = { > */ > static int tpm2_do_selftest(struct tpm_chip *chip) > { > + struct tpm_buf buf; > + int full; > int rc; > - unsigned int delay_msec = 10; > - long duration; > - struct tpm2_cmd cmd; > > - duration = jiffies_to_msecs( > - tpm2_calc_ordinal_duration(chip, > TPM2_CC_SELF_TEST)); > - > - while (1) { > - cmd.header.in = tpm2_selftest_header; > - cmd.params.selftest_in.full_test = 0; > - > - rc = tpm_transmit_cmd(chip, NULL, &cmd, > TPM2_SELF_TEST_IN_SIZE, > - 0, 0, "continue selftest"); > + for (full = 0; full < 2; full++) { > + rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, > TPM2_CC_SELF_TEST); > + if (rc) > + return rc; > > - if (rc != TPM2_RC_TESTING || delay_msec >= duration) > - break; > + tpm_buf_append_u8(&buf, full); > + rc = tpm_transmit_cmd(chip, NULL, buf.data, > PAGE_SIZE, 0, 0, > + "attempting the self test\n"); There shouldn't be a \n in the string: the failure message already appends one. James -- 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 Sat, Mar 03, 2018 at 12:11:59PM -0800, James Bottomley wrote: > I still think removing all logging traces is a mistake for something > that can consume a significant amount of time in the boot sequence. > It's going to cause lost of people doing boot timings to waste lots of > effort. > > However, removing the log messages makes the above statement a lie, so > one of the two needs fixing. The commit itself makes sense but the implementation was sloppy to say the least in v3. It was like prototype/PoC version of something that could be merged to mainline, not something that can be merged to mainline. For example: * You could have implemented it more cleanly without that new 'tpm_transmit_check' helper function as can be seen. * Many log messages contained a redundant "TPM:" prefix. * There duplicate logs to tpm_transmit_cmd(). Pass a NULL as desc tpm_transmit_cmd() if you want to take care of logging yourself. * The commit has the same short summary as the commit it fixes. This issue still persists. Open for suggestions. Please state if v3 contains a log message that has been removed and is still mandatory and I can add it if it makes sense. > > + rc = tpm_transmit_cmd(chip, NULL, buf.data, > > PAGE_SIZE, 0, 0, > > + "attempting the self test\n"); > > There shouldn't be a \n in the string: the failure message already > appends one. Oops, my bad, will fix it. /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 9e80a953d693..1adb976a2e37 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -537,14 +537,26 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space, const char *desc) { const struct tpm_output_header *header = buf; + unsigned int delay_msec = TPM2_DURATION_SHORT; int err; ssize_t len; - len = tpm_transmit(chip, space, (u8 *)buf, bufsiz, flags); - if (len < 0) - return len; + for (;;) { + len = tpm_transmit(chip, space, (u8 *)buf, bufsiz, flags); + if (len < 0) + return len; + err = be32_to_cpu(header->return_code); + if (err != TPM2_RC_TESTING) + break; + + delay_msec *= 2; + if (delay_msec > TPM2_DURATION_LONG) { + dev_err(&chip->dev, "the self test is still running\n"); + break; + } + tpm_msleep(delay_msec); + } - err = be32_to_cpu(header->return_code); if (err != 0 && desc) dev_err(&chip->dev, "A TPM error (%d) occurred %s\n", err, desc); diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index f895fba4e20d..cccd5994a0e1 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -104,6 +104,7 @@ enum tpm2_return_codes { TPM2_RC_HASH = 0x0083, /* RC_FMT1 */ TPM2_RC_HANDLE = 0x008B, TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */ + TPM2_RC_FAILURE = 0x0101, TPM2_RC_DISABLED = 0x0120, TPM2_RC_COMMAND_CODE = 0x0143, TPM2_RC_TESTING = 0x090A, /* RC_WARN */ diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index a700f8f9ead7..6eeff3a60003 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -31,10 +31,6 @@ struct tpm2_startup_in { __be16 startup_type; } __packed; -struct tpm2_self_test_in { - u8 full_test; -} __packed; - struct tpm2_get_tpm_pt_in { __be32 cap_id; __be32 property_id; @@ -60,7 +56,6 @@ struct tpm2_get_random_out { union tpm2_cmd_params { struct tpm2_startup_in startup_in; - struct tpm2_self_test_in selftest_in; struct tpm2_get_tpm_pt_in get_tpm_pt_in; struct tpm2_get_tpm_pt_out get_tpm_pt_out; struct tpm2_get_random_in getrandom_in; @@ -827,16 +822,6 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal) } EXPORT_SYMBOL_GPL(tpm2_calc_ordinal_duration); -#define TPM2_SELF_TEST_IN_SIZE \ - (sizeof(struct tpm_input_header) + \ - sizeof(struct tpm2_self_test_in)) - -static const struct tpm_input_header tpm2_selftest_header = { - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), - .length = cpu_to_be32(TPM2_SELF_TEST_IN_SIZE), - .ordinal = cpu_to_be32(TPM2_CC_SELF_TEST) -}; - /** * tpm2_do_selftest() - ensure that all self tests have passed * @@ -852,27 +837,24 @@ static const struct tpm_input_header tpm2_selftest_header = { */ static int tpm2_do_selftest(struct tpm_chip *chip) { + struct tpm_buf buf; + int full; int rc; - unsigned int delay_msec = 10; - long duration; - struct tpm2_cmd cmd; - duration = jiffies_to_msecs( - tpm2_calc_ordinal_duration(chip, TPM2_CC_SELF_TEST)); - - while (1) { - cmd.header.in = tpm2_selftest_header; - cmd.params.selftest_in.full_test = 0; - - rc = tpm_transmit_cmd(chip, NULL, &cmd, TPM2_SELF_TEST_IN_SIZE, - 0, 0, "continue selftest"); + for (full = 0; full < 2; full++) { + rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_SELF_TEST); + if (rc) + return rc; - if (rc != TPM2_RC_TESTING || delay_msec >= duration) - break; + tpm_buf_append_u8(&buf, full); + rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0, + "attempting the self test\n"); + tpm_buf_destroy(&buf); - /* wait longer than before */ - delay_msec *= 2; - tpm_msleep(delay_msec); + if (rc == TPM2_RC_TESTING) + rc = TPM2_RC_SUCCESS; + if (rc == TPM2_RC_INITIALIZE || rc == TPM2_RC_SUCCESS) + return rc; } return rc; @@ -1058,10 +1040,8 @@ int tpm2_auto_startup(struct tpm_chip *chip) goto out; rc = tpm2_do_selftest(chip); - if (rc != 0 && rc != TPM2_RC_INITIALIZE) { - dev_err(&chip->dev, "TPM self test failed\n"); + if (rc && rc != TPM2_RC_INITIALIZE) goto out; - } if (rc == TPM2_RC_INITIALIZE) { rc = tpm_startup(chip); @@ -1069,10 +1049,8 @@ int tpm2_auto_startup(struct tpm_chip *chip) goto out; rc = tpm2_do_selftest(chip); - if (rc) { - dev_err(&chip->dev, "TPM self test failed\n"); + if (rc) goto out; - } } rc = tpm2_get_pcr_allocation(chip);