Re: [RFC PATCH 8/8] mm: Add basic tests for kpkeys_hardened_cred

[Kees Cook <kees@kernel.org>]

On Mon, Feb 03, 2025 at 10:28:09AM +0000, Kevin Brodsky wrote:
> Add basic tests for the kpkeys_hardened_pgtables feature: try to
> perform a direct write to current->{cred,real_cred} and ensure it
> fails.
> 
> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
> ---
>  mm/Makefile                    |  1 +
>  mm/kpkeys_hardened_cred_test.c | 42 ++++++++++++++++++++++++++++++++++

Current file naming convention[1] would be to name this as:

	mm/tests/kpkeys_hardened_cred_kunit.c

>  security/Kconfig.hardening     | 11 +++++++++
>  3 files changed, 54 insertions(+)
>  create mode 100644 mm/kpkeys_hardened_cred_test.c
> 
> diff --git a/mm/Makefile b/mm/Makefile
> index f7263b7f45b8..2024226902d4 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -149,3 +149,4 @@ obj-$(CONFIG_TMPFS_QUOTA) += shmem_quota.o
>  obj-$(CONFIG_PT_RECLAIM) += pt_reclaim.o
>  obj-$(CONFIG_KPKEYS_HARDENED_PGTABLES) += kpkeys_hardened_pgtables.o
>  obj-$(CONFIG_KPKEYS_HARDENED_PGTABLES_TEST) += kpkeys_hardened_pgtables_test.o
> +obj-$(CONFIG_KPKEYS_HARDENED_CRED_TEST) += kpkeys_hardened_cred_test.o

And for the Kconfig convention says[2] this should be:

	CONFIG_KPKEYS_HARDENED_CRED_KUNIT_TEST

> diff --git a/mm/kpkeys_hardened_cred_test.c b/mm/kpkeys_hardened_cred_test.c
> new file mode 100644
> index 000000000000..46048098f99d
> --- /dev/null
> +++ b/mm/kpkeys_hardened_cred_test.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <kunit/test.h>
> +#include <linux/sched.h>
> +
> +static void write_cred(struct kunit *test)
> +{
> +	long zero = 0;
> +	int ret;
> +
> +	ret = copy_to_kernel_nofault((unsigned long *)current->cred, &zero, sizeof(zero));
> +	KUNIT_EXPECT_EQ_MSG(test, ret, -EFAULT,
> +			    "Write to current->cred wasn't prevented");
> +
> +	ret = copy_to_kernel_nofault((unsigned long *)current->real_cred, &zero, sizeof(zero));
> +	KUNIT_EXPECT_EQ_MSG(test, ret, -EFAULT,
> +			    "Write to current->real_cred wasn't prevented");

This is a good negative test. I would include a positive test as well.
i.e. make sure you can run copy_from_kernel_nofault() to read it
successfully. Otherwise you don't know if you're just getting a bad
address -- we want to distinguish between them. (This is more true for
the next suggestion, since current->cred being broken would be much more
obvious.)

While current->cred is good and easy, I would like to see prepare_creds()
exercised too to get a new cred and validate that it is equally directly
readable and directly not writable, and then use the correct accessors
to perform a successful write to the cred, read back the change,
etc. (i.e. validate the expected behavior too.)

> +}
> +
> +static int kpkeys_hardened_cred_suite_init(struct kunit_suite *suite)
> +{
> +	if (!arch_kpkeys_enabled()) {
> +		pr_err("Cannot run kpkeys_hardened_cred tests: kpkeys are not supported\n");
> +		return 1;
> +	}

Instead of failing ("return 1") I think this should be a "skip" (it is
expected to not work if there is no support) in each test instead:

	if (!arch_kpkeys_enabled())
		kunit_skip(test, "kpkeys are not supported\n");

I'm very happy to see tests! :)

-Kees

[1] https://docs.kernel.org/dev-tools/kunit/style.html#test-file-and-module-names
[2] https://docs.kernel.org/dev-tools/kunit/style.html#test-kconfig-entries

-- 
Kees Cook