lkml.org 
[lkml]   [2012]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.



\
 
 \ /
  Last update: 2012-07-25 18:02    [W:0.112 / U:2.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site