Re: [PATCH v2 3/5] ima-evm-utils: Change tpm2_pcr_read() to use C code

[Mimi Zohar <zohar@linux.ibm.com>]

Hi Ken,

On Mon, 2020-10-12 at 19:44 -0400, Ken Goldman wrote:
> Replace the call out to the command line tools with C functions.
> 
> The algorithm_string_to_algid() function supports only the digest
> algorithms in use.  The table has place holders for other agorithms as
> they are needed and the C strings are defined.
> 
> The table can also be used for an algrithm ID to string function if
> it's ever needed.
> 
> When using the IBM TSS, link in its library.
> 
> Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>

The code seems to be working properly, but needs to be cleaned up. 
There are simple formatting changes and some style changes.

Simple formatting changes:
- Tabs should be 8 characters.
- Remove blank spaces before code.
- 80 characater maximum line length.
- Leave a blank line between variable definitions and code.
- Long comments should start with "/*" and end with "*/" on separate
lines:
/*
 * multiple line comments
 * continued
 */
- When defining function variables, please use the format:
term:
<definition>


Style changes:
- There are valid reasons for having a common function exit (e.g.
freeing memory), otherwise the function should exit early.  The "if (rc
== 0)" tests cause the code to unnecessarily be indented.  Instead the
test could be inverted "if (!rc)" followed by a "goto out".

- Please do not use camel or Hungarian variable naming conventions.  
The variable definition can reference the spec name.

thanks,

Mimi

