[PATCH v2 0/5] selftests/ima: add kexec and kernel module tests

[PATCH v2 0/5] selftests/ima: add kexec and kernel module tests

[Mimi Zohar]

The kernel can be configured to require kexec kernel images and kernel
modules are signed.  An IMA policy can be specified on the boot command
line or a custom IMA policy loaded requiring the kexec kernel image and
kernel modules be signed.  In addition, systems booted in secure boot
mode with the IMA architecture specific policy enabled, require validly
signed kexec kernel images and kernel modules.

There are two methods of signing kernel images and two methods of
signing kernel modules.  In addition, there are two syscalls for each.

kernel image:  PE signature, IMA signature
kexec syscalls: kexec_load, kexec_file_load

Both the PE and IMA kernel image signature can only be verified when
loaded via the kexec_file_load syscall.

kernel moodule: appended signature, IMA signature
kernel module syscalls: init_module, finit_module

The appended kernel module signature can be verified when the kernel
module is loaded via either syscall.  The IMA kernel module signature
can only be verified when the kernel module is loaded via the
finit_module syscall.

The selftests in this patch set verify that only signed kernel images
and kernel modules are loaded as required, based on the kernel config,
the secure boot mode, and the IMA runtime policy.

Loading a kernel image or kernel module requires root privileges.  To
run just the IMA selftests: sudo make TARGETS=ima kselftest

Mimi Zohar (5):
  selftests/ima: cleanup the kexec selftest
  selftests/ima: define a set of common functions
  selftests/ima: define common logging functions
  selftests/ima: kexec_file_load syscall test
  selftests/ima: loading kernel modules

 tools/testing/selftests/ima/Makefile               |   3 +-
 tools/testing/selftests/ima/common_lib.sh          | 154 ++++++++++++++++
 tools/testing/selftests/ima/test_kernel_module.sh  |  96 ++++++++++
 .../testing/selftests/ima/test_kexec_file_load.sh  | 195 +++++++++++++++++++++
 tools/testing/selftests/ima/test_kexec_load.sh     |  53 ++----
 5 files changed, 463 insertions(+), 38 deletions(-)
 create mode 100755 tools/testing/selftests/ima/common_lib.sh
 create mode 100755 tools/testing/selftests/ima/test_kernel_module.sh
 create mode 100755 tools/testing/selftests/ima/test_kexec_file_load.sh

-- 
2.7.5

[PATCH v2 1/5] selftests/ima: cleanup the kexec selftest

[Mimi Zohar]

Remove the few bashisms and use the complete option name for clarity.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 tools/testing/selftests/ima/test_kexec_load.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
index 1c10093fb526..0345803e7bec 100755
--- a/tools/testing/selftests/ima/test_kexec_load.sh
+++ b/tools/testing/selftests/ima/test_kexec_load.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
-# SPDX-License-Identifier: GPL-2.0+
+# SPDX-License-Identifier: GPL-2.0-or-later
 # Loading a kernel image via the kexec_load syscall should fail
-# when the kerne is CONFIG_KEXEC_VERIFY_SIG enabled and the system
+# when the kernel is CONFIG_KEXEC_VERIFY_SIG enabled and the system
 # is booted in secureboot mode.
 
 TEST="$0"
@@ -12,8 +12,8 @@ rc=0
 ksft_skip=4
 
 # kexec requires root privileges
-if [ $UID != 0 ]; then
-	echo "$TEST: must be run as root" >&2
+if [ $(id -ru) -ne 0 ]; then
+	echo "$TEST: requires root privileges" >&2
 	exit $ksft_skip
 fi
 
@@ -33,17 +33,17 @@ secureboot=`hexdump $file | awk '{print substr($4,length($4),1)}'`
 
 # kexec_load should fail in secure boot mode
 KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
-kexec -l $KERNEL_IMAGE &>> /dev/null
-if [ $? == 0 ]; then
-	kexec -u
-	if [ "$secureboot" == "1" ]; then
+kexec --load $KERNEL_IMAGE 2>&1 > /dev/null
+if [ $? -eq 0 ]; then
+	kexec --unload
+	if [ $secureboot -eq 1 ]; then
 		echo "$TEST: kexec_load succeeded [FAIL]"
 		rc=1
 	else
 		echo "$TEST: kexec_load succeeded [PASS]"
 	fi
 else
-	if [ "$secureboot" == "1" ]; then
+	if [ $secureboot -eq 1 ]; then
 		echo "$TEST: kexec_load failed [PASS]"
 	else
 		echo "$TEST: kexec_load failed [FAIL]"
-- 
2.7.5

Re: [PATCH v2 1/5] selftests/ima: cleanup the kexec selftest

[shuah]

Hi Mimi,

Thanks for the patches.

On 2/26/19 4:26 PM, Mimi Zohar wrote:
> Remove the few bashisms and use the complete option name for clarity.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
>   tools/testing/selftests/ima/test_kexec_load.sh | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
> index 1c10093fb526..0345803e7bec 100755
> --- a/tools/testing/selftests/ima/test_kexec_load.sh
> +++ b/tools/testing/selftests/ima/test_kexec_load.sh
> @@ -1,7 +1,7 @@
>   #!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0+
> +# SPDX-License-Identifier: GPL-2.0-or-later

# SPDX-License-Identifier: GPL-2.0

>   # Loading a kernel image via the kexec_load syscall should fail
> -# when the kerne is CONFIG_KEXEC_VERIFY_SIG enabled and the system
> +# when the kernel is CONFIG_KEXEC_VERIFY_SIG enabled and the system
>   # is booted in secureboot mode.
>   
>   TEST="$0"
> @@ -12,8 +12,8 @@ rc=0
>   ksft_skip=4
>   
>   # kexec requires root privileges
> -if [ $UID != 0 ]; then
> -	echo "$TEST: must be run as root" >&2
> +if [ $(id -ru) -ne 0 ]; then
> +	echo "$TEST: requires root privileges" >&2
>   	exit $ksft_skip
>   fi
>   
> @@ -33,17 +33,17 @@ secureboot=`hexdump $file | awk '{print substr($4,length($4),1)}'`
>   
>   # kexec_load should fail in secure boot mode
>   KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
> -kexec -l $KERNEL_IMAGE &>> /dev/null
> -if [ $? == 0 ]; then
> -	kexec -u
> -	if [ "$secureboot" == "1" ]; then
> +kexec --load $KERNEL_IMAGE 2>&1 > /dev/null
> +if [ $? -eq 0 ]; then
> +	kexec --unload
> +	if [ $secureboot -eq 1 ]; then
>   		echo "$TEST: kexec_load succeeded [FAIL]"
>   		rc=1
>   	else
>   		echo "$TEST: kexec_load succeeded [PASS]"
>   	fi
>   else
> -	if [ "$secureboot" == "1" ]; then
> +	if [ $secureboot -eq 1 ]; then
>   		echo "$TEST: kexec_load failed [PASS]"
>   	else
>   		echo "$TEST: kexec_load failed [FAIL]"
> 

The rest looks good to me.

thanks,
-- Shuah

[PATCH v2 2/5] selftests/ima: define a set of common functions

[Mimi Zohar]

Define, update and move get_secureboot_mode() to a common file for use
by other tests.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 tools/testing/selftests/ima/Makefile           |  1 +
 tools/testing/selftests/ima/common_lib.sh      | 24 ++++++++++++++++++++++++
 tools/testing/selftests/ima/test_kexec_load.sh | 17 +++--------------
 3 files changed, 28 insertions(+), 14 deletions(-)
 create mode 100755 tools/testing/selftests/ima/common_lib.sh

diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
index 0b3adf5444b6..46b9e04d2737 100644
--- a/tools/testing/selftests/ima/Makefile
+++ b/tools/testing/selftests/ima/Makefile
@@ -5,6 +5,7 @@ ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
 
 ifeq ($(ARCH),x86)
 TEST_PROGS := test_kexec_load.sh
+TEST_FILES := common_lib.sh
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
new file mode 100755
index 000000000000..5583ea74c14e
--- /dev/null
+++ b/tools/testing/selftests/ima/common_lib.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+# The secure boot mode can be accessed either as the last integer
+# of "od -An -t u1 /sys/firmware/efi/efivars/SecureBoot-*" or from
+# "od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data".
+# Return 1 for SecureBoot mode enabled.
+get_secureboot_mode()
+{
+	local efivarfs="/sys/firmware/efi/efivars"
+	# Make sure that efivars is mounted in the normal location
+	if ! grep -q "^\S\+ $efivarfs efivarfs" /proc/mounts; then
+		echo "$TEST: efivars is not mounted on $efivarfs" >&2
+		exit $ksft_skip
+	fi
+
+	# Get secureboot mode
+	local file="$efivarfs/../vars/SecureBoot-*/data"
+	if [ ! -e $file ]; then
+		echo "$TEST: unknown secureboot mode" >&2
+		exit $ksft_skip
+	fi
+	return `od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data`
+}
diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
index 0345803e7bec..35934e0665f1 100755
--- a/tools/testing/selftests/ima/test_kexec_load.sh
+++ b/tools/testing/selftests/ima/test_kexec_load.sh
@@ -5,7 +5,7 @@
 # is booted in secureboot mode.
 
 TEST="$0"
