Re: [PATCH RFC 4/4] selftests/ipe: test the IPE policy keyring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/ipe: missing CONFIG_CRYPTO_SHA256 in config fragment
- [Low] selftests/ipe: keyctl id check precedes already-provisioned keyring check
--

commit 0116188a11e090ec7a12ebe17082312fda668a18
Author: Manuel Huber <manuelh@nvidia.com>

selftests/ipe: test the IPE policy keyring

This commit adds a kselftest script and configuration fragment to validate
IPE policy keyring operations for both sealed and unsealed boot parameter
states.

> diff --git a/tools/testing/selftests/ipe/config b/tools/testing/selftests/ipe/config
> new file mode 100644
> index 0000000000000..498c224d2c182
> --- /dev/null
> +++ b/tools/testing/selftests/ipe/config
> @@ -0,0 +1,7 @@
> +CONFIG_AUDIT=y
> +CONFIG_AUDITSYSCALL=y
> +CONFIG_KEYS=y
> +CONFIG_PROC_FS=y
> +CONFIG_SECURITY=y
> +CONFIG_SECURITYFS=y
> +CONFIG_SECURITY_IPE=y

[Severity: Medium]
Should this config fragment also include CONFIG_CRYPTO_SHA256=y (along with
CONFIG_NET=y and CONFIG_SYSFS=y, which CONFIG_AUDIT and CONFIG_SECURITY
depend on)?

In test-ipe-keyring.sh, generate_keypair() and sign_policy() generate
RSA-2048 X.509 certificates and PKCS#7 signatures using SHA-256. When a
signed policy is written to /sys/kernel/security/ipe/new_policy,
new_policy() -> ipe_new_policy() -> verify_pkcs7_signature() ->
pkcs7_verify() -> pkcs7_digest() allocates the shash algorithm:

crypto/asymmetric_keys/pkcs7_verify.c:pkcs7_digest() {
	...
	tfm = crypto_alloc_shash(sinfo->sig->hash_algo, 0, 0);
	if (IS_ERR(tfm))
		return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
	...
}

Although CONFIG_SECURITY_IPE selects CONFIG_CRYPTO_LIB_SHA256,
CONFIG_PKCS7_MESSAGE_PARSER, and CONFIG_SYSTEM_DATA_VERIFICATION, none of
those select CONFIG_CRYPTO_SHA256 to register the "sha256" crypto_shash
algorithm.

Without CONFIG_CRYPTO_SHA256=y, pkcs7_digest() returns -ENOPKG. That causes
positive policy loads in test_unsealed_keyring() to fail, and causes the
negative checks in test_sealed_keyring() and test_unsealed_keyring() to
falsely pass before ipe_new_policy() ever reaches ipe_keyring_lookup():

security/ipe/policy.c:ipe_new_policy() {
	...
		if (rc == -ENOKEY || rc == -EKEYREJECTED) {
			keyring = ipe_keyring_lookup();
			if (keyring)
				rc = verify_pkcs7_signature(NULL, 0, new->pkcs7,
							    pkcs7len, keyring,
							    VERIFYING_UNSPECIFIED_SIGNATURE,
							    set_pkcs7_data, new);
		}
	...
}

> diff --git a/tools/testing/selftests/ipe/test-ipe-keyring.sh b/tools/testing/selftests/ipe/test-ipe-keyring.sh
> new file mode 100755
> index 0000000000000..557ccc825fe24
> --- /dev/null
> +++ b/tools/testing/selftests/ipe/test-ipe-keyring.sh

[ ... ]

> +	keyctl setperm "$KEY_SPEC_IPE_KEYRING" \
> +		"$IPE_KEYRING_PERM_LOCKED" >/dev/null 2>&1 ||
> +		fail "drop mutation permissions from the keyring"
> +	ktap_test_pass "mutation permissions can be dropped"
> +
> +	if keyctl id "$KEY_SPEC_IPE_KEYRING" >/dev/null 2>&1; then
> +		fail "the special key ID granted possession rights"
> +	fi
> +	ktap_test_pass "the special key ID grants no possession rights"

[ ... ]

> +keyring_serial=$(keyctl id "$KEY_SPEC_IPE_KEYRING" 2>/dev/null) ||
> +	skip "KEY_SPEC_IPE_KEYRING does not resolve"
> +keyring_description=$(keyctl describe "$keyring_serial" 2>/dev/null) ||
> +	skip "cannot describe the IPE keyring"
> +case "$keyring_description" in
> +	*"keyring: .ipe") ;;
> +	*) skip "KEY_SPEC_IPE_KEYRING resolved to an unexpected key" ;;
> +esac
> +
> +keyring_unsealed=$(cat /sys/module/ipe/parameters/keyring_unsealed)
> +case "$keyring_unsealed" in
> +	Y)
> +		[ "$(keyring_key_count)" = 0 ] ||
> +			skip "the IPE keyring has already been provisioned"
> +		test_unsealed_keyring
> +		;;

[Severity: Low]
Can the "the IPE keyring has already been provisioned" skip check be
reached if the keyring was already provisioned and locked?

When the keyring is locked with IPE_KEYRING_PERM_LOCKED (0x08030000),
KEY_USR_SEARCH is dropped. Because lookup_user_key() returns an unpossessed
key reference for KEY_SPEC_IPE_KEYRING, running keyctl id on
KEY_SPEC_IPE_KEYRING fails with -EACCES once locked (as verified in
test_unsealed_keyring() above).

When test-ipe-keyring.sh is run a second time or on a system where the
keyring was already provisioned and locked, the initial keyctl id check at
the top of the script will fail and skip with "KEY_SPEC_IPE_KEYRING does
not resolve" before reaching the keyring_key_count() check (which uses
keyctl list and only needs read/view permissions). Should the
keyring_key_count() check happen before calling keyctl id when
keyring_unsealed is Y?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-b4-ipe-keyring-rfc-v1-0-156f0b93e843@nvidia.com?part=4