linux-kernel - Re: [PATCH 07/10] random: add new get_random_bytes

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]

Message-ID: <50100E93.1030809@zytor.com>
Date:	Wed, 25 Jul 2012 08:19:47 -0700
From:	"H. Peter Anvin" <hpa@...or.com>
To:	"Theodore Ts'o" <tytso@....edu>,
	Linux Kernel Developers List <linux-kernel@...r.kernel.org>,
	torvalds@...ux-foundation.org, w@....eu, ewust@...ch.edu,
	zakir@...ch.edu, greg@...ah.com, mpm@...enic.com,
	nadiah@...ucsd.edu, jhalderm@...ch.edu, tglx@...utronix.de,
	davem@...emloft.net, stable@...nel.org,
	DJ Johnston <dj.johnston@...el.com>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH 07/10] random: add new get_random_bytes_arch() function

On 07/25/2012 08:10 AM, Theodore Ts'o wrote:
>
> RDRAND is already getting mixed in already in xfer_secondary_pool() so
> we are already taking advantage of Bull Mountain (or any other
> CPU/architecture-specific hw RNG) if it is present.
>

You generate an arbitrary 16 or 32 bytes of RDRAND in one place, but 
that could be substantially less than the actual consumption.

My original tack was looking at doing this in extract_entropy and 
extract_entropy_user, but then I moved it to extract_buf since it looked 
simpler.

> Aside from whether it's better to do this step in
> xfer_secondary_pool() or extract_entropy(), your patch looks very
> wrong to me.  Nothing is actually *using* hash.l[], which is where the
> results of the RDRAND generator is placed.
>
> I assume you were planning on xor'ing hash.w and hash.l, but that's
> not present in your patch.
>
> I also don't understand why you are using a blind union here; it has
> no real advantage that I can see, and so it's all downside.  It bloats
> the patch (making it harder to see that your patch results in a net
> *decrease* in security, since it removes the use of RDRAND in
> xfer_security_pool, and replaces it with a no-op).
>

I don't know the term "blind union", but it is not an unnamed union is 
that is what you mean:

+	union {
+		__u32 w[5];
+		unsigned long l[LONGS(EXTRACT_SIZE)];
+	} hash;

This is a standard C declaration for hash.w and hash.l occupying the 
same memory, so your statement that "nothing uses hash.l" is just plain 
incorrect.  The reasons for using a union here is to make sure that the 
buffer is suitably aligned and padded to be able to be safely accessed 
via "unsigned long", even on architectures with alignment requirements. 
  Thus, xoring into hash.l modifies hash.w as well, so this is NOT a no-op.

Incidentally, this is exactly the same union construct you use before:

-	union {
-		__u32	tmp[OUTPUT_POOL_WORDS];
-		long	hwrand[4];
-	} u;

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
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/