-EFIVARFS="/sys/firmware/efi/efivars"
+. ./common_lib.sh
 rc=0
 
 # Kselftest framework requirement - SKIP code is 4.
@@ -17,19 +17,8 @@ if [ $(id -ru) -ne 0 ]; then
 	exit $ksft_skip
 fi
 
-# Make sure that efivars is mounted in the normal location
-if ! grep -q "^\S\+ $EFIVARFS efivarfs" /proc/mounts; then
-	echo "$TEST: efivars is not mounted on $EFIVARFS" >&2
-	exit $ksft_skip
-fi
-
-# Get secureboot mode
-file="$EFIVARFS/SecureBoot-*"
-if [ ! -e $file ]; then
-	echo "$TEST: unknown secureboot mode" >&2
-	exit $ksft_skip
-fi
-secureboot=`hexdump $file | awk '{print substr($4,length($4),1)}'`
+get_secureboot_mode
+secureboot=$?
 
 # kexec_load should fail in secure boot mode
 KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
-- 
2.7.5

Re: [PATCH v2 2/5] selftests/ima: define a set of common functions

[shuah]

On 2/26/19 4:26 PM, Mimi Zohar wrote:
> Define, update and move get_secureboot_mode() to a common file for use
> by other tests.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
>   tools/testing/selftests/ima/Makefile           |  1 +
>   tools/testing/selftests/ima/common_lib.sh      | 24 ++++++++++++++++++++++++
>   tools/testing/selftests/ima/test_kexec_load.sh | 17 +++--------------
>   3 files changed, 28 insertions(+), 14 deletions(-)
>   create mode 100755 tools/testing/selftests/ima/common_lib.sh
> 
> diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
> index 0b3adf5444b6..46b9e04d2737 100644
> --- a/tools/testing/selftests/ima/Makefile
> +++ b/tools/testing/selftests/ima/Makefile
> @@ -5,6 +5,7 @@ ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
>   
>   ifeq ($(ARCH),x86)
>   TEST_PROGS := test_kexec_load.sh
> +TEST_FILES := common_lib.sh
>   
>   include ../lib.mk
>   
> diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
> new file mode 100755
> index 000000000000..5583ea74c14e
> --- /dev/null
> +++ b/tools/testing/selftests/ima/common_lib.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later

# SPDX-License-Identifier: GPL-2.0

> +
> +# The secure boot mode can be accessed either as the last integer
> +# of "od -An -t u1 /sys/firmware/efi/efivars/SecureBoot-*" or from
> +# "od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data".
> +# Return 1 for SecureBoot mode enabled.
> +get_secureboot_mode()
> +{
> +	local efivarfs="/sys/firmware/efi/efivars"
> +	# Make sure that efivars is mounted in the normal location
> +	if ! grep -q "^\S\+ $efivarfs efivarfs" /proc/mounts; then
> +		echo "$TEST: efivars is not mounted on $efivarfs" >&2
> +		exit $ksft_skip
> +	fi
> +
> +	# Get secureboot mode
> +	local file="$efivarfs/../vars/SecureBoot-*/data"
> +	if [ ! -e $file ]; then
> +		echo "$TEST: unknown secureboot mode" >&2
> +		exit $ksft_skip
> +	fi
> +	return `od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data`
> +}
> diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
> index 0345803e7bec..35934e0665f1 100755
> --- a/tools/testing/selftests/ima/test_kexec_load.sh
> +++ b/tools/testing/selftests/ima/test_kexec_load.sh
> @@ -5,7 +5,7 @@
>   # is booted in secureboot mode.
>   
>   TEST="$0"
> -EFIVARFS="/sys/firmware/efi/efivars"
> +. ./common_lib.sh
>   rc=0
>   
>   # Kselftest framework requirement - SKIP code is 4.
> @@ -17,19 +17,8 @@ if [ $(id -ru) -ne 0 ]; then
>   	exit $ksft_skip
>   fi
>   
> -# Make sure that efivars is mounted in the normal location
> -if ! grep -q "^\S\+ $EFIVARFS efivarfs" /proc/mounts; then
> -	echo "$TEST: efivars is not mounted on $EFIVARFS" >&2
> -	exit $ksft_skip
> -fi
> -
> -# Get secureboot mode
> -file="$EFIVARFS/SecureBoot-*"
> -if [ ! -e $file ]; then
> -	echo "$TEST: unknown secureboot mode" >&2
> -	exit $ksft_skip
> -fi
> -secureboot=`hexdump $file | awk '{print substr($4,length($4),1)}'`
> +get_secureboot_mode
> +secureboot=$?
>   
>   # kexec_load should fail in secure boot mode
>   KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
> 

The rest looks good to me.

thanks,
-- Shuah

Re: [PATCH v2 2/5] selftests/ima: define a set of common functions

[Petr Vorel]

Hi Mimi,

> Define, update and move get_secureboot_mode() to a common file for use
> by other tests.

> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
>  ifeq ($(ARCH),x86)
>  TEST_PROGS := test_kexec_load.sh
> +TEST_FILES := common_lib.sh
In case there might be some common library for whole selftest, I'd name this ima_lib.sh.
(even it won't be in the same directory it might be better for readability).

>  include ../lib.mk

> diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
> new file mode 100755
> index 000000000000..5583ea74c14e
> --- /dev/null
> +++ b/tools/testing/selftests/ima/common_lib.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
I'm sorry, that was me who advised GPL v2+ instead of GPL v2 only :).

...

Kind regards,
Petr

[PATCH v2 3/5] selftests/ima: define common logging functions

[Mimi Zohar]

Define log_info, log_pass, log_fail, and log_skip functions.

Suggested-by: Petr Vorel <pvorel@suse.cz>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 tools/testing/selftests/ima/common_lib.sh      | 43 +++++++++++++++++++++++---
 tools/testing/selftests/ima/test_kexec_load.sh | 19 +++---------
 2 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
index 5583ea74c14e..c6d04006281d 100755
--- a/tools/testing/selftests/ima/common_lib.sh
+++ b/tools/testing/selftests/ima/common_lib.sh
@@ -1,5 +1,36 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0-or-later
+#
+# Kselftest framework defines: ksft_pass=0, ksft_fail=1, ksft_skip=4
+
+VERBOSE="${VERBOSE:-1}"
+
+log_info()
+{
+	[ $VERBOSE -ne 0 ] && echo "[INFO] $1"
+}
+
+# The ksefltest framework requirement returns 0 for PASS.
+log_pass()
+{
+
+	[ $VERBOSE -ne 0 ] && echo "$1 [PASS]"
+	exit 0
+}
+
+# The ksefltest framework requirement returns 1 for FAIL.
+log_fail()
+{
+	[ $VERBOSE -ne 0 ] && echo "$1 [FAIL]"
+	exit 1
+}
+
+# The ksefltest framework requirement returns 4 for SKIP.
+log_skip()
+{
+	[ $VERBOSE -ne 0 ] && echo "$1"
+	exit 4
+}
 
 # The secure boot mode can be accessed either as the last integer
 # of "od -An -t u1 /sys/firmware/efi/efivars/SecureBoot-*" or from
@@ -8,17 +39,19 @@
 get_secureboot_mode()
 {
 	local efivarfs="/sys/firmware/efi/efivars"
+
 	# Make sure that efivars is mounted in the normal location
 	if ! grep -q "^\S\+ $efivarfs efivarfs" /proc/mounts; then
-		echo "$TEST: efivars is not mounted on $efivarfs" >&2
-		exit $ksft_skip
+		log_skip "efivars is not mounted on $efivarfs"
 	fi
 
 	# Get secureboot mode
 	local file="$efivarfs/../vars/SecureBoot-*/data"
 	if [ ! -e $file ]; then
-		echo "$TEST: unknown secureboot mode" >&2
-		exit $ksft_skip
+		log_skip "unknown secureboot mode"
 	fi
-	return `od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data`
+	ret=`od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data`
+	[ $ret -eq 1 ] && log_info "secure boot mode enabled"
+
+	return $ret
 }
diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
index 35934e0665f1..8b99017538ba 100755
--- a/tools/testing/selftests/ima/test_kexec_load.sh
+++ b/tools/testing/selftests/ima/test_kexec_load.sh
@@ -6,15 +6,10 @@
 
 TEST="$0"
 . ./common_lib.sh
-rc=0
-
-# Kselftest framework requirement - SKIP code is 4.
-ksft_skip=4
 
 # kexec requires root privileges
 if [ $(id -ru) -ne 0 ]; then
-	echo "$TEST: requires root privileges" >&2
-	exit $ksft_skip
+	log_skip "requires root privileges" >&2
 fi
 
 get_secureboot_mode
@@ -26,18 +21,14 @@ kexec --load $KERNEL_IMAGE 2>&1 > /dev/null
 if [ $? -eq 0 ]; then
 	kexec --unload
 	if [ $secureboot -eq 1 ]; then
-		echo "$TEST: kexec_load succeeded [FAIL]"
-		rc=1
+		log_fail "kexec_load succeeded"
 	else
-		echo "$TEST: kexec_load succeeded [PASS]"
+		log_pass "kexec_load succeeded"
 	fi
 else
 	if [ $secureboot -eq 1 ]; then
-		echo "$TEST: kexec_load failed [PASS]"
+		log_pass "kexec_load failed"
 	else
-		echo "$TEST: kexec_load failed [FAIL]"
-		rc=1
+		log_fail "kexec_load failed"
 	fi
 fi
-
-exit $rc
-- 
2.7.5

Re: [PATCH v2 3/5] selftests/ima: define common logging functions

[shuah]

On 2/26/19 4:26 PM, Mimi Zohar wrote:
> Define log_info, log_pass, log_fail, and log_skip functions.
> 
> Suggested-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   tools/testing/selftests/ima/common_lib.sh      | 43 +++++++++++++++++++++++---
>   tools/testing/selftests/ima/test_kexec_load.sh | 19 +++---------
>   2 files changed, 43 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
> index 5583ea74c14e..c6d04006281d 100755
> --- a/tools/testing/selftests/ima/common_lib.sh
> +++ b/tools/testing/selftests/ima/common_lib.sh
> @@ -1,5 +1,36 @@
>   #!/bin/sh
>   # SPDX-License-Identifier: GPL-2.0-or-later

Same here:

# SPDX-License-Identifier: GPL-2.0

thanks,
-- Shuah

Re: [PATCH v2 3/5] selftests/ima: define common logging functions

[Petr Vorel]

Hi Mimi,

> Define log_info, log_pass, log_fail, and log_skip functions.

> Suggested-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  tools/testing/selftests/ima/common_lib.sh      | 43 +++++++++++++++++++++++---
>  tools/testing/selftests/ima/test_kexec_load.sh | 19 +++---------
>  2 files changed, 43 insertions(+), 19 deletions(-)

> diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
> index 5583ea74c14e..c6d04006281d 100755
> --- a/tools/testing/selftests/ima/common_lib.sh
> +++ b/tools/testing/selftests/ima/common_lib.sh
> @@ -1,5 +1,36 @@
>  #!/bin/sh
>  # SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Kselftest framework defines: ksft_pass=0, ksft_fail=1, ksft_skip=4
> +
> +VERBOSE="${VERBOSE:-1}"
> +
> +log_info()
> +{
> +	[ $VERBOSE -ne 0 ] && echo "[INFO] $1"
> +}
> +
> +# The ksefltest framework requirement returns 0 for PASS.
> +log_pass()
> +{
> +
> +	[ $VERBOSE -ne 0 ] && echo "$1 [PASS]"
> +	exit 0
> +}
> +
> +# The ksefltest framework requirement returns 1 for FAIL.
> +log_fail()
> +{
> +	[ $VERBOSE -ne 0 ] && echo "$1 [FAIL]"
> +	exit 1
> +}
> +
> +# The ksefltest framework requirement returns 4 for SKIP.
> +log_skip()
> +{
> +	[ $VERBOSE -ne 0 ] && echo "$1"
> +	exit 4
> +}
These might be good candidates for moving to selftest specific shell helper
library (for somebody who wants to do this work).

>  # The secure boot mode can be accessed either as the last integer
>  # of "od -An -t u1 /sys/firmware/efi/efivars/SecureBoot-*" or from
> @@ -8,17 +39,19 @@
>  get_secureboot_mode()
>  {
>  	local efivarfs="/sys/firmware/efi/efivars"
> +
>  	# Make sure that efivars is mounted in the normal location
>  	if ! grep -q "^\S\+ $efivarfs efivarfs" /proc/mounts; then
> -		echo "$TEST: efivars is not mounted on $efivarfs" >&2
> -		exit $ksft_skip
> +		log_skip "efivars is not mounted on $efivarfs"
>  	fi

>  	# Get secureboot mode
>  	local file="$efivarfs/../vars/SecureBoot-*/data"
>  	if [ ! -e $file ]; then
> -		echo "$TEST: unknown secureboot mode" >&2
> -		exit $ksft_skip
> +		log_skip "unknown secureboot mode"
>  	fi
> -	return `od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data`
> +	ret=`od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data`
> +	[ $ret -eq 1 ] && log_info "secure boot mode enabled"
> +
> +	return $ret
>  }
> diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
> index 35934e0665f1..8b99017538ba 100755
> --- a/tools/testing/selftests/ima/test_kexec_load.sh
> +++ b/tools/testing/selftests/ima/test_kexec_load.sh
> @@ -6,15 +6,10 @@

>  TEST="$0"
>  . ./common_lib.sh
> -rc=0
> -
> -# Kselftest framework requirement - SKIP code is 4.
> -ksft_skip=4

>  # kexec requires root privileges
>  if [ $(id -ru) -ne 0 ]; then
> -	echo "$TEST: requires root privileges" >&2
> -	exit $ksft_skip
> +	log_skip "requires root privileges" >&2
You left here redirection to stderr.
+ again requiring root could be in helper library.
>  fi

>  get_secureboot_mode
> @@ -26,18 +21,14 @@ kexec --load $KERNEL_IMAGE 2>&1 > /dev/null
>  if [ $? -eq 0 ]; then
>  	kexec --unload
>  	if [ $secureboot -eq 1 ]; then
> -		echo "$TEST: kexec_load succeeded [FAIL]"
> -		rc=1
> +		log_fail "kexec_load succeeded"
>  	else
> -		echo "$TEST: kexec_load succeeded [PASS]"
> +		log_pass "kexec_load succeeded"
>  	fi
>  else
>  	if [ $secureboot -eq 1 ]; then
> -		echo "$TEST: kexec_load failed [PASS]"
> +		log_pass "kexec_load failed"
>  	else
> -		echo "$TEST: kexec_load failed [FAIL]"
> -		rc=1
> +		log_fail "kexec_load failed"
>  	fi
>  fi
> -
> -exit $rc


Kind regards,
Petr

Re: [PATCH v2 3/5] selftests/ima: define common logging functions

[Petr Vorel]

Hi Mimi,

> Define log_info, log_pass, log_fail, and log_skip functions.

> Suggested-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
>  tools/testing/selftests/ima/common_lib.sh      | 43 +++++++++++++++++++++++---
>  tools/testing/selftests/ima/test_kexec_load.sh | 19 +++---------
>  2 files changed, 43 insertions(+), 19 deletions(-)

...
> diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
> index 5583ea74c14e..c6d04006281d 100755
> --- a/tools/testing/selftests/ima/common_lib.sh
...
>  	# Get secureboot mode
>  	local file="$efivarfs/../vars/SecureBoot-*/data"
>  	if [ ! -e $file ]; then
> -		echo "$TEST: unknown secureboot mode" >&2
> -		exit $ksft_skip
> +		log_skip "unknown secureboot mode"
>  	fi
> -	return `od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data`
> +	ret=`od -An -t u1 /sys/firmware/efi/vars/SecureBoot-*/data`
Missing local. Usually it's good to put all locals at the top.

> +	[ $ret -eq 1 ] && log_info "secure boot mode enabled"
> +
> +	return $ret
>  }
...

Kind regards,
Petr

[PATCH v2 4/5] selftests/ima: kexec_file_load syscall test

[Mimi Zohar]

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@linux.ibm.com>
---
 tools/testing/selftests/ima/Makefile               |   2 +-
 tools/testing/selftests/ima/common_lib.sh          |  97 ++++++++++
 .../testing/selftests/ima/test_kexec_file_load.sh  | 195 +++++++++++++++++++++
 tools/testing/selftests/ima/test_kexec_load.sh     |   1 -
 4 files changed, 293 insertions(+), 2 deletions(-)
 create mode 100755 tools/testing/selftests/ima/test_kexec_file_load.sh

diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
index 46b9e04d2737..049c83c9426c 100644
--- a/tools/testing/selftests/ima/Makefile
+++ b/tools/testing/selftests/ima/Makefile
@@ -4,7 +4,7 @@ uname_M := $(shell uname -m 2>/dev/null || echo not)
 ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
 
 ifeq ($(ARCH),x86)
-TEST_PROGS := test_kexec_load.sh
+TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh
 TEST_FILES := common_lib.sh
 
 include ../lib.mk
diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
index c6d04006281d..24091f29bd09 100755
--- a/tools/testing/selftests/ima/common_lib.sh
+++ b/tools/testing/selftests/ima/common_lib.sh
@@ -4,6 +4,9 @@
 # Kselftest framework defines: ksft_pass=0, ksft_fail=1, ksft_skip=4
 
 VERBOSE="${VERBOSE:-1}"
+IKCONFIG="/tmp/config-`uname -r`"
+KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
+SECURITYFS=$(grep "securityfs" /proc/mounts | awk '{print $2}')
 
 log_info()
 {
@@ -55,3 +58,97 @@ get_secureboot_mode()
 
 	return $ret
 }
