linux-hardening - Re: [RFC PATCH 8/8] mm: Add basic tests for kpkeys_hardened

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202502062024.BCB0DED1D5@keescook>
Date: Thu, 6 Feb 2025 20:52:32 -0800
From: Kees Cook <kees@...nel.org>
To: Kevin Brodsky <kevin.brodsky@....com>
Cc: linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mark Brown <broonie@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	David Howells <dhowells@...hat.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Jann Horn <jannh@...gle.com>, Jeff Xu <jeffxu@...omium.org>,
	Joey Gouly <joey.gouly@....com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Andy Lutomirski <luto@...nel.org>, Marc Zyngier <maz@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Pierre Langlois <pierre.langlois@....com>,
	Quentin Perret <qperret@...gle.com>,
	"Mike Rapoport (IBM)" <rppt@...nel.org>,
	Ryan Roberts <ryan.roberts@....com>,
	Thomas Gleixner <tglx@...utronix.de>, Will Deacon <will@...nel.org>,
	Matthew Wilcox <willy@...radead.org>,
	Qi Zheng <zhengqi.arch@...edance.com>,
	linux-arm-kernel@...ts.infradead.org, linux-mm@...ck.org,
	x86@...nel.org
Subject: Re: [RFC PATCH 8/8] mm: Add basic tests for kpkeys_hardened_cred

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