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 ++++++++++++++++++++++++++++++++++
 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
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");
+}
+
+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;
+	}
+
+	return 0;
+}
+
+static struct kunit_case kpkeys_hardened_cred_test_cases[] = {
+	KUNIT_CASE(write_cred),
+	{}
+};
+
+static struct kunit_suite kpkeys_hardened_cred_test_suite = {
+	.name = "Hardened credentials using kpkeys",
+	.test_cases = kpkeys_hardened_cred_test_cases,
+	.suite_init = kpkeys_hardened_cred_suite_init,
+};
+kunit_test_suite(kpkeys_hardened_cred_test_suite);
+
+MODULE_DESCRIPTION("Tests for the kpkeys_hardened_cred feature");
+MODULE_LICENSE("GPL");
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index 1af3a9dae645..9b0563a03ab4 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -338,6 +338,17 @@ config KPKEYS_HARDENED_CRED
 	  This option has no effect if the system does not support
 	  kernel pkeys.
 
+config KPKEYS_HARDENED_CRED_TEST
+	tristate "KUnit tests for kpkeys_hardened_cred" if !KUNIT_ALL_TESTS
+	depends on KPKEYS_HARDENED_CRED
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  Enable this option to check that the kpkeys_hardened_cred feature
+	  functions as intended, i.e. prevents arbitrary writes to live credentials.
+
+	  If unsure, say N.
+
 endmenu
 
 config CC_HAS_RANDSTRUCT
-- 
2.47.0

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

On 07/02/2025 05:52, Kees Cook wrote:
> 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

I wasn't aware of those guidelines, thanks for the pointer! I got
inspiration from various existing tests, it unfortunately looks like the
conventions in [1] have not been universally adopted. I'll try to follow
them in the next version (of both RFC series).

> [...]
>
> +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.)

That's a fair point, I've actually run into this sort of issues with the
page table tests (in the other RFC series). I can add positive tests
with a regular read (e.g. reading current->cred->uid directly) - no
fault is expected to occur in that case.

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

prepare_creds() does not allocate protected memory, see the introduction
in the cover letter and patch 6. However I could certainly add such
tests for the new helpers protect_creds() and prepare_protected_creds(),
which are meant to be used with override_creds().

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

kasan_suite_init() uses this approach if KASAN is disabled, but skipping
does seem to be a better idea - this way it doesn't show up as an error.

> 	if (!arch_kpkeys_enabled())
> 		kunit_skip(test, "kpkeys are not supported\n");
>
> I'm very happy to see tests! :)

Thank you for the review and suggestions!

- Kevin