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

[Kevin Brodsky <kevin.brodsky@arm.com>]

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