> ---
>  src/Makefile.am      |   1 +
>  src/pcr_tsspcrread.c | 156 +++++++++++++++++++++++++++++++++----------
>  2 files changed, 123 insertions(+), 34 deletions(-)
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index d6c779f..bf18caf 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -26,6 +26,7 @@ if USE_PCRTSS
>  evmctl_SOURCES += pcr_tss.c
>  else
>  evmctl_SOURCES += pcr_tsspcrread.c
> +evmctl_LDADD += -libmtss
>  endif
>  
>  AM_CPPFLAGS = -I$(top_srcdir) -include config.h
> diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
> index 118c7d2..eae68b7 100644
> --- a/src/pcr_tsspcrread.c
> +++ b/src/pcr_tsspcrread.c
> @@ -50,6 +50,10 @@
>  #include "utils.h"
>  #include "imaevm.h"
>  
> +#define TPM_POSIX	/* use Posix, not Windows constructs in TSS */
> +#undef MAX_DIGEST_SIZE	/* imaevm uses a different value than the TSS */
> +#include <ibmtss/tss.h>
> +
>  #define CMD "tsspcrread"
>  
>  static char path[PATH_MAX];
> @@ -68,44 +72,128 @@ int tpm2_pcr_supported(void)
>  	return 1;
>  }
>  
> -int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
> -		 int len, char **errmsg)
> -{
> -	FILE *fp;
> -	char pcr[100];	/* may contain an error */
> -	char cmd[PATH_MAX + 50];
> -	int ret;
> -
> -	sprintf(cmd, "%s -halg %s -ha %d -ns 2> /dev/null",
> -		path, algo_name, pcrHandle);
> -	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;
> -	}
> +/* Table mapping C strings to TCG algorithm identifiers */
> +
> +typedef struct tdAlgorithm_Map {
> +    const char *algorithm_string;
> +    TPMI_ALG_HASH algid;
> +} Algorithm_Map;
>  
> -	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;
> +Algorithm_Map algorithm_map[] = {
> +				 { "sha1", TPM_ALG_SHA1},
> +				 { "sha256", TPM_ALG_SHA256},
> +#if 0	/* uncomment as these digest algorithms are supported */
> +				 { "", TPM_ALG_SHA384},
> +				 { "", TPM_ALG_SHA512},
> +				 { "", TPM_ALG_SM3_256},
> +				 { "", TPM_ALG_SHA3_256},
> +				 { "", TPM_ALG_SHA3_384},
> +				 { "", TPM_ALG_SHA3_512},
> +#endif
> +};
> +
> +/* algorithm_string_to_algid() converts a digest algorithm from a C string to a TCG algorithm
> +   identifier as defined in the TCG Algorithm Regisrty..
> +
> +   Returns TPM_ALG_ERROR if the string has an unsupported value.
> +*/
> +
> +static TPMI_ALG_HASH algorithm_string_to_algid(const char *algorithm_string)
> +{
> +    size_t 	i;
> +    for (i=0 ; i < sizeof(algorithm_map)/sizeof(Algorithm_Map) ; i++) {
> +	if (strcmp(algorithm_string, algorithm_map[i].algorithm_string) == 0) {
> +	    return algorithm_map[i].algid; /* if match */
>  	}
> +    }
> +    return TPM_ALG_ERROR;
> +}
>  
> -	/* get the popen "cmd" return code */
> -	ret = pclose(fp);
> +/* tpm2_pcr_read() reads the PCR
>  
> -	/* Treat an unallocated bank as an error */
> -	if (!ret && (strlen(pcr) < SHA_DIGEST_LENGTH))
> -		ret = -1;
> +   algo_name is the PCR digest algorithm (the PCR bank) as a C string
> +   pcrHandle is the PCR number to read
> +   hwpcr is a buffer for the PCR output in binary
> +   len is the allocated size of hwpcr and should match the digest algorithm
> +*/
>  
> -	if (!ret)
> -		hex2bin(hwpcr, pcr, len);
> -	else
> -		*errmsg = strndup(pcr, strlen(pcr) - 1); /* remove newline */
> +int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
> +		 int len, char **errmsg)
> +{
> +        int 			ret = 0;	/* function return code */
> +	TPM_RC			rc = 0;		/* TCG return code */
> +	PCR_Read_In 		pcrReadIn;	/* command input */
> +	PCR_Read_Out 		pcrReadOut;	/* response output */
> +	TSS_CONTEXT		*tssContext = NULL;
> +	TPMI_ALG_HASH 		alg_id;		/* PCR algorithm */
>  
> -	return ret;
> +	if (rc == 0) {		/* map algorithm string to TCG value */
> +	    alg_id = algorithm_string_to_algid(algo_name);
> +	    if (alg_id == TPM_ALG_ERROR) {
> +		ret = asprintf(errmsg, "tpm2_pcr_read: unknown algorithm %s", algo_name);
> +		if (ret == -1) {	/* the contents of errmsg is undefined */
> +		    *errmsg = NULL;
> +		}
> +		rc = 1;
> +	    }
> +	}
> +	if (rc == 0) {
> +	    rc = TSS_Create(&tssContext);
> +	}
> +	/* call TSS to execute the command */
> +	if (rc == 0) {
> +	    pcrReadIn.pcrSelectionIn.count = 1;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].hash = alg_id;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].sizeofSelect = 3;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[0] = 0;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[1] = 0;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[2] = 0;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[pcrHandle / 8] =
> +		1 << (pcrHandle % 8);
> +	    rc = TSS_Execute(tssContext,
> +			     (RESPONSE_PARAMETERS *)&pcrReadOut,
> +			     (COMMAND_PARAMETERS *)&pcrReadIn,
> +			     NULL,
> +			     TPM_CC_PCR_Read,
> +			     TPM_RH_NULL, NULL, 0);
> +	}
> +	if (rc == 0) {
> +	    /* nothing read, bank missing */
> +	    if (pcrReadOut.pcrValues.count == 0) {
> +		ret = asprintf(errmsg, "tpm2_pcr_read: returned count 0 for %s", algo_name);
> +		if (ret == -1) {	/* the contents of errmsg is undefined */
> +		    *errmsg = NULL;
> +		}
> +		rc = 1;
> +	    }
> +	    /* len parameter did not match the digest algorithm */
> +	    else if (pcrReadOut.pcrValues.digests[0].t.size != len) {
> +		ret = asprintf(errmsg,
> +			       "tpm2_pcr_read: expected length %d actual %u for %s",
> +			       len, pcrReadOut.pcrValues.digests[0].t.size, algo_name);
> +		if (ret == -1) {	/* the contents of errmsg is undefined */
> +		    *errmsg = NULL;
> +		}
> +		rc = 1;
> +	    }
> +	    else {
> +		memcpy(hwpcr,
> +		       pcrReadOut.pcrValues.digests[0].t.buffer,
> +		       pcrReadOut.pcrValues.digests[0].t.size);
> +	    }
> +	}
> +	{
> +	    TPM_RC rc1 = TSS_Delete(tssContext);
> +	    if (rc == 0) {
> +		rc = rc1;
> +	    }
> +	}
> +	/* map TCG return code to function return code */
> +	if (rc == 0) {
> +	    return 0;
> +	}
> +	else {
> +	    return -1;
> +	}
>  }
> +