Re: [PATCH v3] ima-evm-utils: use tsspcrread to read the TPM 2.0 PCRs

["Bruno E. O. Meneguele" <bmeneg@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5165 bytes --]

On Tue, Jul 23, 2019 at 04:27:06PM -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 v3:
> - Suggested by Vitaly, replace snprintf() & strdup() with asprintf().
> 
> 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 |  7 +++++++
>  src/evmctl.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 9beb4b6c2377..3fc63b367c91 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -7,6 +7,7 @@ AC_CONFIG_HEADERS([config.h])
>  AC_CONFIG_MACRO_DIR([m4])
>  
>  AC_CANONICAL_HOST
> +AC_USE_SYSTEM_EXTENSIONS
>  
>  # Checks for programs.
>  AC_PROG_CC
> @@ -29,6 +30,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 +77,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..3289061fe8d2 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,43 @@ 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) {
> +		ret = asprintf(errmsg, "popen failed: %s", strerror(errno));
> +		if (ret == -1)	/* the contents of errmsg is undefined */
> +			*errmsg = NULL;
> +		return -1;
> +	}
> +
> +	if (fgets(pcr, sizeof(pcr), fp) == NULL) {
> +		ret = asprintf(errmsg, "tsspcrread failed: %s",
> +			       strerror(errno));
> +		if (ret == -1)	/* the contents of errmsg is undefined */
> +			*errmsg = NULL;
> +		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 +1693,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
> 

Looks good to me.

thanks Mimi!

Reviewed-by: Bruno E. O. Meneguele <bmeneg@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]