+
+# Look for config option in Kconfig file.
+# Return 1 for found and 0 for not found.
+kconfig_enabled()
+{
+	local config="$1"
+	local msg="$2"
+
+        grep -E -q $config $IKCONFIG
+        if [ $? -eq 0 ]; then
+                log_info "$msg"
+                return 1
+        fi
+        return 0
+}
+
+# Attempt to get the kernel config first via proc, and then by
+# extracting it from the kernel image or the configs.ko using
+# scripts/extract-ikconfig.
+# Return 1 for found and 0 for not found.
+get_kconfig()
+{
+	local proc_config="/proc/config.gz"
+	local module_dir="/lib/modules/`uname -r`"
+	local configs_module="$module_dir/kernel/kernel/configs.ko"
+
+	if [ ! -f $proc_config ]; then
+		modprobe configs > /dev/null 2>&1
+	fi
+	if [ -f $proc_config ]; then
+		cat $proc_config | gunzip > $IKCONFIG 2>/dev/null
+		if [ $? -eq 0 ]; then
+			return 1
+		fi
+	fi
+
+	local extract_ikconfig="$module_dir/source/scripts/extract-ikconfig"
+	if [ ! -f $extract_ikconfig ]; then
+		log_skip "extract-ikconfig not found"
+	fi
+
+	$extract_ikconfig $KERNEL_IMAGE > $IKCONFIG 2>/dev/null
+	if [ $? -eq 1 ]; then
+		if [ ! -f $configs_module ]; then
+			log_skip "CONFIG_IKCONFIG not enabled"
+		fi
+		$extract_ikconfig $configs_module > $IKCONFIG
+		if [ $? -eq 1 ]; then
+			log_skip "CONFIG_IKCONFIG not enabled"
+		fi
+	fi
+	return 1
+}
+
+# Make sure that securityfs is mounted
+mount_securityfs()
+{
+	if [ -z $SECURITYFS ]; then
+		SECURITYFS=/sys/kernel/security
+		mount -t securityfs security $SECURITYFS
+	fi
+
+	if [ ! -d "$SECURITYFS" ]; then
+		log_fail "$SECURITYFS :securityfs is not mounted"
+	fi
+}
+
+# The policy rule format is an "action" followed by key-value pairs.  This
+# function supports up to two key-value pairs, in any order.
+# For example: action func=<keyword> [appraise_type=<type>]
+# Return 1 for found and 0 for not found.
+check_ima_policy()
+{
+	local action=$1
+	local keypair1="$2"
+	local keypair2="$3"
+
+	mount_securityfs
+
+	local ima_policy=$SECURITYFS/ima/policy
+	if [ ! -e $ima_policy ]; then
+		log_fail "$ima_policy not found"
+	fi
+
+	if [ -n $keypair2 ]; then
+		grep -e "^$action.*$keypair1" "$ima_policy" | \
+			grep -q -e "$keypair2"
+	else
+		grep -q -e "^$action.*$keypair1" "$ima_policy"
+	fi
+
+	[ $? -eq 0 ] && ret=1 || ret=0
+        return $ret
+}
diff --git a/tools/testing/selftests/ima/test_kexec_file_load.sh b/tools/testing/selftests/ima/test_kexec_file_load.sh
new file mode 100755
index 000000000000..e08c7e6cf28c
--- /dev/null
+++ b/tools/testing/selftests/ima/test_kexec_file_load.sh
@@ -0,0 +1,195 @@
+#!/bin/sh
+# 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.
+
+TEST="KEXEC_FILE_LOAD"
+. ./common_lib.sh
+
+trap "{ rm -f $IKCONFIG ; }" EXIT
+
+# Some of the IMA builtin policies may require the kexec kernel image to
+# be signed, but these policy rules may be replaced with a custom
+# policy.  Only CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS persists after
+# loading a custom policy.  Check if it is enabled, before reading the
+# IMA runtime sysfs policy file.
+# Return 1 for IMA signature required and 0 for not required.
+is_ima_sig_required()
+{
+	local ret=0
+
+	kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS=y" \
+		"IMA kernel image signature required"
+	if [ $? -eq 1 ]; then
+		log_info "IMA signature required"
+		return 1
+	fi
+
+	# The architecture specific or a custom policy may require the
+	# kexec kernel image be signed.  Policy rules are walked
+	# sequentially.  As a result, a policy rule may be defined, but
+	# might not necessarily be used.  This test assumes if a policy
+	# rule is specified, that is the intent.
+	if [ $ima_read_policy -eq 1 ]; then
+		check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
+			"appraise_type=imasig"
+		ret=$?
+		[ $ret -eq 1 ] && log_info "IMA signature required";
+	fi
+	return $ret
+}
+
+# The kexec_file_load_test() is complicated enough, require pesign.
+# Return 1 for PE signature found and 0 for not found.
+check_for_pesig()
+{
+	which pesign > /dev/null 2>&1
+	if [ $?	-eq 1 ]; then
+		log_skip "pesign not found"
+	fi
+
+	pesign -i $KERNEL_IMAGE --show-signature | grep -q "No signatures"
+	local ret=$?
+	if [ $ret -eq 1 ]; then
+		log_info "kexec kernel image PE signed"
+	else
+		log_info "kexec kernel image not PE signed"
+	fi
+	return $ret
+}
+
+# The kexec_file_load_test() is complicated enough, require getfattr.
+# Return 1 for IMA signature found and 0 for not found.
+check_for_imasig()
+{
+	local ret=0
+
+	which getfattr > /dev/null 2>&1
+	if [ $?	-eq 1 ]; then
+		log_skip "getfattr not found"
+	fi
+
+	line=$(getfattr -n security.ima -e hex --absolute-names $KERNEL_IMAGE 2>&1)
+	echo $line | grep -q "security.ima=0x03"
+	if [ $? -eq 0 ]; then
+		ret=1
+		log_info "kexec kernel image IMA signed"
+	else
+		log_info "kexec kernel image not IMA signed"
+	fi
+	return $ret
+}
+
+kexec_file_load_test()
+{
+	local succeed_msg="kexec_file_load succeeded"
+	local failed_msg="kexec_file_load failed"
+	local key_msg="try enabling the CONFIG_INTEGRITY_PLATFORM_KEYRING"
+
+	line=$(kexec --load --kexec-file-syscall $KERNEL_IMAGE 2>&1)
+
+	if [ $? -eq 0 ]; then
+		kexec --unload --kexec-file-syscall
+
+		# In secureboot mode with an architecture  specific
+		# policy, make sure either an IMA or PE signature exists.
+		if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ] && \
+			[ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ]; then
+			log_fail "$succeed_msg (missing sig)"
+		fi
+
+		if [ $pe_sig_required -eq 1 ] && [ $pe_signed -eq 0 ]; then
+			log_fail "$succeed_msg (missing PE sig)"
+		fi
+
+		if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
+			log_fail "$succeed_msg (missing IMA sig)"
+		fi
+
+		if [ $pe_sig_required -eq 0 ] && [ $ima_sig_required -eq 0 ] \
+		    && [ $ima_read_policy -eq 0 ] && [ $ima_signed -eq 0 ]; then
+			log_fail "$succeed_msg (possibly missing IMA sig)"
+		fi
+
+		log_pass "$succeed_msg"
+	fi
+
+	# Check the reason for the kexec_file_load failure
+	echo $line | grep -q "Required key not available"
+	if [ $? -eq 0 ]; then
+		if [ $platform_keyring -eq 0 ]; then
+			log_pass "$failed_msg (-ENOKEY), $key_msg"
+		else
+			log_pass "$failed_msg (-ENOKEY)"
+		fi
+	fi
+
+	if [ $pe_sig_required -eq 1 ] && [ $pe_signed -eq 0 ]; then
+		log_pass "$failed_msg (missing PE sig)"
+	fi
+
+	if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
+		log_pass "$failed_msg (missing IMA sig)"
+	fi
+
+	if [ $pe_sig_required -eq 0 ] && [ $ima_sig_required -eq 0 ] \
+	    && [ $ima_read_policy -eq 0 ] && [ $ima_signed -eq 0 ]; then
+		log_pass "$failed_msg (possibly missing IMA sig)"
+	fi
+
+	log_pass "$failed_msg"
+	return 0
+}
+
+# kexec requires root privileges
+if [ $(id -ru) -ne 0 ]; then
+	log_skip "requires root privileges"
+fi
+
+# get the kernel config
+get_kconfig
+
+# Determine which kernel config options are enabled
+kconfig_enabled "CONFIG_IMA_ARCH_POLICY=y" \
+	"architecture specific policy enabled"
+arch_policy=$?
+
+kconfig_enabled "CONFIG_INTEGRITY_PLATFORM_KEYRING=y" \
+	"platform keyring enabled"
+platform_keyring=$?
+
+kconfig_enabled "CONFIG_IMA_READ_POLICY=y" "reading IMA policy permitted"
+ima_read_policy=$?
+
+kconfig_enabled "CONFIG_KEXEC_BZIMAGE_VERIFY_SIG=y" \
+	"PE signed kernel image required"
+pe_sig_required=$?
+
+is_ima_sig_required
+ima_sig_required=$?
+
+get_secureboot_mode
+secureboot=$?
+
+if [ $secureboot -eq 0 ] && [ $arch_policy -eq 0 ] && \
+   [ $pe_sig_required -eq 0 ] && [ $ima_sig_required -eq 0 ] && \
+   [ $ima_read_policy -eq 1 ]; then
+	log_skip "No signature verification required"
+fi
+
+# Are there pe and ima signatures
+check_for_pesig
+pe_signed=$?
+
+check_for_imasig
+ima_signed=$?
+
+# Test loading the kernel image via kexec_file_load syscall
+kexec_file_load_test
diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
index 8b99017538ba..1c00fd6c4dcd 100755
--- a/tools/testing/selftests/ima/test_kexec_load.sh
+++ b/tools/testing/selftests/ima/test_kexec_load.sh
@@ -16,7 +16,6 @@ get_secureboot_mode
 secureboot=$?
 
 # kexec_load should fail in secure boot mode
-KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
 kexec --load $KERNEL_IMAGE 2>&1 > /dev/null
 if [ $? -eq 0 ]; then
 	kexec --unload
-- 
2.7.5

Re: [PATCH v2 4/5] selftests/ima: kexec_file_load syscall test

[shuah]

On 2/26/19 4:26 PM, Mimi Zohar wrote:
> 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@linux.ibm.com>
> ---
>   tools/testing/selftests/ima/Makefile               |   2 +-
>   tools/testing/selftests/ima/common_lib.sh          |  97 ++++++++++
>   .../testing/selftests/ima/test_kexec_file_load.sh  | 195 +++++++++++++++++++++
>   tools/testing/selftests/ima/test_kexec_load.sh     |   1 -
>   4 files changed, 293 insertions(+), 2 deletions(-)
>   create mode 100755 tools/testing/selftests/ima/test_kexec_file_load.sh
> 
> diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
> index 46b9e04d2737..049c83c9426c 100644
> --- a/tools/testing/selftests/ima/Makefile
> +++ b/tools/testing/selftests/ima/Makefile
> @@ -4,7 +4,7 @@ uname_M := $(shell uname -m 2>/dev/null || echo not)
>   ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
>   
>   ifeq ($(ARCH),x86)
> -TEST_PROGS := test_kexec_load.sh
> +TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh
>   TEST_FILES := common_lib.sh
>   
>   include ../lib.mk
> diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
> index c6d04006281d..24091f29bd09 100755
> --- a/tools/testing/selftests/ima/common_lib.sh
> +++ b/tools/testing/selftests/ima/common_lib.sh
> @@ -4,6 +4,9 @@
>   # Kselftest framework defines: ksft_pass=0, ksft_fail=1, ksft_skip=4
>   
>   VERBOSE="${VERBOSE:-1}"
> +IKCONFIG="/tmp/config-`uname -r`"
> +KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
> +SECURITYFS=$(grep "securityfs" /proc/mounts | awk '{print $2}')
>   
>   log_info()
>   {
> @@ -55,3 +58,97 @@ get_secureboot_mode()
>   
>   	return $ret
>   }
> +
> +# Look for config option in Kconfig file.
> +# Return 1 for found and 0 for not found.
> +kconfig_enabled()
> +{
> +	local config="$1"
> +	local msg="$2"
> +
> +        grep -E -q $config $IKCONFIG
> +        if [ $? -eq 0 ]; then
> +                log_info "$msg"
> +                return 1
> +        fi
> +        return 0
> +}
> +
> +# Attempt to get the kernel config first via proc, and then by
> +# extracting it from the kernel image or the configs.ko using
> +# scripts/extract-ikconfig.
> +# Return 1 for found and 0 for not found.
> +get_kconfig()
> +{
> +	local proc_config="/proc/config.gz"
> +	local module_dir="/lib/modules/`uname -r`"
> +	local configs_module="$module_dir/kernel/kernel/configs.ko"
> +
> +	if [ ! -f $proc_config ]; then
> +		modprobe configs > /dev/null 2>&1
> +	fi
> +	if [ -f $proc_config ]; then
> +		cat $proc_config | gunzip > $IKCONFIG 2>/dev/null
> +		if [ $? -eq 0 ]; then
> +			return 1
> +		fi
> +	fi
> +
> +	local extract_ikconfig="$module_dir/source/scripts/extract-ikconfig"
> +	if [ ! -f $extract_ikconfig ]; then
> +		log_skip "extract-ikconfig not found"
> +	fi
> +
> +	$extract_ikconfig $KERNEL_IMAGE > $IKCONFIG 2>/dev/null
> +	if [ $? -eq 1 ]; then
> +		if [ ! -f $configs_module ]; then
> +			log_skip "CONFIG_IKCONFIG not enabled"
> +		fi
> +		$extract_ikconfig $configs_module > $IKCONFIG
> +		if [ $? -eq 1 ]; then
> +			log_skip "CONFIG_IKCONFIG not enabled"
> +		fi
> +	fi
> +	return 1
> +}
> +
> +# Make sure that securityfs is mounted
> +mount_securityfs()
> +{
> +	if [ -z $SECURITYFS ]; then
> +		SECURITYFS=/sys/kernel/security
> +		mount -t securityfs security $SECURITYFS
> +	fi
> +
> +	if [ ! -d "$SECURITYFS" ]; then
> +		log_fail "$SECURITYFS :securityfs is not mounted"
> +	fi
> +}
> +
> +# The policy rule format is an "action" followed by key-value pairs.  This
> +# function supports up to two key-value pairs, in any order.
> +# For example: action func=<keyword> [appraise_type=<type>]
> +# Return 1 for found and 0 for not found.
> +check_ima_policy()
> +{
> +	local action=$1
> +	local keypair1="$2"
> +	local keypair2="$3"
> +
> +	mount_securityfs
> +
> +	local ima_policy=$SECURITYFS/ima/policy
> +	if [ ! -e $ima_policy ]; then
> +		log_fail "$ima_policy not found"
> +	fi
> +
> +	if [ -n $keypair2 ]; then
> +		grep -e "^$action.*$keypair1" "$ima_policy" | \
> +			grep -q -e "$keypair2"
> +	else
> +		grep -q -e "^$action.*$keypair1" "$ima_policy"
> +	fi
> +
> +	[ $? -eq 0 ] && ret=1 || ret=0
> +        return $ret
> +}
> diff --git a/tools/testing/selftests/ima/test_kexec_file_load.sh b/tools/testing/selftests/ima/test_kexec_file_load.sh
> new file mode 100755
> index 000000000000..e08c7e6cf28c
> --- /dev/null
> +++ b/tools/testing/selftests/ima/test_kexec_file_load.sh
> @@ -0,0 +1,195 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#

Same here

# SPDX-License-Identifier: GPL-2.0

thanks,
-- Shuah

Re: [PATCH v2 4/5] selftests/ima: kexec_file_load syscall test

[Petr Vorel]

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@linux.ibm.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

