Re: [PATCH v2.1 6/7] ima-evm-utils: Extract digest algorithms from hash_info.h

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

On Sat, 2018-12-01 at 06:41 +0300, Vitaly Chikunov wrote:
> On Fri, Nov 30, 2018 at 02:22:28PM -0500, Mimi Zohar wrote:
> > On Thu, 2018-11-29 at 15:27 +0300, Vitaly Chikunov wrote:
> > > If configured with "--with-kernel-headers=PATH" try to extract hash
> > > algorithms from "hash_info.h" from the kernel source tree or
> > > kernel-headers package located in the specified path. (Otherwise, it
> > > will be tried to get from the installed kernel.)
> > > 
> > > This also introduces two algorithm lists, one is built-in and another is
> > > from the kernel source. (They should never contain conflicting algorithm
> > > IDs by their append-only nature.) If the digest is not found in the
> > > built-in list it will be searched in the list from kernel's
> > > "hash_info.h".
> > > 
> > > This patch will allow evmctl to be just recompiled to work with digest
> > > algorithms introduced in the newer kernels.
> > > 
> > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > ---
> > > Changes since v1:
> > > - New patch.
> > > Changes since v2:
> > > - Mark PATH portion of "--with-kernel-headers=PATH" non-optional and
> > >   change description to reflect correct behavior.
> > 
> > Defaulting to the currently running kernel build tree would have been
> > nice.
> 
> It already does. As description states "(Otherwise, it will be tried to get
> from the installed kernel.)" Also:
> 
> > > +AC_ARG_WITH(kernel_headers, [AS_HELP_STRING([--with-kernel-headers=PATH],
> > > +	    [specifies the Linux kernel-headers package location or kernel root directory you want to use])],
> > > +	    [KERNEL_HEADERS="$withval"],
> > > +	    [KERNEL_HEADERS=/lib/modules/$(uname -r)/source])
> 
> if "--with-kernel-headers" is not specified it will default to
> `/lib/modules/$(uname -r)/source` which points to the kernel-headers or
> kernel source tree root.

I missed that.

<snip>

> > > +	/* first iterate over builtin algorithms */
> > >  	for (i = 0; i < PKEY_HASH__LAST; i++)
> > >  		if (pkey_hash_algo[i] &&
> > >  		    !strcmp(algo, pkey_hash_algo[i]))
> > >  			return i;
> > > 
> > > +	/* iterate over algorithms provided by kernel-headers */
> > > +	for (i = 0; i < HASH_ALGO__LAST; i++) {
> > > +		if (hash_algo_name[i] &&
> > > +		    !algocmp(algo, hash_algo_name[i]))
> > > +			return i;
> > > +	}
> > 
> > Assuming the two lists are in sync, which they should be, "i" could be
> > set to PKEY_HASH__LAST.
> 
> Can not do that, since pkey_hash_algo is currently sparse array and could
> skip entries which are present in hash_algo_name.

Missed that too.

> 
> Note, that since "[PATCH v2 7/7] ima-evm-utils: Try to load digest by
> its alias" lists will have different semantics: the pkey_hash_algo list is
> compared algo names with strmatch which supports algo aliases, and the
> hash_algo_name list is compared using algocmp which does not compare
> non-alphanumeric chars such as `_` and `-`, so streebog-512, streebog_512
> or just streebog512 all would work (to match algo id).
> 
> Concluding, if you would not insist on parsing crypto/hash_info.c, I
> don't need fixing anything in this patch.

Agreed, the patch looks fine as is.

Mimi