Message-ID: <f0da60dd3cb3ed7857aaa64fb5289d5aace844cd.camel@huaweicloud.com>
Subject: Re: [PATCH ima-evm-utils v2] Add tests for MMAP_CHECK and
 MMAP_CHECK_REQPROT hooks
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
To:     Stefan Berger <stefanb@linux.ibm.com>, zohar@linux.ibm.com,
        dmitry.kasatkin@gmail.com, jmorris@namei.org, serge@hallyn.com
Cc:     linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, viro@zeniv.linux.org.uk,
        pvorel@suse.cz, Roberto Sassu <roberto.sassu@huawei.com>
Date:   Wed, 01 Feb 2023 17:05:18 +0100
In-Reply-To: <deec5230-b72e-3325-3dfc-fb8c818526a4@linux.ibm.com>
References: <20230131174245.2343342-1-roberto.sassu@huaweicloud.com>
         <20230131174245.2343342-3-roberto.sassu@huaweicloud.com>
         <deec5230-b72e-3325-3dfc-fb8c818526a4@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2023-01-31 at 17:39 -0500, Stefan Berger wrote:
> 
> On 1/31/23 12:42, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > +check_mmap() {
> > +	local hook="$1"
> > +	local arg="$2"
> > +	local test_file fowner rule result test_file_entry
> > +
> > +	echo -e "\nTest: ${FUNCNAME[0]} (hook=\"$hook\", test_mmap arg: \"$arg\")"
> > +
> > +	if ! test_file=$(mktemp -p "$PWD"); then
> > +		echo "${RED}Cannot write $test_file${NORM}"
> > +		return "$HARDFAIL"
> > +	fi
> > +
> > +	fowner="$MMAP_CHECK_FOWNER"
> > +	rule="$MEASURE_MMAP_CHECK_RULE"
> > +
> > +	if [ "$hook" = "MMAP_CHECK_REQPROT" ]; then
> > +		fowner="$MMAP_CHECK_REQPROT_FOWNER"
> > +		rule="$MEASURE_MMAP_CHECK_REQPROT_RULE"
> > +	fi
> > +
> > +	if ! chown "$fowner" "$test_file"; then
> > +		echo "${RED}Cannot change owner of $test_file${NORM}"
> > +		return "$HARDFAIL"
> > +	fi
> > +
> > +	check_load_ima_rule "$rule"
> > +	result=$?
> > +	if [ $result -ne "$OK" ]; then
> > +		return $result
> > +	fi
> > +
> > +	test_mmap "$test_file" "$arg"
> 
> In this case it should succeed or fail depending on the $rule?  I am just wondering whether to check $? here as well for expected outcome...

I agree. For the check_mmap() test, test_mmap is always expected to
succeed.

> > +
> > +	if [ "$TFAIL" != "yes" ]; then
> > +		echo -n "Result (expect found): "
> > +	else
> > +		echo -n "Result (expect not found): "
> > +	fi
> > +
> > +	test_file_entry=$(awk '$5 == "'"$test_file"'"' < /sys/kernel/security/ima/ascii_runtime_measurements)
> > +	if [ -z "$test_file_entry" ]; then
> > +		echo "not found"
> > +		return "$FAIL"
> > +	fi
> > +
> > +	echo "found"
> > +	return "$OK"
> > +}
> > +if [ -n "$TST_KEY_PATH" ]; then
> > +	if [ "${TST_KEY_PATH:0:1}" != "/" ]; then
> > +		echo "${RED}Absolute path required for the signing key${NORM}"
> > +		exit "$FAIL"
> > +	fi
> > +
> > +	if [ ! -f "$TST_KEY_PATH" ]; then
> > +		echo "${RED}Kernel signing key not found in $TST_KEY_PATH${NORM}"
> > +		exit "$FAIL"
> > +	fi
> > +
> > +	key_path="$TST_KEY_PATH"
> 
> g_key_path ? or pass as parameter to check_deny (better IMO)

There are other global variables. Also the expect_ lines now are clean
and say more or less what the test is about. Maybe better g_key_path,
will think about for new tests.

> > +elif [ -f "$PWD/../signing_key.pem" ]; then
> > +	key_path="$PWD/../signing_key.pem"
> > +elif [ -f "/lib/modules/$(uname -r)/source/certs/signing_key.pem" ]; then
> > +	key_path="/lib/modules/$(uname -r)/source/certs/signing_key.pem"
> > +elif [ -f "/lib/modules/$(uname -r)/build/certs/signing_key.pem" ]; then
> > +	key_path="/lib/modules/$(uname -r)/build/certs/signing_key.pem"
> > +else
> > +	echo "${CYAN}Kernel signing key not found${NORM}"
> > +	exit "$SKIP"
> > +fi
> > +
> > +key_path_der=$(mktemp)
> 
> g_key_path_der for consistency

Ok.

> > +++ b/tests/test_mmap.c
> > @@ -0,0 +1,75 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Tool to test IMA MMAP_CHECK and MMAP_CHECK_REQPROT hooks.
> > + */
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <sys/stat.h>
> > +#include <sys/mman.h>
> > +#include <sys/personality.h>
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	struct stat st;
> > +	void *ptr, *ptr_write = NULL;
> > +	int ret, fd, fd_write, prot = PROT_READ;
> > +
> > +	if (!argv[1])
> > +		return -ENOENT;
> > +
> > +	if (argv[2] && !strcmp(argv[2], "read_implies_exec")) {
> > +		ret = personality(READ_IMPLIES_EXEC);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	if (stat(argv[1], &st) == -1)
> > +		return -errno;
> > +
> > +	if (argv[2] && !strcmp(argv[2], "exec_on_writable")) {
> > +		fd_write = open(argv[1], O_RDWR);
> > +		if (fd_write == -1)
> > +			return -errno;
> > +
> > +		ptr_write = mmap(0, st.st_size, PROT_WRITE, MAP_SHARED,
> > +				 fd_write, 0);
> > +		close(fd_write);
> > +
> > +		if (ptr_write == (void *)-1)
> > +			return -errno;
> > +	}
> > +
> > +	fd = open(argv[1], O_RDONLY);
> > +	if (fd == -1) {
> > +		if (ptr_write)
> > +			munmap(ptr_write, st.st_size);
> > +
> > +		return -errno;
> > +	}
> > +
> > +	if (argv[2] && !strncmp(argv[2], "exec", 4))
> > +		prot |= PROT_EXEC;
> > +
> > +	ptr = mmap(0, st.st_size, prot, MAP_PRIVATE, fd, 0);
> > +
> > +	close(fd);
> > +
> > +	if (ptr_write)
> > +		munmap(ptr_write, st.st_size);
> > +
> > +	if (ptr == (void *)-1)
> > +		return -errno;
> > +
> > +	ret = 0;
> > +
> > +	if (argv[2] && !strcmp(argv[2], "mprotect"))
> > +		ret = mprotect(ptr, st.st_size, PROT_EXEC);
> > +
> > +	munmap(ptr, st.st_size);
> > +	return ret;
> > +}
> 
> Are there any unexpected failure cases here where it should report an error to the user?

Uhm, ok. I differentiated when an error could occur from when it should
not.

Roberto