LGTM, minor comments below.
...
> +++ b/tools/testing/selftests/ima/common_lib.sh
...
> +# Look for config option in Kconfig file.
> +# Return 1 for found and 0 for not found.
> +kconfig_enabled()
> +{
> +	local config="$1"
> +	local msg="$2"
> +
Mixing tabs and spaces (spaces below).
> +        grep -E -q $config $IKCONFIG
> +        if [ $? -eq 0 ]; then
> +                log_info "$msg"
> +                return 1
> +        fi
> +        return 0
> +}
> +
> +# Attempt to get the kernel config first via proc, and then by
> +# extracting it from the kernel image or the configs.ko using
> +# scripts/extract-ikconfig.
> +# Return 1 for found and 0 for not found.
> +get_kconfig()
> +{
> +	local proc_config="/proc/config.gz"
> +	local module_dir="/lib/modules/`uname -r`"
> +	local configs_module="$module_dir/kernel/kernel/configs.ko"
> +
> +	if [ ! -f $proc_config ]; then
> +		modprobe configs > /dev/null 2>&1
> +	fi
> +	if [ -f $proc_config ]; then
> +		cat $proc_config | gunzip > $IKCONFIG 2>/dev/null
> +		if [ $? -eq 0 ]; then
> +			return 1
> +		fi
> +	fi
> +
> +	local extract_ikconfig="$module_dir/source/scripts/extract-ikconfig"
> +	if [ ! -f $extract_ikconfig ]; then
> +		log_skip "extract-ikconfig not found"
> +	fi
> +
> +	$extract_ikconfig $KERNEL_IMAGE > $IKCONFIG 2>/dev/null
> +	if [ $? -eq 1 ]; then
> +		if [ ! -f $configs_module ]; then
> +			log_skip "CONFIG_IKCONFIG not enabled"
> +		fi
> +		$extract_ikconfig $configs_module > $IKCONFIG
> +		if [ $? -eq 1 ]; then
> +			log_skip "CONFIG_IKCONFIG not enabled"
> +		fi
> +	fi
> +	return 1
> +}
> +
> +# Make sure that securityfs is mounted
> +mount_securityfs()
> +{
> +	if [ -z $SECURITYFS ]; then
> +		SECURITYFS=/sys/kernel/security
> +		mount -t securityfs security $SECURITYFS
> +	fi
> +
> +	if [ ! -d "$SECURITYFS" ]; then
> +		log_fail "$SECURITYFS :securityfs is not mounted"
		log_fail "$SECURITYFS: securityfs is not mounted"
> +	fi
> +}
> +
> +# The policy rule format is an "action" followed by key-value pairs.  This
> +# function supports up to two key-value pairs, in any order.
> +# For example: action func=<keyword> [appraise_type=<type>]
> +# Return 1 for found and 0 for not found.
> +check_ima_policy()
> +{
> +	local action=$1
	local action="$1"
(sorry this is nitpicking, I'd be consistent)
> +	local keypair1="$2"
> +	local keypair2="$3"
> +
> +	mount_securityfs
> +
> +	local ima_policy=$SECURITYFS/ima/policy
> +	if [ ! -e $ima_policy ]; then
> +		log_fail "$ima_policy not found"
> +	fi
> +
> +	if [ -n $keypair2 ]; then
> +		grep -e "^$action.*$keypair1" "$ima_policy" | \
> +			grep -q -e "$keypair2"
> +	else
> +		grep -q -e "^$action.*$keypair1" "$ima_policy"
> +	fi
> +
> +	[ $? -eq 0 ] && ret=1 || ret=0
> +        return $ret
return $? is enough here (+ ret was not defined as local and mixing tabs with spaces)
> +}
> diff --git a/tools/testing/selftests/ima/test_kexec_file_load.sh b/tools/testing/selftests/ima/test_kexec_file_load.sh
> new file mode 100755
> index 000000000000..e08c7e6cf28c
> --- /dev/null
> +++ b/tools/testing/selftests/ima/test_kexec_file_load.sh
...

> +	# The architecture specific or a custom policy may require the
> +	# kexec kernel image be signed.  Policy rules are walked
> +	# sequentially.  As a result, a policy rule may be defined, but
> +	# might not necessarily be used.  This test assumes if a policy
> +	# rule is specified, that is the intent.
> +	if [ $ima_read_policy -eq 1 ]; then
> +		check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
> +			"appraise_type=imasig"
> +		ret=$?
> +		[ $ret -eq 1 ] && log_info "IMA signature required";
> +	fi
> +	return $ret
> +}
> +
> +# The kexec_file_load_test() is complicated enough, require pesign.
> +# Return 1 for PE signature found and 0 for not found.
> +check_for_pesig()
> +{
> +	which pesign > /dev/null 2>&1
> +	if [ $?	-eq 1 ]; then
> +		log_skip "pesign not found"
> +	fi
Maybe just (matter of preference)
	which pesign > /dev/null 2>&1 || log_skip "pesign not found"
> +
> +	pesign -i $KERNEL_IMAGE --show-signature | grep -q "No signatures"
> +	local ret=$?
> +	if [ $ret -eq 1 ]; then
> +		log_info "kexec kernel image PE signed"
> +	else
> +		log_info "kexec kernel image not PE signed"
> +	fi
> +	return $ret
> +}

...
> +# kexec requires root privileges
> +if [ $(id -ru) -ne 0 ]; then
> +	log_skip "requires root privileges"
> +fi
This is repeated several times => good candidate for helper even here in IMA
specific library.

Kind regards,
Petr

Re: [PATCH v2 4/5] selftests/ima: kexec_file_load syscall test

[Mimi Zohar]

On Thu, 2019-02-28 at 23:00 +0100, Petr Vorel wrote:

> > +	local keypair1="$2"
> > +	local keypair2="$3"
> > +
> > +	mount_securityfs
> > +
> > +	local ima_policy=$SECURITYFS/ima/policy
> > +	if [ ! -e $ima_policy ]; then
> > +		log_fail "$ima_policy not found"
> > +	fi
> > +
> > +	if [ -n $keypair2 ]; then
> > +		grep -e "^$action.*$keypair1" "$ima_policy" | \
> > +			grep -q -e "$keypair2"
> > +	else
> > +		grep -q -e "^$action.*$keypair1" "$ima_policy"
> > +	fi
> > +
> > +	[ $? -eq 0 ] && ret=1 || ret=0
> > +        return $ret
> return $? is enough here (+ ret was not defined as local and mixing tabs with spaces)

"grep -q" exits with zero if any match is found.  This line inverts
the result so that 1 is returned for found.  v3 will make "ret" local
and fix the tabs/spaces.

Mimi

Re: [PATCH v2 4/5] selftests/ima: kexec_file_load syscall test

[Petr Vorel]

Hi Mimi,

> On Thu, 2019-02-28 at 23:00 +0100, Petr Vorel wrote:

> > > +	local keypair1="$2"
> > > +	local keypair2="$3"
> > > +
> > > +	mount_securityfs
> > > +
> > > +	local ima_policy=$SECURITYFS/ima/policy
> > > +	if [ ! -e $ima_policy ]; then
> > > +		log_fail "$ima_policy not found"
> > > +	fi
> > > +
> > > +	if [ -n $keypair2 ]; then
> > > +		grep -e "^$action.*$keypair1" "$ima_policy" | \
> > > +			grep -q -e "$keypair2"
> > > +	else
> > > +		grep -q -e "^$action.*$keypair1" "$ima_policy"
> > > +	fi
> > > +
> > > +	[ $? -eq 0 ] && ret=1 || ret=0
> > > +        return $ret
> > return $? is enough here (+ ret was not defined as local and mixing tabs with spaces)

> "grep -q" exits with zero if any match is found.  This line inverts
> the result so that 1 is returned for found.
Right. Sorry for wrong report :).
>  v3 will make "ret" local and fix the tabs/spaces.

> Mimi


Kind regards,
Petr

Re: [PATCH v2 4/5] selftests/ima: kexec_file_load syscall test

[Mimi Zohar]

On Mon, 2019-03-11 at 09:34 +0100, Petr Vorel wrote:
> Hi Mimi,
> 
> > On Thu, 2019-02-28 at 23:00 +0100, Petr Vorel wrote:
> 
> > > > +	local keypair1="$2"
> > > > +	local keypair2="$3"
> > > > +
> > > > +	mount_securityfs
> > > > +
> > > > +	local ima_policy=$SECURITYFS/ima/policy
> > > > +	if [ ! -e $ima_policy ]; then
> > > > +		log_fail "$ima_policy not found"
> > > > +	fi
> > > > +
> > > > +	if [ -n $keypair2 ]; then
> > > > +		grep -e "^$action.*$keypair1" "$ima_policy" | \
> > > > +			grep -q -e "$keypair2"
> > > > +	else
> > > > +		grep -q -e "^$action.*$keypair1" "$ima_policy"
> > > > +	fi
> > > > +
> > > > +	[ $? -eq 0 ] && ret=1 || ret=0
> > > > +        return $ret
> > > return $? is enough here (+ ret was not defined as local and
> mixing tabs with spaces)
> 
> > "grep -q" exits with zero if any match is found.  This line inverts
> > the result so that 1 is returned for found.
> Right. Sorry for wrong report :).

Thank you so much for reviewing the patches!  Other than deferring
making the IMA "common" functions generic, hopefully I didn't miss
anything.  I just posted the v3 version.

Mimi

> >  v3 will make "ret" local and fix the tabs/spaces.
> 
> 
> Kind regards,
> Petr
> 

Re: [PATCH v2 4/5] selftests/ima: kexec_file_load syscall test

[Petr Vorel]

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@linux.ibm.com>

> --- a/tools/testing/selftests/ima/common_lib.sh
...
> +# Look for config option in Kconfig file.
> +# Return 1 for found and 0 for not found.
I'd revert the return value (for shell is 0 as ok),
but matter of preference.
> +kconfig_enabled()
> +{
> +	local config="$1"
> +	local msg="$2"
> +
> +        grep -E -q $config $IKCONFIG
> +        if [ $? -eq 0 ]; then
> +                log_info "$msg"
> +                return 1
> +        fi
> +        return 0
> +}
> +
> +# Attempt to get the kernel config first via proc, and then by
> +# extracting it from the kernel image or the configs.ko using
> +# scripts/extract-ikconfig.
> +# Return 1 for found and 0 for not found.
"and 0 for not found": This is not true as it uses log_skip which exits.
And you don't read this value anywhere.
> +get_kconfig()
> +{
> +	local proc_config="/proc/config.gz"
> +	local module_dir="/lib/modules/`uname -r`"
> +	local configs_module="$module_dir/kernel/kernel/configs.ko"
> +
> +	if [ ! -f $proc_config ]; then
> +		modprobe configs > /dev/null 2>&1
> +	fi
> +	if [ -f $proc_config ]; then
> +		cat $proc_config | gunzip > $IKCONFIG 2>/dev/null
> +		if [ $? -eq 0 ]; then
> +			return 1
> +		fi
> +	fi
> +
> +	local extract_ikconfig="$module_dir/source/scripts/extract-ikconfig"
> +	if [ ! -f $extract_ikconfig ]; then
> +		log_skip "extract-ikconfig not found"
> +	fi
> +
> +	$extract_ikconfig $KERNEL_IMAGE > $IKCONFIG 2>/dev/null
> +	if [ $? -eq 1 ]; then
> +		if [ ! -f $configs_module ]; then
> +			log_skip "CONFIG_IKCONFIG not enabled"
> +		fi
> +		$extract_ikconfig $configs_module > $IKCONFIG
> +		if [ $? -eq 1 ]; then
> +			log_skip "CONFIG_IKCONFIG not enabled"
> +		fi
> +	fi
> +	return 1
> +}


