Message-Id: <1549288166.4146.80.camel@linux.ibm.com>
Date:   Mon, 04 Feb 2019 08:49:26 -0500
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Petr Vorel <pvorel@...e.cz>
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, kexec@...ts.infradead.org,
        David Howells <dhowells@...hat.com>,
        Dave Young <dyoung@...hat.com>,
        Eric Biederman <ebiederm@...ssion.com>,
        Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH 3/3] selftests/ima: kexec_file_load syscall test

On Sun, 2019-02-03 at 23:02 +0100, Petr Vorel wrote:
> Hi Mimi,
> 
> > The kernel can be configured to verify PE signed kernel images, IMA
> > kernel image signatures, both types of signatures, or none.  This test
> > verifies only properly signed kernel images are loaded into memory,
> > based on the kernel configuration and runtime policies.
> 
> > Signed-off-by: Mimi Zohar <zohar@...ux.ibm.com>
> Reviewed-by: Petr Vorel <pvorel@...e.cz>

Thank you for the specific and generic suggestions to simplify/clean
up the tests!  The suggestions, below, and the "print" helpers will
really make a difference.

Mimi

> 
> ...
> > +++ b/tools/testing/selftests/ima/test_kexec_file_load.sh
> > @@ -0,0 +1,250 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0+
> # SPDX-License-Identifier: GPL-2.0-or-later
> > +#
> > +# Loading a kernel image via the kexec_file_load syscall can verify either
> > +# the IMA signature stored in the security.ima xattr or the PE signature,
> > +# both signatures depending on the IMA policy, or none.
> > +#
> > +# To determine whether the kernel image is signed, this test depends
> > +# on pesign and getfattr.  This test also requires the kernel to be
> > +# built with CONFIG_IKCONFIG enabled and either CONFIG_IKCONFIG_PROC
> > +# enabled or access to the extract-ikconfig script.
> > +
> > +VERBOSE=1
> Maybe allow to disable verbose without source change?
> VERBOSE="${VERBOSE:-1}"
> 
> > +EXTRACT_IKCONFIG=$(ls /lib/modules/`uname -r`/source/scripts/extract-ikconfig)
> > +IKCONFIG=/tmp/config-`uname -r`
> > +PROC_CONFIG="/proc/config.gz"
> > +KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
> > +PESIGN=/usr/bin/pesign
> > +GETFATTR=/usr/bin/getfattr
> > +
> > +TEST="$0"
> > +. ./common_lib.sh
> > +
> > +# Kselftest framework requirement - SKIP code is 4.
> > +ksft_skip=4
> > +
> > +kconfig_enabled()
> > +{
> > +	RC=0
> > +	egrep -q $1 $IKCONFIG
> > +	if [ $? -eq 0 ]; then
> > +		RC=1
> > +	fi
> > +	return $RC
> > +}
> This would be enough (grep with -e returns only 0 or 1):
> kconfig_enabled()
> {
> 	grep -E -q $1 $IKCONFIG
> }
> > +
> > +# policy rule format: action func=<keyword> [appraise_type=<type>]
> > +check_ima_policy()
> > +{
> > +	IMA_POLICY=/sys/kernel/security/ima/policy
> > +
> > +	RC=0
> > +	if [ $# -eq 3 ]; then
> > +		grep -e $2 $IMA_POLICY | grep -e "^$1.*$3" 2>&1 >/dev/null
> > +	else
> > +		grep -e $2 $IMA_POLICY | grep -e "^$1" 2>&1 >/dev/null
> > +	fi
> > +	if [ $? -eq 0 ]; then
> > +		RC=1
> > +	fi
> > +	return $RC
> > +}
> This would be enough and more descriptive:
> check_ima_policy()
> {
> 	local action="$1"
> 	local keyword="$2"
> 	local type="$3"
> 
> 	[ -n "$type" ] && type="appraise_type=$type"
> 	grep -q "^$action.*func=$keyword.*$type" /sys/kernel/security/ima/policy
> }
> 
> > +
> > +check_kconfig_options()
> > +{
> > +	# Attempt to get the kernel config first via proc, and then by
> > +	# extracting it from the kernel image using scripts/extract-ikconfig.
> > +	if [ ! -f $PROC_CONFIG ]; then
> > +		modprobe configs 2>/dev/null
> > +	fi
> > +	if [ -f $PROC_CONFIG ]; then
> > +		cat $PROC_CONFIG > $IKCONFIG
> > +	fi
> > +
> > +	if [ ! -f $IKCONFIG ]; then
> > +		if [ ! -f $EXTRACT_IKCONFIG ]; then
> > +			echo "$TEST: requires access to extract-ikconfig" >&2
> > +			exit $ksft_skip
> > +		fi
> > +
> > +		$EXTRACT_IKCONFIG $KERNEL_IMAGE > $IKCONFIG
> > +		kconfig_enabled "CONFIG_IKCONFIG=y"
> > +		if [ $? -eq 0 ]; then
> > +			echo "$TEST: requires the kernel to be built with CONFIG_IKCONFIG" >&2
> > +			exit $ksft_skip
> > +		fi
> > +	fi
> > +
> > +	kconfig_enabled "CONFIG_KEXEC_BZIMAGE_VERIFY_SIG=y"
> > +	pe_sig_required=$?
> > +	if [ $VERBOSE -ne 0 ] && [ $pe_sig_required -eq 1 ]; then
> > +		echo "$TEST: [INFO] PE signed kernel image required"
> > +	fi
> Checks for $VERBOSE here and in other kconfig_enabled usages bellow are a bit
> redundant. And you check for assigned variable now and then later on,
> you use these variables as global (and reset $ima_sig_required in
> check_runtime().
> 
> How about using functions instead:
> log_info()
> {
> 	echo "$TEST: [INFO] $1"
> }
> (Reducing some duplicity, IMHO helper functions in shell library used in all
> selftest tests would be useful)
> 
> kconfig_enabled()
> {
> 	local config="$1"
> 	local msg="$2"
> 	local ret
> 
> 	grep -E -q $config $IKCONFIG
> 	ret=$?
> 	[ $VERBOSE -ne 0 ] && [ $ret -eq 1 ] && log_info "$msg"
> 	return $ret
> }
> 
> ima_sig_enabled()
> {
> 	kconfig_enabled "CONFIG_KEXEC_BZIMAGE_VERIFY_SIG=y" \
> 		"PE signed kernel image required"
> }
> 
> ima_sig_enabled()
> {
> 	kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS=y" \
> 		"IMA kernel image signature required"
> }
> Warning is printed each time, but that's deliberate.
> If it's not wanted, it can be moved into setup.
> 
> ...
> > +check_kconfig_options
> > +check_for_apps
> > +check_runtime
> > +check_for_sigs
> > +kexec_file_load_test
> 
> > +rc=$?
> > +exit $rc
> These two are redundant.
> 
> Kind regards,
> Petr
> 

Powered by blists - more mailing lists