shadow-maint / shadow

Upstream shadow tree
Other
287 stars 227 forks source link

lib/csrand.c: Fix the lower part of the domain of csrand_uniform() #1025

Closed alejandro-colomar closed 3 weeks ago

alejandro-colomar commented 3 weeks ago

I accidentally broke this code during an un-optimization. We need to start from a random value of the width of the limit, that is, 32 bits.

Thanks to Jason for pointing to his similar code in the kernel, which made me see my mistake.

Fixes: 2a61122b5e8f ("Unoptimize the higher part of the domain of csrand_uniform()") Closes: https://github.com/shadow-maint/shadow/issues/1015 Reported-by: @michaelbrunnbauer Link: https://git.zx2c4.com/linux-rng/tree/drivers/char/random.c#n535 Cc: @zx2c4 Link: https://github.com/shadow-maint/shadow/pull/638 Link: https://github.com/shadow-maint/shadow/issues/634 Link: https://github.com/shadow-maint/shadow/pull/624

alejandro-colomar commented 3 weeks ago

@michaelbrunnbauer would you mind testing if this solves the problem for you?

Thanks for the bug report!! And sorry for goofing this. :)

michaelbrunnbauer commented 3 weeks ago

Actually, I do. You completely ignored what I wrote ("The problem seems to be that the algorithm in csrand_uniform32 assumes csrand() to be in the range 2^32 while it is actually 2^64") only to discover painstakingly it was true.

alejandro-colomar commented 3 weeks ago

Hi @michaelbrunnbauer ,

You completely ignored what I wrote ("The problem seems to be that the algorithm in csrand_uniform32 assumes csrand() to be in the range 2^32 while it is actually 2^64") only to discover painstakingly it was true.

I didn't ignore it. I just didn't understand it (I didn't see that you had the precise line of code that had the bug and why). It was later when I found the problem when I understood that post from you. I'm very sorry about that, which I understand to be offensive. It probably added to the fact that nobody had responded to you in 2 weeks, which was also accidental.

Actually, I do.

Okay. This has all been accidental, and I tend to be nice and respectful (or that's what I believe). I hope you don't run away from here. Again, I'm sorry for the misunderstandings.

Have a lovely day! Alex

michaelbrunnbauer commented 3 weeks ago

OK, I'll test the code. Currently struggling to find a downloadable diff from the pull request.

michaelbrunnbauer commented 3 weeks ago

It works - thank for fixing this. I will try to add my review as it seems to be requested.

alejandro-colomar commented 3 weeks ago

Thanks!

$ git range-diff shadow/master gh/csrand csrand 
1:  0f7571bf ! 1:  f3cc11b8 lib/csrand.c: Fix the lower part of the domain of c>
    @@ Commit message
         Link: <https://github.com/shadow-maint/shadow/pull/638>
         Link: <https://github.com/shadow-maint/shadow/issues/634>
         Link: <https://github.com/shadow-maint/shadow/pull/624>
    +    Tested-by: Michael Brunnbauer <https://github.com/michaelbrunnbauer>
    +    Reviewed-by: Michael Brunnbauer <https://github.com/michaelbrunnbauer>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## lib/csrand.c ##
alejandro-colomar commented 3 weeks ago

OK, I'll test the code. Currently struggling to find a downloadable diff from the pull request.

There's a trick: if you append .patch to the url of a pull request, you get a patch set that you can pass to git-am(1). In this case, it would be https://github.com/shadow-maint/shadow/pull/1025.patch

The same can be done with .diff, and it gives a diff without the commit metadata.