Bug 677688 - mix_pool_bytes_extract reads 3 bytes too many?
Summary: mix_pool_bytes_extract reads 3 bytes too many?
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: kernel
Version: 6.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: ---
Assignee: Neil Horman
QA Contact: Red Hat Kernel QE team
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-02-15 15:42 UTC by Stephan Mueller
Modified: 2011-02-15 15:56 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-02-15 15:49:19 UTC
Target Upstream Version:


Attachments (Terms of Use)

Description Stephan Mueller 2011-02-15 15:42:05 UTC
Description of problem:

I am wondering whether the following is a bug:

static void mix_pool_bytes_extract(struct entropy_store *r, const void *in,
                                   int nbytes, __u8 out[64])
{
...
        const char *bytes = in;
        __u32 w;
...
        while (nbytes--) {
                w = rol32(*bytes++, input_rotate & 31);

===> This function is called with some bytes in *in which is casted to a 
character array. The function must be given the size of the input data in 
nbytes. That character array is looped through byte-wise. Within the loop, the 
function reads 32 bit (i.e 4 bytes) from the offset within the character array 
as rol32 is defined with:

static inline __u32 rol32(__u32 word, unsigned int shift)

My question: isn't there a bug as the loop always reads 3 bytes too many? For 
example, I call the function with only one byte, the rol32 reads my byte plus 
subsequent bytes. Or if I call the function with 10 bytes, it reads 1 byte 
past my array when nbytes=3, 2bytes past the end when nbytes=2 and 3 bytes 
past the end when nbytes=1.

Is that really the intention?


Note, Miroslav Trmac replied:

I might be grossly misunderstanding the problem you see - let me try to explain my thoughts in detail and we'll see where we disagree.

Unlike Perl, in C the type of a "destination" of a value never affects the type used to evaluate the "source" of a value.

Let me try to rewrite this in more detail, with "tN" being temporary variables representing the internal implementation of the computation:
A) { char t1 = "*bytes"; } is evaluated
   This reads a single byte (because the type of "*bytes" is "const char")
B) { __u32 t2 = (__u32)t1; } is evaluated
   This conversion is implicit in the function call mechanism.
C) { __u32 t3 = rol32(t2, ...); } is evaluated
D) { w = t3; } is evaluated
E) Sometime after A, and no later than immediately after D,
   { bytes = bytes + 1; } is evaluated.
   This increments "bytes" to point at the next byte, not at the next word,
   because "bytes" has type "char *".

Where I replied again:

your explanation is nice, but it does not tell me what the contents of your tn 
variables are.

I would expect the following behavior in binary representation:

t1 = *bytes = "01000000"

After, say, rol32(t1, 4); t1 should contain:

t1 = 00000010

How does that fit with your explanation of a 32 bit variable type?

Comment 2 Neil Horman 2011-02-15 15:49:08 UTC
sorry, after discussion on list, this is NOTABUG.  The fact that we dereference the bytes array prior to passing it to rol32 means we cast the value of that byte rather than the pointer to that byte, so we never overrun the array, and despite the u32 cast, we never operate on more than a byte of data at a time.

Comment 3 Neil Horman 2011-02-15 15:54:44 UTC
regarding the last question about expected behavior, the assumption is wrong.  its a rol32 function, so even if you pass in a byte of data, the set bits may end up in the upper 24 bits of the result, it won't wrap at 8 bits, and thats expected.  Its why the result value (w) and all of the pool data is managed as 32 bit words.

Comment 4 Steve Grubb 2011-02-15 15:56:29 UTC
I suppose it would be easy to cobble together a test program from the afore mentioned functions and see what really happens if we needed to.


Note You need to log in before you can comment on or make changes to this bug.