Re: [PATCH] char: random: casting prevents missing calculations

[Arnd Bergmann]

On Sun, May 7, 2017 at 9:58 PM, Karim Eshapa <karim.eshapa@xxxxxxxxx> wrote:
> On Sun, 7 May 2017 20:36:55 +0200, Arnd Bergmann wrote:
>>On Sun, May 7, 2017 at 2:47 PM, Karim Eshapa <karim.eshapa@xxxxxxxxx> wrote:
>>> Cast (long)jiffies and (long)state->last_time beacause
>>> they tends to unsigned long. may cause a problem specially
>>> when comparison happens (< 0).
>>>
>>> Signed-off-by: Karim Eshapa <karim.eshapa@xxxxxxxxx>
>>>
>>I don't understand what you are saying above, and the patch does not
>>appear to have any effect since the destination variable is already of
>>type 'long'.
>>
>>What problem did you observe?
>>
>
> I mean if jiffies = 0xf0000000 and
> state->last_time was = 0x70000000 then
>
> sample.jiffies = jiffies; here you assign signed long with
> unsigned with last bit = 1
> ..
> ...
> if (!state->dont_count_entropy) {
>         delta = sample.jiffies - state->last_time;
>         state->last_time = sample.jiffies;
>         ...
>         ...
>
>         if (delta < 0)
>         so, here this condition will be
>         true while it has to be false.
> }
>
> So, I think that may cause a problem.

No:

a) your patch does not change this behavior.
b) The case you describe (delta == -LONG_MIN) only hits when
    add_timer_randomness gets called after exactly a 0x80000000
    jiffies (24 days plus a bit, or more depending on CONFIG_HZ)
   delay, when in practice it should get called for every input
   or block event.
c) If you do manage to time the input even exactly to the right
    time interval of 24 days and the computer does nothing else
    in the meantime, min_t(int, fls(delta>>1), 11) is still limited to
    11 bits for this one time event, and that would be a reasonable
    number of entropy bits for waiting this long.

      Arnd