Message-ID: <deec5230-b72e-3325-3dfc-fb8c818526a4@linux.ibm.com>
Date:   Tue, 31 Jan 2023 17:39:20 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH ima-evm-utils v2] Add tests for MMAP_CHECK and
 MMAP_CHECK_REQPROT hooks
Content-Language: en-US
To:     Roberto Sassu <roberto.sassu@huaweicloud.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>
References: <20230131174245.2343342-1-roberto.sassu@huaweicloud.com>
 <20230131174245.2343342-3-roberto.sassu@huaweicloud.com>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20230131174245.2343342-3-roberto.sassu@huaweicloud.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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...

> +
> +	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)

> +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

> +++ 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?

    Stefan