Re: [PATCH v2 3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

[Boyan Karatotev <boyan.karatotev@arm.com>]

On 02/09/2020 18:08, Dave Martin wrote:
> On Mon, Aug 31, 2020 at 12:04:49PM +0100, Boyan Karatotev wrote:
>> +/*
>> + * fork() does not change keys. Only exec() does so call a worker program.
>> + * Its only job is to sign a value and report back the resutls
>> + */
>> +TEST(exec_unique_keys)
>> +{
> 
> The kernel doesn't guarantee that keys are unique.
> 
> Can we present all the "unique keys" wording differently, say
> 
> 	exec_key_collision_likely()

I agree that this test's name is a bit out of place. I would rather have
it named "exec_changed_keys" though.

> Otherwise people might infer from this test code that the keys are
> supposed to be truly unique and start reporting bugs on the kernel.
> 
> I can't see an obvious security argument for unique keys (rather, the
> keys just need to be "unique enough".  That's the job of
> get_random_bytes().)

The "exec_unique_keys" test only checks that the keys changed after an
exec() which I think the name change would reflect.

The thing with the "single_thread_unique_keys" test is that the kernel
says the the keys will be random. Yes, there is no uniqueness guarantee
but I'm not sure how to phrase it differently. There is some minuscule
chance that the keys end up the same, but for this test I pretend this
will not happen. Would changing up the comments and the failure message
communicate this? Maybe substitute "unique" for "different" and say how
many keys clashed?

-- 
Regards,
Boyan