Re: [PATCH] random: add rng-seed= command line option

[Mark Salyzyn <salyzyn@android.com>]

On 2/7/20 7:58 AM, Theodore Y. Ts'o wrote:
> What was the base of your patch?  It's not applying on my kernel tree.
>
> On Fri, Feb 07, 2020 at 07:07:59AM -0800, Mark Salyzyn wrote:
>> A followup to commit 428826f5358c922dc378830a1717b682c0823160
>> ("fdt: add support for rng-seed") to extend what was started
>> with Open Firmware (OF or Device Tree) parsing, but also add
>> it to the command line.
>>
>> If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the rng-seed
>> command line option length as added trusted entropy.
>>
>> Always rrase all views of the rng-seed option, except early command
>> line parsing, to prevent leakage to applications or modules, to
>> eliminate any attack vector.
> s/rrase/erase/
Noticed that immediately after posting, figured there would be another 
round ;-}
>
>> It is preferred to add rng-seed to the Device Tree, but some
>> platforms do not have this option, so this adds the ability to
>> provide some command-line-limited data to the entropy through this
>> alternate mechanism.  Expect all 8 bits to be used, but must exclude
>> space to be accounted in the command line.
> "all 8 bits"?

Command line (and Device Tree for that matter) can provide 8-bits of 
data, and specifically for the command line as long as they skip space 
and nul characters, we will be stripping the content out of the command 
line because we strip it from view, so that no one gets hot and bothered.

I expected this to be contentious, this is why I call it out. Does 
_anyone_ have a disagreement with allowing raw data (minus nul and space 
characters) to be part of the rng-seed?

>
>> @@ -875,6 +909,21 @@ asmlinkage __visible void __init start_kernel(void)
>>   	rand_initialize();
>>   	add_latent_entropy();
>>   	add_device_randomness(command_line, strlen(command_line));
>> +	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
>> +		size_t l = strlen(command_line);
>> +		char *rng_seed = strnstr(command_line, rng_seed_str, l);
>> +
>> +		if (rng_seed) {
>> +			char *end;
>> +
>> +			rng_seed += strlen(rng_seed_str);
>> +			l -= rng_seed - command_line;
>> +			end = strnchr(rng_seed, l, ' ');
>> +			if (end)
>> +				l = end - rng_seed;
>> +			credit_trusted_entropy(l);
>> +		}
>> +	}
> This doesn't look right at all.  It calls credit_trusted_entropy(),
> but it doesn't actually feed the contents of rng_seed where.  Why not
> just call add_hwgeneterator_randomness() and drop adding this
> credit_trusted_entropy(l)?

It is already added (will add comment so that this is clear) just above 
with add_device_randomness(command_line,). So all we need to do is 
_credit_ the entropy increase.

A call to add_hwgenerator_randomness() can block when the minimum 
threshold has been fulfilled resulting in a kernel panic, and would mix 
the bytes a second time when fed.

-- Mark