Re: [openssl.org #2837] BN_nist_mod_384 inconsistency when compiling with the Windows DDK

[Paul Belanger via RT]

It works.  The new assembler is:

LEA eax,[c_d]
MOV DWORD PTR [res],eax
MOV ecx,DWORD PTR [mask]
NOT ecx
AND ecx,DWORD PTR [res]
MOV edx,DWORD PTR [r_d]
AND edx,DWORD PTR [mask]
OR ecx,edx
MOV DWORD PTR [res],ecx

The addition of the LEA instruction gets the pointer value from c_d instead
of its first element.

Note that you forgot to add it to the BN_nist_mod_521() function, probably
because the variable is named "t_d" in that function instead of "c_d".


> Question was if you can *confirm* if it does the trick.


You wrote:

> You don't need any extra variable, just do 'res = c_d;' and then 'res =
> ...res&~mask...'


Not to be pedantic, but that's not a question, that's a statement.  If you
want me to try something, ask, don't assume.   This isn't my job.

Working around compiler bugs is a gray zone. It's not exactly
> appropriate to do it, but it was done at several occasions. But there is
> one thing that is not gray zone, formulations like "I *assume* the same
> applies to other keys" are not. In other words if something will be
> done, no room for such assumptions will be left. I.e. it won't be about
> your particular usage case, but whole bn_nist.c. So that if you want to
> see it happen, you ought to confirm that the workaround applies to all
> subroutines. Best would be if you can confirm that all relevant EC tests
> fail and then after patch succeed.
>

I can appreciate that you must spend a good portion of your time reading
bug reports on this mailing list, and that it can get frustrating, but that
shouldn't excuse a poor attitude and poor etiquette.  I took time out of my
day (and from my actual job) to write a very detailed bug report, complete
with suggested fix.  Debugging OpenSSL by following assembly code isn't my
job.  If this is the way you respond to bug reports, I'll be sure to keep
them to myself in the future.

Thank you for the patch, and I'll consider your advice in the future.

Paul

It works.? The new assembler is:

LEA eax,[c_d]
MOV DWORD PTR [res],eax
MOV ecx,DWORD PTR [mask]
NOT ecx
AND ecx,DWORD PTR [res]
MOV edx,DWORD PTR [r_d]
AND edx,DWORD PTR [mask]
OR ecx,edx
MOV DWORD PTR [res],ecx

The addition of the LEA instruction gets the pointer value from c_d instead of its first element.

Note that you forgot to add it to the BN_nist_mod_521() function, probably because the variable is named "t_d" in that function instead of "c_d".

?

Question was if you can *confirm* if it does the trick.?

?
You wrote:

You don't need any extra variable, just do 'res = c_d;' and then 'res =
...res&~mask...'

Not to be pedantic, but that's not a question, that's a statement.? If you want me to try something, ask, don't assume.?? This isn't my job.

Working around compiler bugs is a gray zone. It's not exactly
appropriate to do it, but it was done at several occasions. But there is
one thing that is not gray zone, formulations like "I *assume* the same
applies to other keys" are not. In other words if something will be
done, no room for such assumptions will be left. I.e. it won't be about
your particular usage case, but whole bn_nist.c. So that if you want to
see it happen, you ought to confirm that the workaround applies to all
subroutines. Best would be if you can confirm that all relevant EC tests
fail and then after patch succeed.

I can appreciate that you must spend a good portion of your time reading bug reports on this mailing list, and that it can get frustrating, but that shouldn't excuse a poor attitude and poor etiquette.? I took time out of my day (and from my actual job) to write a very detailed bug report, complete with suggested fix.? Debugging OpenSSL by following assembly code isn't my job.? If this is the way you respond to bug reports, I'll be sure to keep them to myself in the future.

Thank you for the patch, and I'll consider your advice in the future.

Paul