Re: [PATCH ima-evm-utils] add support for reading per bank TPM 2.0 PCRs via sysfs

[Stefan Berger <stefanb@linux.ibm.com>]

On 9/2/22 19:08, Tergel Myanganbayar wrote:
> Until Linux kernel version 5.11, a TSS was required to read TPM 2.0 PCR
> values. A feature which exposed the per bank TPM 2.0 PCRs directly via
> sysfs was upstreamed in newer Kernel versions.

>> Use this recent feature in IMA-EVM-UTILS to remove TSS dependency.
> 
> Signed-off-by: Tergel Myanganbayar <tergel@linux.ibm.com>
> ---
>   src/evmctl.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 46a34cc..d5fe988 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -1899,6 +1899,7 @@ static int read_one_bank(struct tpm_bank_info *tpm_bank, FILE *fp)
>   
>   static char *pcrs = "/sys/class/tpm/tpm0/device/pcrs";  /* Kernels >= 4.0 */
>   static char *misc_pcrs = "/sys/class/misc/tpm0/device/pcrs";
> +static char tpm2_pcr_path[28] = "/sys/class/tpm/tpm0/pcr-sha";

I don't think this 'constant' is necessary since it's only used once 
though the same may be true for the other two. If you want to keep it it 
should probably be written like this

static const char *tpm2_pcr_path = "/sys/class/tpm/tpm0/pcr-sha";

The others should also have a 'const'.
>   
>   /* Read one of the TPM 1.2 sysfs files if present */
>   static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
> @@ -1922,7 +1923,55 @@ static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
>   	for (i = 1; i < num_banks; i++)
>   		tpm_banks[i].supported = 0;
>   	return 0;
> +}
> +
> +static int read_tpm2_one_bank(struct tpm_bank_info *tpm_bank, int bank)
> +{
> +	FILE *fp;
> +	char file_name[NAME_MAX];
> +	char digest[MAX_DIGEST_SIZE + 1];
> +	char *p;
> +	int i;
> +
> +	for (i = 0; i < NUM_PCRS; i++) {
> +		sprintf(file_name, "%s%d/%d", tpm2_pcr_path, bank, i);

... and just write "/sys/class/tpm/tpm0/pcr-sha%d/%d" here ?

> +		fp = fopen(file_name, "r");
> +		if (!fp)
> +			return -1;
> +
> +		p = fgets(digest, (tpm_bank->digest_size * 2 + 1), fp);

no need for parenthesis

> +		if (!p)
> +			return -1;

fclose(fp) before the return

> +
> +		hex2bin(tpm_bank->pcr[i], digest, tpm_bank->digest_size);
> +		fclose(fp);
> +	}
> +	return 0;
> +}
> +
> +static int read_sysfs_tpm2_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
> +{
> +	int banks[2] = {1, 256};
> +	int rt, j;
> +	int tpm_enabled = 0;
>   
> +	if (imaevm_params.verbose > LOG_INFO)
> +		log_info("Trying to read TPM 2.0 PCRs via sysfs.\n");
> +
> +	for (j = 0; j < num_banks; j++) {
> +		rt = read_tpm2_one_bank(&tpm_banks[j], banks[j]);
> +		tpm_banks[j].supported = 0;
> +
> +		if (rt < 0)
> +			continue;
> +
> +		tpm_enabled = 1;
> +		tpm_banks[j].supported = 1;
> +	}
> +
> +	if (tpm_enabled == 0)
> +		return -1;
> +	return 0;


You could have just copied and pasted from read_tpm_banks():

return tpm_enabled ? 0 : 1;
>   }
>   
>   /* Read PCRs from per-bank file(s) specified via --pcrs */
> @@ -2008,6 +2057,9 @@ static int read_tpm_banks(int num_banks, struct tpm_bank_info *bank)
>   	if (read_sysfs_pcrs(num_banks, bank) == 0)
>   		return 0;
>   
> +	if (read_sysfs_tpm2_pcrs(num_banks, bank) == 0)
> +		return 0;
> +
>   	/* Any userspace applications available for reading TPM 2.0 PCRs? */
>   	if (!tpm2_pcr_supported()) {
>   		log_debug("Failed to read TPM 2.0 PCRs\n");
> @@ -2024,6 +2076,7 @@ static int read_tpm_banks(int num_banks, struct tpm_bank_info *bank)
>   					    bank[i].pcr[pcr_handle],
>   					    bank[i].digest_size,
>   					    &errmsg);
> +			bank[i].supported = 1;

Is this a bugfix?

>   			if (err) {
>   				log_debug("Failed to read %s PCRs: (%s)\n",
>   					  bank[i].algo_name, errmsg);