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@xxxxxxx>
> ---
> 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
