ima-evm-utils: use tsspcrread to read the TPM 2.0 PCRs
diff mbox series

Message ID 1563802368-8460-1-git-send-email-zohar@linux.ibm.com
State New
Headers show
Series
  • ima-evm-utils: use tsspcrread to read the TPM 2.0 PCRs
Related show

Commit Message

Mimi Zohar July 22, 2019, 1:32 p.m. UTC
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.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 configure.ac    |  3 +++
 src/Makefile.am |  3 +++
 src/evmctl.c    | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 54 insertions(+), 5 deletions(-)

Comments

Vitaly Chikunov July 22, 2019, 3:58 p.m. UTC | #1
Mimi,

On Mon, Jul 22, 2019 at 09:32:48AM -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.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  configure.ac    |  3 +++
>  src/Makefile.am |  3 +++
>  src/evmctl.c    | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 9e0926f10404..cbb9397be138 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -1402,6 +1400,38 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len)
>  	return result;
>  }
>  
> +#ifdef IBMTSS
> +static int tpm_pcr_read2(int idx, uint8_t *hwpcr, int len, char **errmsg)
> +{
> +	FILE *fp;
> +	char pcr[100];	/* may contain an error */
> +	char cmd[36];
> +	int ret = 0;
> +	int i;
> +
> +	sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns", idx);
> +	fp = popen(cmd, "r");
> +	if (!fp)
> +		return -1;
> +
> +	fgets(pcr, 100, fp);

Should it be sizeof(pcr)?

I don't know convention of `tsspcrread' but maybe fgets() return value
should be checked too in case of error of executing `tsspcrread' or
error inside of `tsspcrread' (like pcr read error).

> +
> +	/* pcr might contain an error message */
> +	for (i = 0; i < strlen(pcr) - 1 && !ret; i++) {
> +		if (isspace(pcr[i]))
> +			ret = -1;

Probably `strlen(pcr)' should be without `- 1'.

> +	}
> +
> +	if (!ret)
> +		hex2bin(hwpcr, pcr, SHA_DIGEST_LENGTH);
> +	else
> +		*errmsg = pcr;

pcr isn't static nor malloc'ed.

> +
> +	pclose(fp);
> +	return ret;
> +}
> +#endif
> +
>  #define TCG_EVENT_NAME_LEN_MAX	255
>  
>  struct template_entry {
> @@ -1658,8 +1688,21 @@ 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)))
> -			exit(1);
> +		if (tpm_pcr_read(i, hwpcr, sizeof(hwpcr))) {
> +#ifdef IBMTSS
> +			char *errmsg = NULL;
> +
> +			err = tpm_pcr_read2(i, hwpcr, sizeof(hwpcr), &errmsg);
> +			if (err) {
> +				log_info("Failed to read either TPM 1.2 or TPM 2.0 PCRs.\n\t %s", 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
Bruno E. O. Meneguele July 22, 2019, 6:52 p.m. UTC | #2
Hi Mirian,

On Mon, Jul 22, 2019 at 06:58:35PM +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Mon, Jul 22, 2019 at 09:32:48AM -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.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  configure.ac    |  3 +++
> >  src/Makefile.am |  3 +++
> >  src/evmctl.c    | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  3 files changed, 54 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 9e0926f10404..cbb9397be138 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -1402,6 +1400,38 @@ static int tpm_pcr_read(int idx, uint8_t *pcr, int len)
> >  	return result;
> >  }
> >  
> > +#ifdef IBMTSS
> > +static int tpm_pcr_read2(int idx, uint8_t *hwpcr, int len, char **errmsg)
> > +{
> > +	FILE *fp;
> > +	char pcr[100];	/* may contain an error */
> > +	char cmd[36];
> > +	int ret = 0;
> > +	int i;
> > +
> > +	sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns", idx);
> > +	fp = popen(cmd, "r");
> > +	if (!fp)
> > +		return -1;
> > +
> > +	fgets(pcr, 100, fp);
> 
> Should it be sizeof(pcr)?
> 
> I don't know convention of `tsspcrread' but maybe fgets() return value
> should be checked too in case of error of executing `tsspcrread' or
> error inside of `tsspcrread' (like pcr read error).
> 
> > +
> > +	/* pcr might contain an error message */
> > +	for (i = 0; i < strlen(pcr) - 1 && !ret; i++) {
> > +		if (isspace(pcr[i]))
> > +			ret = -1;
> 
> Probably `strlen(pcr)' should be without `- 1'.
> 
> > +	}
> > +
> > +	if (!ret)
> > +		hex2bin(hwpcr, pcr, SHA_DIGEST_LENGTH);
> > +	else
> > +		*errmsg = pcr;
> 
> pcr isn't static nor malloc'ed.
> 
> > +
> > +	pclose(fp);
> > +	return ret;
> > +}
> > +#endif
> > +
> >  #define TCG_EVENT_NAME_LEN_MAX	255
> >  
> >  struct template_entry {
> > @@ -1658,8 +1688,21 @@ 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)))
> > -			exit(1);
> > +		if (tpm_pcr_read(i, hwpcr, sizeof(hwpcr))) {
> > +#ifdef IBMTSS
> > +			char *errmsg = NULL;
> > +
> > +			err = tpm_pcr_read2(i, hwpcr, sizeof(hwpcr), &errmsg);
> > +			if (err) {
> > +				log_info("Failed to read either TPM 1.2 or TPM 2.0 PCRs.\n\t %s", 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

Besides to what Vitaly have pointed...

exit(1) has been the standard exit code in case of failure, wouldn't
that be better to keep it instead of change it to -1? (points
highlighted above)
Mimi Zohar July 22, 2019, 7:15 p.m. UTC | #3
Vitaly, Bruno,

On Mon, 2019-07-22 at 15:52 -0300, Bruno E. O. Meneguele wrote:
> On Mon, Jul 22, 2019 at 06:58:35PM +0300, Vitaly Chikunov wrote:
> > Mimi,
> > 
> > On Mon, Jul 22, 2019 at 09:32:48AM -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.
> > > 
> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

Thank you for reviewing this patch so quickly!  Your comments are much
appreciated and will be addressed in the version.

Mimi

Patch
diff mbox series

diff --git a/configure.ac b/configure.ac
index 9beb4b6c2377..40fea93fef2f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -28,6 +28,8 @@  PKG_CHECK_MODULES(LIBCRYPTO, [libcrypto >= 0.9.8 ])
 AC_SUBST(KERNEL_HEADERS)
 AC_CHECK_HEADER(unistd.h)
 AC_CHECK_HEADERS(openssl/conf.h)
+AC_SEARCH_LIBS(TSS_Transmit, ibmtss, [have_ibmtss=yes], [have_ibmtss=no])
+AM_CONDITIONAL([CONFIG_IBMTSS], [test "x$have_ibmtss" = "xyes"])
 
 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 +73,5 @@  echo
 echo	"Configuration:"
 echo	"          debug: $pkg_cv_enable_debug"
 echo	"   openssl-conf: $enable_openssl_conf"
+echo	"         ibmtss: $have_ibmtss"
 echo
diff --git a/src/Makefile.am b/src/Makefile.am
index 9c037e21dc4f..f0990fb01210 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -21,6 +21,9 @@  evmctl_SOURCES = evmctl.c
 evmctl_CPPFLAGS = $(AM_CPPFLAGS) $(LIBCRYPTO_CFLAGS)
 evmctl_LDFLAGS = $(LDFLAGS_READLINE)
 evmctl_LDADD =  $(LIBCRYPTO_LIBS) -lkeyutils libimaevm.la
+if CONFIG_IBMTSS
+evmctl_CFLAGS = -DIBMTSS
+endif
 
 AM_CPPFLAGS = -I$(top_srcdir) -include config.h
 
diff --git a/src/evmctl.c b/src/evmctl.c
index 9e0926f10404..cbb9397be138 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,38 @@  static int tpm_pcr_read(int idx, uint8_t *pcr, int len)
 	return result;
 }
 
+#ifdef IBMTSS
+static int tpm_pcr_read2(int idx, uint8_t *hwpcr, int len, char **errmsg)
+{
+	FILE *fp;
+	char pcr[100];	/* may contain an error */
+	char cmd[36];
+	int ret = 0;
+	int i;
+
+	sprintf(cmd, "tsspcrread -halg sha1 -ha %d -ns", idx);
+	fp = popen(cmd, "r");
+	if (!fp)
+		return -1;
+
+	fgets(pcr, 100, fp);
+
+	/* pcr might contain an error message */
+	for (i = 0; i < strlen(pcr) - 1 && !ret; i++) {
+		if (isspace(pcr[i]))
+			ret = -1;
+	}
+
+	if (!ret)
+		hex2bin(hwpcr, pcr, SHA_DIGEST_LENGTH);
+	else
+		*errmsg = pcr;
+
+	pclose(fp);
+	return ret;
+}
+#endif
+
 #define TCG_EVENT_NAME_LEN_MAX	255
 
 struct template_entry {
@@ -1658,8 +1688,21 @@  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)))
-			exit(1);
+		if (tpm_pcr_read(i, hwpcr, sizeof(hwpcr))) {
+#ifdef IBMTSS
+			char *errmsg = NULL;
+
+			err = tpm_pcr_read2(i, hwpcr, sizeof(hwpcr), &errmsg);
+			if (err) {
+				log_info("Failed to read either TPM 1.2 or TPM 2.0 PCRs.\n\t %s", 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));