Kind regards,
Petr

[PATCH v2 5/5] selftests/ima: loading kernel modules

[Mimi Zohar]

While the appended kernel module signature can be verified, when loading
a kernel module via either the init_module or the finit_module syscall,
verifying the IMA signature requires access to the file descriptor,
which is only available via the finit_module syscall.  As "modprobe"
does not provide a flag allowing the syscall - init_module or
finit_module - to be specified, this patch does not load a kernel
module.

This test simply verifies that on secure boot enabled systems with
"CONFIG_IMA_ARCH_POLICY" configured, that at least an appended kernel
module signature or an IMA signature is required based on the Kconfig
and the runtime IMA policy.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 tools/testing/selftests/ima/Makefile              |  2 +-
 tools/testing/selftests/ima/test_kernel_module.sh | 96 +++++++++++++++++++++++
 2 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/ima/test_kernel_module.sh

diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
index 049c83c9426c..ef5201ff0bea 100644
--- a/tools/testing/selftests/ima/Makefile
+++ b/tools/testing/selftests/ima/Makefile
@@ -4,7 +4,7 @@ uname_M := $(shell uname -m 2>/dev/null || echo not)
 ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
 
 ifeq ($(ARCH),x86)
-TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh
+TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh test_kernel_module.sh
 TEST_FILES := common_lib.sh
 
 include ../lib.mk
diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
new file mode 100755
index 000000000000..4009e1b60b03
--- /dev/null
+++ b/tools/testing/selftests/ima/test_kernel_module.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# On secure boot enabled systems with "CONFIG_IMA_ARCH_POLICY" configured,
+# this test verifies that at least an appended kernel module signature or
+# an IMA signature is required.  It does not attempt to load a kernel module.
+
+TEST="KERNEL_MODULE"
+. ./common_lib.sh
+
+trap "{ rm -f $IKCONFIG ; }" EXIT
+
+# Some of the IMA builtin policies may require the kernel modules to
+# be signed, but these policy rules may be replaced with a custom
+# policy.  Only CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS persists after
+# loading a custom policy.  Check if it is enabled, before reading the
+# IMA runtime sysfs policy file.
+# Return 1 for IMA signature required and 0 for not required.
+is_ima_sig_required()
+{
+	local ret=0
+
+	kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS=y" \
+		"IMA kernel module signature required"
+	if [ $? -eq 1 ]; then
+		log_info "IMA kernel module signature required"
+		return 1
+	fi
+
+	# The architecture specific or a custom policy may require the
+	# kernel module to be signed.  Policy rules are walked sequentially.
+	# As a result, a policy rule may be defined, but might not necessarily
+	# be used.  This test assumes if a policy rule is specified, that is
+	# the intent.
+	if [ $ima_read_policy -eq 1 ]; then
+		check_ima_policy "appraise" "func=MODULE_CHECK" \
+			"appraise_type=imasig"
+		ret=$?
+		[ $ret -eq 1 ] && log_info "IMA signature required";
+	fi
+	return $ret
+}
+
+# loading kernel modules requires root privileges
+if [ $(id -ru) -ne 0 ]; then
+	log_skip "requires root privileges"
+fi
+
+# Are appended signatures required?
+if [ -e /sys/module/module/parameters/sig_enforce ]; then
+	sig_enforce=$(cat /sys/module/module/parameters/sig_enforce)
+	if [ $sig_enforce = "Y" ]; then
+		log_pass "appended kernel module signature required"
+	fi
+fi
+
+get_secureboot_mode
+if [ $? -eq 0 ]; then
+	log_skip "secure boot not enabled"
+fi
+
+# get the kernel config
+get_kconfig
+
+# Determine which kernel config options are enabled
+kconfig_enabled "CONFIG_IMA_ARCH_POLICY=y" \
+	"architecture specific policy enabled"
+arch_policy=$?
+
+kconfig_enabled "CONFIG_MODULE_SIG=y" \
+	"appended kernel modules signature enabled"
+appended_sig_enabled=$?
+
+kconfig_enabled "CONFIG_IMA_READ_POLICY=y" "reading IMA policy permitted"
+ima_read_policy=$?
+
+is_ima_sig_required
+ima_sig_required=$?
+
+if [ $arch_policy -eq 0 ]; then
+	log_skip "architecture specific policy not enabled"
+fi
+
+if [ $appended_sig_enabled -eq 1 ]; then
+	log_fail "appended kernel module signature enabled, but not required"
+fi
+
+if [ $ima_sig_required -eq 1 ]; then
+	log_pass "IMA kernel module signature required"
+fi
+
+if [ $ima_read_policy -eq 1 ]; then
+	log_fail "IMA kernel module signature not required"
+else
+	log_skip "reading IMA policy not permitted"
+fi
-- 
2.7.5

Re: [PATCH v2 5/5] selftests/ima: loading kernel modules

[shuah]

On 2/26/19 4:27 PM, Mimi Zohar wrote:
> While the appended kernel module signature can be verified, when loading
> a kernel module via either the init_module or the finit_module syscall,
> verifying the IMA signature requires access to the file descriptor,
> which is only available via the finit_module syscall.  As "modprobe"
> does not provide a flag allowing the syscall - init_module or
> finit_module - to be specified, this patch does not load a kernel
> module.
> 
> This test simply verifies that on secure boot enabled systems with
> "CONFIG_IMA_ARCH_POLICY" configured, that at least an appended kernel
> module signature or an IMA signature is required based on the Kconfig
> and the runtime IMA policy.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   tools/testing/selftests/ima/Makefile              |  2 +-
>   tools/testing/selftests/ima/test_kernel_module.sh | 96 +++++++++++++++++++++++
>   2 files changed, 97 insertions(+), 1 deletion(-)
>   create mode 100755 tools/testing/selftests/ima/test_kernel_module.sh
> 
> diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
> index 049c83c9426c..ef5201ff0bea 100644
> --- a/tools/testing/selftests/ima/Makefile
> +++ b/tools/testing/selftests/ima/Makefile
> @@ -4,7 +4,7 @@ uname_M := $(shell uname -m 2>/dev/null || echo not)
>   ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
>   
>   ifeq ($(ARCH),x86)
> -TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh
> +TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh test_kernel_module.sh
>   TEST_FILES := common_lib.sh
>   
>   include ../lib.mk
> diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
> new file mode 100755
> index 000000000000..4009e1b60b03
> --- /dev/null
> +++ b/tools/testing/selftests/ima/test_kernel_module.sh
> @@ -0,0 +1,96 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later

Same here

# SPDX-License-Identifier: GPL-2.0

> +#
> +# On secure boot enabled systems with "CONFIG_IMA_ARCH_POLICY" configured,
> +# this test verifies that at least an appended kernel module signature or
> +# an IMA signature is required.  It does not attempt to load a kernel module.
> +
> +TEST="KERNEL_MODULE"
> +. ./common_lib.sh
> +
> +trap "{ rm -f $IKCONFIG ; }" EXIT
> +
> +# Some of the IMA builtin policies may require the kernel modules to
> +# be signed, but these policy rules may be replaced with a custom
> +# policy.  Only CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS persists after
> +# loading a custom policy.  Check if it is enabled, before reading the
> +# IMA runtime sysfs policy file.
> +# Return 1 for IMA signature required and 0 for not required.
> +is_ima_sig_required()
> +{
> +	local ret=0
> +
> +	kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS=y" \
> +		"IMA kernel module signature required"
> +	if [ $? -eq 1 ]; then
> +		log_info "IMA kernel module signature required"
> +		return 1
> +	fi
> +
> +	# The architecture specific or a custom policy may require the
> +	# kernel module to be signed.  Policy rules are walked sequentially.
> +	# As a result, a policy rule may be defined, but might not necessarily
> +	# be used.  This test assumes if a policy rule is specified, that is
> +	# the intent.
> +	if [ $ima_read_policy -eq 1 ]; then
> +		check_ima_policy "appraise" "func=MODULE_CHECK" \
> +			"appraise_type=imasig"
> +		ret=$?
> +		[ $ret -eq 1 ] && log_info "IMA signature required";
> +	fi
> +	return $ret
> +}
> +
> +# loading kernel modules requires root privileges
> +if [ $(id -ru) -ne 0 ]; then
> +	log_skip "requires root privileges"
> +fi
> +
> +# Are appended signatures required?
> +if [ -e /sys/module/module/parameters/sig_enforce ]; then
> +	sig_enforce=$(cat /sys/module/module/parameters/sig_enforce)
> +	if [ $sig_enforce = "Y" ]; then
> +		log_pass "appended kernel module signature required"
> +	fi
> +fi
> +
> +get_secureboot_mode
> +if [ $? -eq 0 ]; then
> +	log_skip "secure boot not enabled"
> +fi
> +
> +# get the kernel config
> +get_kconfig
> +

