linux-kernel - 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: <d009306c-eb1c-48ca-ab8e-d891b413d55f@arm.com>
Date: Tue, 11 Feb 2025 09:58:25 +0100
From: Kevin Brodsky <kevin.brodsky@....com>
To: Kees Cook <kees@...nel.org>
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 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@....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