Re: [PATCH ima-evm-utils] Check for tsspcrread in runtime

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

On Wed, 2020-07-15 at 08:21 +0200, Petr Vorel wrote:
> > On Tue, 2020-07-14 at 17:46 +0200, Petr Vorel wrote:
> > > instead of checking in build time as it's runtime dependency.
> > > Also log when tsspcrread not found to make debugging easier.
> 
> > > We search for tsspcrread unless there is tss2-esys with Esys_PCR_Read(),
> > > thus pcr_none.c was dropped as unneeded.
> 
> > > file_exist(), file_exist() and MIN() taken from LTP project.
> 
> > One of these "file_exists" I assume is suppose to be "tst_get_path".
> Yes. I'm sorry, thanks for catching it.
> 
> 
> > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > ---
> > > Hi Mimi,
> 
> > > small improvement based on the current next-testing branch
> > > (9638068aff2476b567185d7eb94126449ad89ca7).
> 
> > > I'm sorry I don't have the required setup, thus didn't test this patch.
> 
> > > Kind regards,
> > > Petr
> 
> > Nice!  It works.
> Thanks a lot for a testing?

Yes, reviewed/tested together.

diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
> > > @@ -47,8 +48,21 @@
> 
> > >  #include "utils.h"
> 
> > > -int tpm2_pcr_supported(void)
> > > +#define CMD "tsspcrread"
> > > +
> > > +static char path[PATH_MAX];
> > > +
> > > +int tpm2_pcr_supported(char **errmsg)
> > >  {
> > > +	int ret;
> > > +
> > > +	if (get_cmd_path(CMD, path, sizeof(path))) {
> > > +		ret = asprintf(errmsg, "Couldn't find '%s' in $PATH", CMD);
> > > +		if (ret == -1)	/* the contents of errmsg is undefined */
> > > +			*errmsg = NULL;
> > > +		return 0;
> > > +	}
> > > +
> 
> > Any chance you could also emit the pathname on success as well?
> 
> Do you mean to print it into stderr:
> 
> int tpm2_pcr_supported(char **errmsg)
> {
> 	int ret;
> 
> 	if (get_cmd_path(CMD, path, sizeof(path))) {
> 		ret = asprintf(errmsg, "Couldn't find '%s' in $PATH", CMD);
> 		if (ret == -1)	/* the contents of errmsg is undefined */
> 			*errmsg = NULL;
> 		return 0;
> 	}
> 
> 	ret = asprintf(errmsg, "Found '%s' in $PATH", CMD);
> 	if (ret == -1)	/* the contents of errmsg is undefined */
> 		*errmsg = NULL;
> 	return 1;
> }
> 

When running these tests remotely, it helps to know which method of
reading the PCRs is used.  How about adding something like this to
both instances of tpm2_pcr_supported()?

        if (imaevm_params.verbose > LOG_INFO)
                log_info("Using %s to read PCRs.\n", CMD);

> Shell I post v2 or you amend my patch?

Either way is fine. 

> BTW I was thinking to create custom function / macro for handling errmsg to
> reduce duplicity.

Sure, I assume that would be in addition to log_err() and log_errno().

> 
> + there is minor warning on newer gcc, I'm not sure how to fix that:
> 
> evmctl.c: In function ‘read_tpm_banks’:
> evmctl.c:1404:25: warning: ‘%2.2d’ directive writing between 2 and 10 bytes into a region of size 3 [-Wformat-overflow=]
>  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
>       |                         ^~~~~
> evmctl.c:1404:20: note: directive argument in the range [0, 2147483647]
>  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
>       |                    ^~~~~~~~~~~
> evmctl.c:1404:3: note: ‘sprintf’ output between 7 and 15 bytes into a destination of size 7
>  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Interesting.  Checking that "i" isn't greater than 99 solves this
warning.  Changing pcr_str size from 7 to 8 solves the other warning.

Mimi