get_kconfig() will be good candidate as a kselftest common
function. Is that possible?

> +# Determine which kernel config options are enabled
> +kconfig_enabled "CONFIG_IMA_ARCH_POLICY=y" \
> +	"architecture specific policy enabled"
> +arch_policy=$?
> +
> +kconfig_enabled "CONFIG_MODULE_SIG=y" \
> +	"appended kernel modules signature enabled"
> +appended_sig_enabled=$?
> +
> +kconfig_enabled "CONFIG_IMA_READ_POLICY=y" "reading IMA policy permitted"
> +ima_read_policy=$?
> +
> +is_ima_sig_required
> +ima_sig_required=$?
> +
> +if [ $arch_policy -eq 0 ]; then
> +	log_skip "architecture specific policy not enabled"
> +fi
> +
> +if [ $appended_sig_enabled -eq 1 ]; then
> +	log_fail "appended kernel module signature enabled, but not required"
> +fi
> +
> +if [ $ima_sig_required -eq 1 ]; then
> +	log_pass "IMA kernel module signature required"
> +fi
> +
> +if [ $ima_read_policy -eq 1 ]; then
> +	log_fail "IMA kernel module signature not required"
> +else
> +	log_skip "reading IMA policy not permitted"
> +fi
> 

thanks,
-- Shuah

Re: [PATCH v2 5/5] selftests/ima: loading kernel modules

[Mimi Zohar]

Hi Shuah,

> > diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
> > new file mode 100755
> > index 000000000000..4009e1b60b03
> > --- /dev/null
> > +++ b/tools/testing/selftests/ima/test_kernel_module.sh
> > @@ -0,0 +1,96 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> 
> Same here
> 
> # SPDX-License-Identifier: GPL-2.0

Sure, I'll make the change here and in the other places.


> > +get_secureboot_mode
> > +if [ $? -eq 0 ]; then
> > +	log_skip "secure boot not enabled"
> > +fi
> > +
> > +# get the kernel config
> > +get_kconfig
> > +
> 
> get_kconfig() will be good candidate as a kselftest common
> function. Is that possible?

Sure, where would it go?  get_kconfig calls log_skip.  I didn't see
any common logging functions.  Petr suggested defining a set of common
logging functions.  Did you want to only make "log_skip" a common
function or the other logging functions log_pass, log_fail, log_info
as well?

Thanks,

Mimi

Re: [PATCH v2 5/5] selftests/ima: loading kernel modules

[shuah]

Hi Mimi,

On 2/27/19 7:14 AM, Mimi Zohar wrote:
> Hi Shuah,
> 
>>> diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
>>> new file mode 100755
>>> index 000000000000..4009e1b60b03
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/ima/test_kernel_module.sh
>>> @@ -0,0 +1,96 @@
>>> +#!/bin/sh
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>
>> Same here
>>
>> # SPDX-License-Identifier: GPL-2.0
> 
> Sure, I'll make the change here and in the other places.
> 
> 

Thanks.

>>> +get_secureboot_mode
>>> +if [ $? -eq 0 ]; then
>>> +	log_skip "secure boot not enabled"
>>> +fi
>>> +
>>> +# get the kernel config
>>> +get_kconfig
>>> +
>>
>> get_kconfig() will be good candidate as a kselftest common
>> function. Is that possible?
> 
> Sure, where would it go?  get_kconfig calls log_skip.  I didn't see
> any common logging functions.  Petr suggested defining a set of common
> logging functions.  Did you want to only make "log_skip" a common
> function or the other logging functions log_pass, log_fail, log_info
> as well?
> 

We can do this as a separate effort in the interest of getting these
in the interest of getting these in.

We have common functions in ksefltest.h for c and we don't have them
for tests scripts. We might be able to collect common routines such
as get_kconfig into a common .sh and include in tests. If you have time
to do this, that will be great. It can be done as a separate effort.

thanks,
-- Shuah

Re: [PATCH v2 5/5] selftests/ima: loading kernel modules

[Mimi Zohar]

> >>> +get_secureboot_mode
> >>> +if [ $? -eq 0 ]; then
> >>> +	log_skip "secure boot not enabled"
> >>> +fi
> >>> +
> >>> +# get the kernel config
> >>> +get_kconfig
> >>> +
> >>
> >> get_kconfig() will be good candidate as a kselftest common
> >> function. Is that possible?
> > 
> > Sure, where would it go?  get_kconfig calls log_skip.  I didn't see
> > any common logging functions.  Petr suggested defining a set of common
> > logging functions.  Did you want to only make "log_skip" a common
> > function or the other logging functions log_pass, log_fail, log_info
> > as well?
> > 
> 
> We can do this as a separate effort in the interest of getting these
> in the interest of getting these in.
> 
> We have common functions in ksefltest.h for c and we don't have them
> for tests scripts. We might be able to collect common routines such
> as get_kconfig into a common .sh and include in tests. If you have time
> to do this, that will be great. It can be done as a separate effort.

Ok.  For now, I'm waiting for some review/ack's.

Mimi

Re: [PATCH v2 5/5] selftests/ima: loading kernel modules

[Petr Vorel]

Hi Shuah,

> We can do this as a separate effort in the interest of getting these
> in the interest of getting these in.

> We have common functions in ksefltest.h for c and we don't have them
> for tests scripts. We might be able to collect common routines such
> as get_kconfig into a common .sh and include in tests. If you have time
> to do this, that will be great. It can be done as a separate effort.
Some inspiration, what sort of helpers can be for shell see LTP shell API [1].
These helpers tests help tests to be short, readable and with unified output.
(I like library variables [2] which are self describing and making test even
shorter), setup and cleanup functions called automatically, ...
The same applies for C API [3] (e.g. SAFE_*() helpers for handling errors, ...).

I wonder if there is even any interest of having any sort of helpers or even
framework for shell, when so far there was nothing.

> thanks,
> -- Shuah


Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell
[2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#232-library-variables
[3] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c

Re: [PATCH v2 5/5] selftests/ima: loading kernel modules

[Petr Vorel]

Hi Mimi,

> While the appended kernel module signature can be verified, when loading
> a kernel module via either the init_module or the finit_module syscall,
> verifying the IMA signature requires access to the file descriptor,
> which is only available via the finit_module syscall.  As "modprobe"
> does not provide a flag allowing the syscall - init_module or
> finit_module - to be specified, this patch does not load a kernel
> module.

> This test simply verifies that on secure boot enabled systems with
> "CONFIG_IMA_ARCH_POLICY" configured, that at least an appended kernel
> module signature or an IMA signature is required based on the Kconfig
> and the runtime IMA policy.

> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
...
> +# Are appended signatures required?
> +if [ -e /sys/module/module/parameters/sig_enforce ]; then
> +	sig_enforce=$(cat /sys/module/module/parameters/sig_enforce)
> +	if [ $sig_enforce = "Y" ]; then
> +		log_pass "appended kernel module signature required"
> +	fi
> +fi
Another possible helper [1]:
is_enabled() # or sysfs_enabled
{
	[ -f "$1" ] && [ "$(cat $1)" = "Y" -o "$(cat $1)" = "1" ]
}

is_enabled /sys/module/module/parameters/sig_enforce && \
	log_pass "appended kernel module signature required"

...


Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/blob/master/ver_linux#L30

Re: [PATCH v2 5/5] selftests/ima: loading kernel modules

[Mimi Zohar]

On Thu, 2019-02-28 at 23:32 +0100, Petr Vorel wrote:
> Hi Mimi,
> 
> > While the appended kernel module signature can be verified, when loading
> > a kernel module via either the init_module or the finit_module syscall,
> > verifying the IMA signature requires access to the file descriptor,
> > which is only available via the finit_module syscall.  As "modprobe"
> > does not provide a flag allowing the syscall - init_module or
> > finit_module - to be specified, this patch does not load a kernel
> > module.
> 
> > This test simply verifies that on secure boot enabled systems with
> > "CONFIG_IMA_ARCH_POLICY" configured, that at least an appended kernel
> > module signature or an IMA signature is required based on the Kconfig
> > and the runtime IMA policy.
> 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> ...
> > diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
> ...
> > +# Are appended signatures required?
> > +if [ -e /sys/module/module/parameters/sig_enforce ]; then
> > +	sig_enforce=$(cat /sys/module/module/parameters/sig_enforce)
> > +	if [ $sig_enforce = "Y" ]; then
> > +		log_pass "appended kernel module signature required"
> > +	fi
> > +fi
> Another possible helper [1]:
> is_enabled() # or sysfs_enabled
> {
> 	[ -f "$1" ] && [ "$(cat $1)" = "Y" -o "$(cat $1)" = "1" ]
> }
> 
> is_enabled /sys/module/module/parameters/sig_enforce && 
> 	log_pass "appended kernel module signature required"
> 

Agreed.  As this is being used only here in the IMA selftests,
deferring making this change until there is a generic common library.

Mimi