Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 3/4] kselftests/arm64: add PAuth test for whether
 exec() changes keys
To:     Dave Martin <Dave.Martin@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        Will Deacon <will@kernel.org>, boian4o1@gmail.com,
        Catalin Marinas <catalin.marinas@arm.com>,
        amit.kachhap@arm.com, vincenzo.frascino@arm.com,
        Shuah Khan <shuah@kernel.org>
References: <20200831110450.30188-1-boyan.karatotev@arm.com>
 <20200831110450.30188-4-boyan.karatotev@arm.com>
 <20200902170854.GK6642@arm.com>
From:   Boyan Karatotev <boyan.karatotev@arm.com>
Message-ID: <926691e4-1990-207e-bcb9-40ab6d3b0fa0@arm.com>
Date:   Thu, 3 Sep 2020 11:48:37 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200902170854.GK6642@arm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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