Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v5] arm64: Implement archrandom.h for ARMv8.5-RNG
To:     Mark Rutland <mark.rutland@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-crypto@vger.kernel.org,
        ard.biesheuvel@linaro.org
References: <20191108135751.3218-1-rth@twiddle.net>
 <20191108143025.GD11465@lakrids.cambridge.arm.com>
From:   Richard Henderson <richard.henderson@linaro.org>
Openpgp: preference=signencrypt
Message-ID: <846ba15f-b777-c0c9-6720-32b96d6c45ed@linaro.org>
Date:   Sat, 9 Nov 2019 10:04:28 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20191108143025.GD11465@lakrids.cambridge.arm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On 11/8/19 3:30 PM, Mark Rutland wrote:
> On Fri, Nov 08, 2019 at 02:57:51PM +0100, Richard Henderson wrote:
>> From: Richard Henderson <richard.henderson@linaro.org>
>>
>> Expose the ID_AA64ISAR0.RNDR field to userspace, as the
>> RNG system registers are always available at EL0.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> v2: Use __mrs_s and fix missing cc clobber (Mark),
>>     Log rng failures with pr_warn (Mark),
> 
> When I suggested this, I meant in the probe path.
> 
> Since it can legitimately fail at runtime, I don't think it's worth
> logging there. Maybe it's worth recording stats, but the generic wrapper
> could do that.

Ah, ok, dropped.

>> +#ifdef CONFIG_ARCH_RANDOM
>> +	{
>> +		.desc = "Random Number Generator",
>> +		.capability = ARM64_HAS_RNG,
>> +		.type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE,
> 
> As above, if we're advertisting this to userspace and/or VMs, this must
> be a system-wide feature, and cannot be a weak local feature.

Could you draw me the link between struct arm64_cpu_capabilities, as seen here,
and struct arm64_ftr_bits, which exposes the system registers to userspace/vms?

AFAICS, ARM64_HAS_RNG is private to the kernel; there is no ELF HWCAP value
exposed to userspace by this.

The adjustment of ID_AA64ISAR0.RNDR is FTR_LOWER_SAFE, which means the minimum
value of all online cpus.  (Which seems to generate a pr_warn in
check_update_ftr_reg for hot-plug secondaries that do not match.)


> We don't bother with special-casing local handling mismatch like this
> for other features. I'd ratehr that:
> 
> * On the boot CPU, prior to detecting secondaries, we can seed the usual
>   pool with the RNG if the boot CPU has it.
> 
> * Once secondaries are up, if the feature is present system-wide, we can
>   make use of the feature as a system-wide feature. If not, we don't use
>   the RNG.

Unless I'm mis-reading things, there is not a setting for ARM64_CPUCAP_* that
allows exactly this.  If I use ARM64_CPUCAP_SYSTEM_FEATURE, then the feature is
not detected early enough for the boot cpu.

I can change this to ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE.  That way it is
system-wide, and also detected early enough to be used for rand_initialize().
However, it has the side effect that secondaries are not allowed to omit RNG if
the boot cpu has RNG.

Is there some setting that I've missed?  Is it ok to kick the problem down the
road until someone actually builds mis-matched hardware?


> ... so this can be:
> 
> bool arch_get_random_long(unsigned long *v)
> {
> 	bool ok;
> 
> 	if (!cpus_have_const_cap(ARM64_HAS_RNG))
> 		return false;
> 
> 	/*
> 	 * Reads of RNDR set PSTATE.NZCV to 0b0000 on success,
> 	 * and set PSTATE.NZCV to 0b0100 otherwise.
> 	 */
> 	asm volatile(
> 		__mrs_s("%0", SYS_RNDR_EL0) "\n"
> 	"	cset %w1, ne\n"
> 	: "=r" (*v), "=r" (ok)
> 	:
> 	: "cc");
> 
> 	return ok;
> }
> 
> ...with similar for arch_get_random_seed_long().

Done.

>>  config RANDOM_TRUST_CPU
>>  	bool "Trust the CPU manufacturer to initialize Linux's CRNG"
>> -	depends on X86 || S390 || PPC
>> +	depends on X86 || S390 || PPC || ARM64
> 
> Can't that depend on ARCH_RANDOM instead?

Yes, it can.


r~