linux-kernel - Re: [PATCH v3 2/5] random,x86: Add arch_get_slow_rng

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 16 Jul 2014 14:59:05 -0700
From:	"H. Peter Anvin" <hpa@...or.com>
To:	Andy Lutomirski <luto@...capital.net>, kvm@...r.kernel.org,
	"Theodore Ts'o" <tytso@....edu>, linux-kernel@...r.kernel.org,
	Kees Cook <keescook@...omium.org>, x86@...nel.org
CC:	Daniel Borkmann <dborkman@...hat.com>,
	Srivatsa Vaddagiri <vatsa@...ux.vnet.ibm.com>,
	Raghavendra K T <raghavendra.kt@...ux.vnet.ibm.com>,
	Gleb Natapov <gleb@...nel.org>,
	Paolo Bonzini <pbonzini@...hat.com>, bsd@...hat.com
Subject: Re: [PATCH v3 2/5] random,x86: Add arch_get_slow_rng_u64

On 07/16/2014 02:45 PM, Andy Lutomirski wrote:
> diff --git a/arch/x86/include/asm/archslowrng.h b/arch/x86/include/asm/archslowrng.h
> new file mode 100644
> index 0000000..c8e8d0d
> --- /dev/null
> +++ b/arch/x86/include/asm/archslowrng.h
> @@ -0,0 +1,30 @@
> +/*
> + * This file is part of the Linux kernel.
> + *
> + * Copyright (c) 2014 Andy Lutomirski
> + * Authors: Andy Lutomirski <luto@...capital.net>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#ifndef ASM_X86_ARCHSLOWRANDOM_H
> +#define ASM_X86_ARCHSLOWRANDOM_H
> +
> +#ifndef CONFIG_ARCH_SLOW_RNG
> +# error archslowrng.h should not be included if !CONFIG_ARCH_SLOW_RNG
> +#endif
> +

I'm *seriously* questioning the wisdom of this.  A much saner thing
would be to do:

#ifndef CONFIG_ARCH_SLOW_RNG

/* Not supported */
static inline int arch_get_slow_rng_u64(u64 *v)
{
	(void)v;
	return 0;
}

#endif

... which is basically what we do for the archrandom stuff.

I'm also wondering if it makes sense to have a function which prefers
arch_get_random*() over this one as a preferred interface.  Something like:

int get_random_arch_u64_slow_ok(u64 *v)
{
	int i;
	u64 x = 0;
	unsigned long l;

	for (i = 0; i < 64/BITS_PER_LONG; i++) {
		if (!arch_get_random_long(&l))
			return arch_get_slow_rng_u64(v);

		x |=  l << (i*BITS_PER_LONG);
	}
	*v = l;
	return 0;
}

This still doesn't address the issue e.g. on x86 where RDRAND is
available but we haven't set up alternatives yet.  So it might be that
what we really want is to encapsulate this fallback in arch code and do
a more direct enumeration.

> +
> +static int kvm_get_slow_rng_u64(u64 *v)
> +{
> +	/*
> +	 * Allow migration from a hypervisor with the GET_RNG_SEED
> +	 * feature to a hypervisor without it.
> +	 */
> +	if (rdmsrl_safe(MSR_KVM_GET_RNG_SEED, v) == 0)
> +		return 1;
> +	else
> +		return 0;
> +}

How about:

return rdmsrl_safe(MSR_KVM_GET_RNG_SEED, v) == 0;

The naming also feels really inconsistent...
	
	-hpa

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/