Re: [PATCH] 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: 3225 bytes --]

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)

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