| Message ID | 1563893743-4586-1-git-send-email-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ima-evm-utils: use tsspcrread to read the TPM 2.0 PCRs | expand |
Hi Mirian, On Tue, Jul 23, 2019 at 10:55:43AM -0400, Mimi Zohar wrote: > The kernel does not expose the crypto agile TPM 2.0 PCR banks to > userspace like it exposes PCRs for TPM 1.2. As a result, a userspace > application is required to read PCRs. > > This patch adds tsspcrread support for reading the TPM 2.0 PCRs. > tsspcrread is one application included in the ibmtss package. > > Sample error messages: > Failed to read PCRs: (tsspcrread failed: No such file or directory) > Failed to read PCRs: (TSS_Dev_Open: Error opening /dev/tpmrm0) > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > Changelog v2: > - Fix "errmsg" not always being initialized within tpm_pcr_read2(). > Reported by Bruno. > - Shortened error msg length. > - Renamed tpm_pcr_read2() to tpm2_pcr_read(). > - Included Petr Vorel's autotool change, which looks for tsspcrread, > rather than the tss library. > > Changelog v1: > - Based on Vitaly's review, test the popen "cmd" return code. Use > pclose to determine if the result of the popen command succeeded or > failed. Removed the now unneeded checking for spaces. > - Dynamically allocated the error msg and other changes based on > Vitaly's review. > - Based on Bruno's review, reverted the return code change. At some > point, we'll need to re-visit the return codes in general. > > configure.ac | 6 ++++++ > src/evmctl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 57 insertions(+), 4 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 9beb4b6c2377..09b111c4ca4a 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -29,6 +29,11 @@ AC_SUBST(KERNEL_HEADERS) > AC_CHECK_HEADER(unistd.h) > AC_CHECK_HEADERS(openssl/conf.h) > > +AC_CHECK_PROG(TSSPCRREAD, [tsspcrread], yes, no) > +if test "x$TSSPCRREAD" = "xyes"; then > + AC_DEFINE(HAVE_TSSPCRREAD, 1, [Define to 1 if you have tsspcrread binary installed])], > +fi > + > AC_CHECK_HEADERS(sys/xattr.h, , [AC_MSG_ERROR([sys/xattr.h header not found. You need the c-library development package.])]) > AC_CHECK_HEADERS(keyutils.h, , [AC_MSG_ERROR([keyutils.h header not found. You need the libkeyutils development package.])]) > > @@ -71,4 +76,5 @@ echo > echo "Configuration:" > echo " debug: $pkg_cv_enable_debug" > echo " openssl-conf: $enable_openssl_conf" > +echo " tsspcrread: $TSSPCRREAD" > echo > diff --git a/src/evmctl.c b/src/evmctl.c > index 9e0926f10404..9a2bfad928d1 100644 > --- a/src/evmctl.c > +++ b/src/evmctl.c > @@ -1383,10 +1383,8 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) > if (!fp) > fp = fopen(misc_pcrs, "r"); > > - if (!fp) { > - log_err("Unable to open %s or %s\n", pcrs, misc_pcrs); > + if (!fp) > return -1; > - } > > for (;;) { > p = fgets(buf, sizeof(buf), fp); > @@ -1402,6 +1400,41 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) > return result; > } > > +#ifdef HAVE_TSSPCRREAD > +static int tpm2_pcr_read(int idx, uint8_t *hwpcr, int len, char **errmsg) > +{ > + FILE *fp; > + char pcr[100]; /* may contain an error */ > + char cmd[50]; > + int ret; > + > + sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns 2> /dev/null", idx); > + fp = popen(cmd, "r"); > + if (!fp) { > + snprintf(pcr, sizeof(pcr), "popen failed: %s", strerror(errno)); > + *errmsg = strdup("popen failed:"); Should it have been *errmsg = strdup(pcr); ? > + return -1; > + } > + > + if (fgets(pcr, sizeof(pcr), fp) == NULL) { > + snprintf(pcr, sizeof(pcr), "tsspcrread failed: %s", > + strerror(errno)); > + *errmsg = strdup(pcr); > + ret = pclose(fp); > + return -1; > + } > + > + /* get the popen "cmd" return code */ > + ret = pclose(fp); > + if (!ret) > + hex2bin(hwpcr, pcr, SHA_DIGEST_LENGTH); > + else > + *errmsg = strndup(pcr, strlen(pcr) - 1); /* remove newline */ > + > + return ret; > +} > +#endif > + > #define TCG_EVENT_NAME_LEN_MAX 255 > > struct template_entry { > @@ -1658,8 +1691,22 @@ static int ima_measurement(const char *file) > log_info("PCRAgg %.2d: ", i); > log_dump(pcr[i], SHA_DIGEST_LENGTH); > > - if (tpm_pcr_read(i, hwpcr, sizeof(hwpcr))) > + if (tpm_pcr_read(i, hwpcr, sizeof(hwpcr))) { > +#ifdef HAVE_TSSPCRREAD > + char *errmsg = NULL; > + > + err = tpm2_pcr_read(i, hwpcr, sizeof(hwpcr), &errmsg); > + if (err) { > + log_info("Failed to read PCRs: (%s)\n", errmsg); > + free(errmsg); > + exit(1); > + } > +#else > + log_info("Failed to read TPM 1.2 PCRs.\n"); > exit(1); > +#endif > + } > + > log_info("HW PCR-%d: ", i); > log_dump(hwpcr, sizeof(hwpcr)); > > -- > 2.7.5 >
On Tue, 2019-07-23 at 12:47 -0300, Bruno E. O. Meneguele wrote: > > @@ -1402,6 +1400,41 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) > > return result; > > } > > > > +#ifdef HAVE_TSSPCRREAD > > +static int tpm2_pcr_read(int idx, uint8_t *hwpcr, int len, char **errmsg) > > +{ > > + FILE *fp; > > + char pcr[100]; /* may contain an error */ > > + char cmd[50]; > > + int ret; > > + > > + sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns 2> /dev/null", idx); > > + fp = popen(cmd, "r"); > > + if (!fp) { > > + snprintf(pcr, sizeof(pcr), "popen failed: %s", strerror(errno)); > > + *errmsg = strdup("popen failed:"); > > Should it have been > > *errmsg = strdup(pcr); > Yes, of course. thanks! Mimi
Mimi, On Tue, Jul 23, 2019 at 11:53:10AM -0400, Mimi Zohar wrote: > On Tue, 2019-07-23 at 12:47 -0300, Bruno E. O. Meneguele wrote: > > > > @@ -1402,6 +1400,41 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) > > > return result; > > > } > > > > > > +#ifdef HAVE_TSSPCRREAD > > > +static int tpm2_pcr_read(int idx, uint8_t *hwpcr, int len, char **errmsg) > > > +{ > > > + FILE *fp; > > > + char pcr[100]; /* may contain an error */ > > > + char cmd[50]; > > > + int ret; > > > + > > > + sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns 2> /dev/null", idx); > > > + fp = popen(cmd, "r"); > > > + if (!fp) { > > > + snprintf(pcr, sizeof(pcr), "popen failed: %s", strerror(errno)); > > > + *errmsg = strdup("popen failed:"); > > > > Should it have been > > > > *errmsg = strdup(pcr); > > > Yes, of course. Or better to use asprintf(3). Thanks,
On Tue, 2019-07-23 at 19:41 +0300, Vitaly Chikunov wrote: > Mimi, > > On Tue, Jul 23, 2019 at 11:53:10AM -0400, Mimi Zohar wrote: > > On Tue, 2019-07-23 at 12:47 -0300, Bruno E. O. Meneguele wrote: > > > > > > @@ -1402,6 +1400,41 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) > > > > return result; > > > > } > > > > > > > > +#ifdef HAVE_TSSPCRREAD > > > > +static int tpm2_pcr_read(int idx, uint8_t *hwpcr, int len, char **errmsg) > > > > +{ > > > > + FILE *fp; > > > > + char pcr[100]; /* may contain an error */ > > > > + char cmd[50]; > > > > + int ret; > > > > + > > > > + sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns 2> /dev/null", idx); > > > > + fp = popen(cmd, "r"); > > > > + if (!fp) { > > > > + snprintf(pcr, sizeof(pcr), "popen failed: %s", strerror(errno)); > > > > + *errmsg = strdup("popen failed:"); > > > > > > Should it have been > > > > > > *errmsg = strdup(pcr); > > > > > Yes, of course. > > Or better to use asprintf(3). That's even better, assuming that we want to include AC_USE_SYSTEM_EXTENSIONS in configure.ac? Did you want to make this change as a separate patch, or should I fold it into this one? Mimi
Mimi, On Tue, Jul 23, 2019 at 02:24:53PM -0400, Mimi Zohar wrote: > On Tue, 2019-07-23 at 19:41 +0300, Vitaly Chikunov wrote: > > On Tue, Jul 23, 2019 at 11:53:10AM -0400, Mimi Zohar wrote: > > > On Tue, 2019-07-23 at 12:47 -0300, Bruno E. O. Meneguele wrote: > > > > > > > > @@ -1402,6 +1400,41 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) > > > > > return result; > > > > > } > > > > > > > > > > +#ifdef HAVE_TSSPCRREAD > > > > > +static int tpm2_pcr_read(int idx, uint8_t *hwpcr, int len, char **errmsg) > > > > > +{ > > > > > + FILE *fp; > > > > > + char pcr[100]; /* may contain an error */ > > > > > + char cmd[50]; > > > > > + int ret; > > > > > + > > > > > + sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns 2> /dev/null", idx); > > > > > + fp = popen(cmd, "r"); > > > > > + if (!fp) { > > > > > + snprintf(pcr, sizeof(pcr), "popen failed: %s", strerror(errno)); > > > > > + *errmsg = strdup("popen failed:"); > > > > > > > > Should it have been > > > > > > > > *errmsg = strdup(pcr); > > > > > > > Yes, of course. > > > > Or better to use asprintf(3). > > That's even better, assuming that we want to include > AC_USE_SYSTEM_EXTENSIONS in configure.ac? Yes. > Did you want to make this change as a separate patch, or should I fold > it into this one? Probably you, since you are first to add snprintf+strdup. Thanks,
On Tue, 2019-07-23 at 22:31 +0300, Vitaly Chikunov wrote: > Mimi, > > On Tue, Jul 23, 2019 at 02:24:53PM -0400, Mimi Zohar wrote: > > On Tue, 2019-07-23 at 19:41 +0300, Vitaly Chikunov wrote: > > > On Tue, Jul 23, 2019 at 11:53:10AM -0400, Mimi Zohar wrote: > > > > On Tue, 2019-07-23 at 12:47 -0300, Bruno E. O. Meneguele wrote: > > > > > > > > > > @@ -1402,6 +1400,41 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) > > > > > > return result; > > > > > > } > > > > > > > > > > > > +#ifdef HAVE_TSSPCRREAD > > > > > > +static int tpm2_pcr_read(int idx, uint8_t *hwpcr, int len, char **errmsg) > > > > > > +{ > > > > > > + FILE *fp; > > > > > > + char pcr[100]; /* may contain an error */ > > > > > > + char cmd[50]; > > > > > > + int ret; > > > > > > + > > > > > > + sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns 2> /dev/null", idx); > > > > > > + fp = popen(cmd, "r"); > > > > > > + if (!fp) { > > > > > > + snprintf(pcr, sizeof(pcr), "popen failed: %s", strerror(errno)); > > > > > > + *errmsg = strdup("popen failed:"); > > > > > > > > > > Should it have been > > > > > > > > > > *errmsg = strdup(pcr); > > > > > > > > > Yes, of course. > > > > > > Or better to use asprintf(3). > > > > That's even better, assuming that we want to include > > AC_USE_SYSTEM_EXTENSIONS in configure.ac? > > Yes. > > > Did you want to make this change as a separate patch, or should I fold > > it into this one? > > Probably you, since you are first to add snprintf+strdup. I didn't mean instead of this patch, but in addition to, on top of this patch with the "strdup(pcr)" correction. Mimi
Mimi, On Tue, Jul 23, 2019 at 03:37:28PM -0400, Mimi Zohar wrote: > On Tue, 2019-07-23 at 22:31 +0300, Vitaly Chikunov wrote: > > On Tue, Jul 23, 2019 at 02:24:53PM -0400, Mimi Zohar wrote: > > > On Tue, 2019-07-23 at 19:41 +0300, Vitaly Chikunov wrote: > > > > On Tue, Jul 23, 2019 at 11:53:10AM -0400, Mimi Zohar wrote: > > > > > On Tue, 2019-07-23 at 12:47 -0300, Bruno E. O. Meneguele wrote: > > > > > > > > > > > > @@ -1402,6 +1400,41 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) > > > > > > > return result; > > > > > > > } > > > > > > > > > > > > > > +#ifdef HAVE_TSSPCRREAD > > > > > > > +static int tpm2_pcr_read(int idx, uint8_t *hwpcr, int len, char **errmsg) > > > > > > > +{ > > > > > > > + FILE *fp; > > > > > > > + char pcr[100]; /* may contain an error */ > > > > > > > + char cmd[50]; > > > > > > > + int ret; > > > > > > > + > > > > > > > + sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns 2> /dev/null", idx); > > > > > > > + fp = popen(cmd, "r"); > > > > > > > + if (!fp) { > > > > > > > + snprintf(pcr, sizeof(pcr), "popen failed: %s", strerror(errno)); > > > > > > > + *errmsg = strdup("popen failed:"); > > > > > > > > > > > > Should it have been > > > > > > > > > > > > *errmsg = strdup(pcr); > > > > > > > > > > > Yes, of course. > > > > > > > > Or better to use asprintf(3). > > > > > > That's even better, assuming that we want to include > > > AC_USE_SYSTEM_EXTENSIONS in configure.ac? > > > > Yes. > > > > > Did you want to make this change as a separate patch, or should I fold > > > it into this one? > > > > Probably you, since you are first to add snprintf+strdup. > > I didn't mean instead of this patch, but in addition to, on top of > this patch with the "strdup(pcr)" correction. I thought you will post new version anyway with `strdup(pcr)` fix so why not add `asprintf` in the same time. If you don't want I can post change after release. Currently, I try to make `make check` tests. (Don't want you to wait for them before release too.) Thanks,
diff --git a/configure.ac b/configure.ac index 9beb4b6c2377..09b111c4ca4a 100644 --- a/configure.ac +++ b/configure.ac @@ -29,6 +29,11 @@ AC_SUBST(KERNEL_HEADERS) AC_CHECK_HEADER(unistd.h) AC_CHECK_HEADERS(openssl/conf.h) +AC_CHECK_PROG(TSSPCRREAD, [tsspcrread], yes, no) +if test "x$TSSPCRREAD" = "xyes"; then + AC_DEFINE(HAVE_TSSPCRREAD, 1, [Define to 1 if you have tsspcrread binary installed])], +fi + AC_CHECK_HEADERS(sys/xattr.h, , [AC_MSG_ERROR([sys/xattr.h header not found. You need the c-library development package.])]) AC_CHECK_HEADERS(keyutils.h, , [AC_MSG_ERROR([keyutils.h header not found. You need the libkeyutils development package.])]) @@ -71,4 +76,5 @@ echo echo "Configuration:" echo " debug: $pkg_cv_enable_debug" echo " openssl-conf: $enable_openssl_conf" +echo " tsspcrread: $TSSPCRREAD" echo diff --git a/src/evmctl.c b/src/evmctl.c index 9e0926f10404..9a2bfad928d1 100644 --- a/src/evmctl.c +++ b/src/evmctl.c @@ -1383,10 +1383,8 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) if (!fp) fp = fopen(misc_pcrs, "r"); - if (!fp) { - log_err("Unable to open %s or %s\n", pcrs, misc_pcrs); + if (!fp) return -1; - } for (;;) { p = fgets(buf, sizeof(buf), fp); @@ -1402,6 +1400,41 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len) return result; } +#ifdef HAVE_TSSPCRREAD +static int tpm2_pcr_read(int idx, uint8_t *hwpcr, int len, char **errmsg) +{ + FILE *fp; + char pcr[100]; /* may contain an error */ + char cmd[50]; + int ret; + + sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns 2> /dev/null", idx); + fp = popen(cmd, "r"); + if (!fp) { + snprintf(pcr, sizeof(pcr), "popen failed: %s", strerror(errno)); + *errmsg = strdup("popen failed:"); + return -1; + } + + if (fgets(pcr, sizeof(pcr), fp) == NULL) { + snprintf(pcr, sizeof(pcr), "tsspcrread failed: %s", + strerror(errno)); + *errmsg = strdup(pcr); + ret = pclose(fp); + return -1; + } + + /* get the popen "cmd" return code */ + ret = pclose(fp); + if (!ret) + hex2bin(hwpcr, pcr, SHA_DIGEST_LENGTH); + else + *errmsg = strndup(pcr, strlen(pcr) - 1); /* remove newline */ + + return ret; +} +#endif + #define TCG_EVENT_NAME_LEN_MAX 255 struct template_entry { @@ -1658,8 +1691,22 @@ static int ima_measurement(const char *file) log_info("PCRAgg %.2d: ", i); log_dump(pcr[i], SHA_DIGEST_LENGTH); - if (tpm_pcr_read(i, hwpcr, sizeof(hwpcr))) + if (tpm_pcr_read(i, hwpcr, sizeof(hwpcr))) { +#ifdef HAVE_TSSPCRREAD + char *errmsg = NULL; + + err = tpm2_pcr_read(i, hwpcr, sizeof(hwpcr), &errmsg); + if (err) { + log_info("Failed to read PCRs: (%s)\n", errmsg); + free(errmsg); + exit(1); + } +#else + log_info("Failed to read TPM 1.2 PCRs.\n"); exit(1); +#endif + } + log_info("HW PCR-%d: ", i); log_dump(hwpcr, sizeof(hwpcr));
The kernel does not expose the crypto agile TPM 2.0 PCR banks to userspace like it exposes PCRs for TPM 1.2. As a result, a userspace application is required to read PCRs. This patch adds tsspcrread support for reading the TPM 2.0 PCRs. tsspcrread is one application included in the ibmtss package. Sample error messages: Failed to read PCRs: (tsspcrread failed: No such file or directory) Failed to read PCRs: (TSS_Dev_Open: Error opening /dev/tpmrm0) Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- Changelog v2: - Fix "errmsg" not always being initialized within tpm_pcr_read2(). Reported by Bruno. - Shortened error msg length. - Renamed tpm_pcr_read2() to tpm2_pcr_read(). - Included Petr Vorel's autotool change, which looks for tsspcrread, rather than the tss library. Changelog v1: - Based on Vitaly's review, test the popen "cmd" return code. Use pclose to determine if the result of the popen command succeeded or failed. Removed the now unneeded checking for spaces. - Dynamically allocated the error msg and other changes based on Vitaly's review. - Based on Bruno's review, reverted the return code change. At some point, we'll need to re-visit the return codes in general. configure.ac | 6 ++++++ src/evmctl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 57 insertions(+), 4 deletions(-)