Re: [openssl.org #3397] Fwd: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
Discovered this problem while trying to fix https://github.com/joyent/node/issues/7704. Attached is a fix for it. Trouble is that modified code might avoid crash, but it doesn't produce correct result either. [No, not even Adam's suggestion]. Actually bn_mul_mont is abused in bn_exp.c, because it's actually allowed to do nothing depending on arguments, which is noted by zero return value. The criteria for doing nothing is platform-specific, for example some platforms insist that number of elements in even and data pointers are aligned in specific manner. But there is one common criteria and it is requirement that number of elements is larger than one. Correct solution would be to introduce for example bn_mul_mont_eligible call that could be queried prior taking the path calling bn_mul_mont. For the moment there is only one platform that have this path, x86_64, and as interim measure it's possible to implement the boundary condition check as http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=eca441b2b4d33d2a18d163ef9b4b3aff14251c73. __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3397] Fwd: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
Andy, I'd still pull Adam's changes, at least for consistency reasons. Other assembly files seems to be using signed comparison for the same kinds of operations. What do you think about it? Cheers, Fedor. On Wed, Jul 2, 2014 at 9:54 PM, Andy Polyakov via RT r...@openssl.org wrote: Discovered this problem while trying to fix https://github.com/joyent/node/issues/7704. Attached is a fix for it. Trouble is that modified code might avoid crash, but it doesn't produce correct result either. [No, not even Adam's suggestion]. Actually bn_mul_mont is abused in bn_exp.c, because it's actually allowed to do nothing depending on arguments, which is noted by zero return value. The criteria for doing nothing is platform-specific, for example some platforms insist that number of elements in even and data pointers are aligned in specific manner. But there is one common criteria and it is requirement that number of elements is larger than one. Correct solution would be to introduce for example bn_mul_mont_eligible call that could be queried prior taking the path calling bn_mul_mont. For the moment there is only one platform that have this path, x86_64, and as interim measure it's possible to implement the boundary condition check as http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=eca441b2b4d33d2a18d163ef9b4b3aff14251c73 .
Re: [openssl.org #3397] Fwd: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
Andy, I'd still pull Adam's changes, at least for consistency reasons. Other assembly files seems to be using signed comparison for the same kinds of operations. What do you think about it? Cheers, Fedor. On Wed, Jul 2, 2014 at 9:54 PM, Andy Polyakov via RT r...@openssl.org wrote: Discovered this problem while trying to fix https://github.com/joyent/node/issues/7704. Attached is a fix for it. Trouble is that modified code might avoid crash, but it doesn't produce correct result either. [No, not even Adam's suggestion]. Actually bn_mul_mont is abused in bn_exp.c, because it's actually allowed to do nothing depending on arguments, which is noted by zero return value. The criteria for doing nothing is platform-specific, for example some platforms insist that number of elements in even and data pointers are aligned in specific manner. But there is one common criteria and it is requirement that number of elements is larger than one. Correct solution would be to introduce for example bn_mul_mont_eligible call that could be queried prior taking the path calling bn_mul_mont. For the moment there is only one platform that have this path, x86_64, and as interim measure it's possible to implement the boundary condition check as http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=eca441b2b4d33d2a18d163ef9b4b3aff14251c73 . __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3397] Fwd: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
I'd still pull Adam's changes, at least for consistency reasons. Other assembly files seems to be using signed comparison for the same kinds of operations. What do you think about it? I think it's appropriate to harmonize branches with interface. But it takes deal of concentration and unfortunately for this moment I'm limited in time. As mentioned, solution is *interim* and I leave the ticket open with intention to come back to it and resolve it by double-checking all modules, harmonizing branches and implementing bn_mul_mont_eligible (or similar). __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3397] Fwd: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
I'm totally willing to cooperate on this, and may have enough skills to do it. Do you think it could be possible for us to collaborate on this topic? Thank you, Fedor. On Wed, Jul 2, 2014 at 11:08 PM, Andy Polyakov via RT r...@openssl.org wrote: I'd still pull Adam's changes, at least for consistency reasons. Other assembly files seems to be using signed comparison for the same kinds of operations. What do you think about it? I think it's appropriate to harmonize branches with interface. But it takes deal of concentration and unfortunately for this moment I'm limited in time. As mentioned, solution is *interim* and I leave the ticket open with intention to come back to it and resolve it by double-checking all modules, harmonizing branches and implementing bn_mul_mont_eligible (or similar). __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3397] Fwd: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
I'm totally willing to cooperate on this, and may have enough skills to do it. Do you think it could be possible for us to collaborate on this topic? Thank you, Fedor. On Wed, Jul 2, 2014 at 11:08 PM, Andy Polyakov via RT r...@openssl.org wrote: I'd still pull Adam's changes, at least for consistency reasons. Other assembly files seems to be using signed comparison for the same kinds of operations. What do you think about it? I think it's appropriate to harmonize branches with interface. But it takes deal of concentration and unfortunately for this moment I'm limited in time. As mentioned, solution is *interim* and I leave the ticket open with intention to come back to it and resolve it by double-checking all modules, harmonizing branches and implementing bn_mul_mont_eligible (or similar).
Re: [openssl.org #3397] Fwd: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
I'm totally willing to cooperate on this, and may have enough skills to do it. Do you think it could be possible for us to collaborate on this topic? Sure! I'd appreciate it. Start by preparing patch harmonizing branches with interface. Still I can't promise swift review and reply, so please be patient for couple of weeks. __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
[openssl.org #3397] Fwd: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
Hello everyone! Discovered this problem while trying to fix https://github.com/joyent/node/issues/7704. Attached is a fix for it. Cheers, Fedor. -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 - From c6a4d5ff66cd886023f75780e876053f019ed8de Mon Sep 17 00:00:00 2001 From: Fedor Indutny fe...@indutny.com Date: Fri, 6 Jun 2014 14:33:10 -0700 Subject: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs Fix index overflow in bn_mul_mont and bn_mul_mont_gather5. - --- crypto/bn/asm/x86_64-mont.pl | 4 ++-- crypto/bn/asm/x86_64-mont5.pl | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crypto/bn/asm/x86_64-mont.pl b/crypto/bn/asm/x86_64-mont.pl index 3803928..7ec1654 100755 - --- a/crypto/bn/asm/x86_64-mont.pl +++ b/crypto/bn/asm/x86_64-mont.pl @@ -174,7 +174,7 @@ $code.=___; mulq$m1 # np[j]*m1 cmp $num,$j - - jne .L1st + jl .L1st add %rax,$hi1 mov ($ap),%rax # ap[0] @@ -240,7 +240,7 @@ $code.=___; mulq$m1 # np[j]*m1 cmp $num,$j - - jne .Linner + jl .Linner add %rax,$hi1 mov ($ap),%rax # ap[0] diff --git a/crypto/bn/asm/x86_64-mont5.pl b/crypto/bn/asm/x86_64-mont5.pl index c107df9..5cc0689 100755 - --- a/crypto/bn/asm/x86_64-mont5.pl +++ b/crypto/bn/asm/x86_64-mont5.pl @@ -207,7 +207,7 @@ $code.=___; mulq$m1 # np[j]*m1 cmp $num,$j - - jne .L1st + jl .L1st movq%xmm0,$m0 # bp[1] @@ -290,7 +290,7 @@ $code.=___; mulq$m1 # np[j]*m1 cmp $num,$j - - jne .Linner + jl .Linner movq%xmm0,$m0 # bp[i+1] - -- 1.9.3 -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQIcBAEBAgAGBQJTkjPhAAoJEPsOEJWxeXmZc00P/3KY+9WOVwZ2Ad+1uqRquxAQ zlLDmhRAae0Z7mGrsExW9EHOu0RD8sMgX901uy4CAKOPon86skxbe70TPJ+8ZmMv tGgHrJhGos6fe6x47r9KgpYFng3lltN/i6rgArDnK9fZa5+7n9/Txz/NjMUgVMd/ Fw0zAvqOqpy5Q5YxgD85tBMInZqliYiV2NGKa7hHlnCcWzqEEZThy0LpAltb4L4c bBAviH6EzxFWE4i7f9Id0e6V0taoN5LMM0Ia2UikCXlA1sPchwojirmXEVu/vvYu AnBsnBYLUSxHCnUFOT/EezAkiZlXnQEr8ZGOmoxfPWWrFJ4rMuxgZpjrZbZ6cIwf CTgP0SxUQkftbP354aAJGeP8c4AtRP/PLEFB1rZovbVC0v99wzAo9xb69yXy+hUR 3D2C1Xcj7TYS2wB+u4sbUvluaLIWLTFCRxiONqNhG3J7oQQHrkleWkhrktbcQBxi Rotqtn34zkPN6cYWjjc6j6Ww6fQpKRsyRb677fe695Jp/MgRcsg/QTz+uVg5U3HS tt/9XDm6fg6FKD7PgBNn5okoDkKtWSRUOEUddy28B/YSB/c7AVWvbH6cLW3dp0iF 20/YuGEWSogzOEcy8Tsn0I3owLSMOe5fSCtYMoFJRCD1hlgonXpbEMMq85dnF/vg JT5UiUwprJ54GkHFnhDX =j4Ho -END PGP SIGNATURE- 0001-x86_64-asm-fix-bn_mul_mont-on-odd-len-BNs.patch Description: Binary data
Re: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
Bump. On Fri, Jun 6, 2014 at 2:35 PM, Fedor Indutny fe...@indutny.com wrote: Hello everyone! Discovered this problem while trying to fix https://github.com/joyent/node/issues/7704. Attached is a fix for it. Cheers, Fedor.
[PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
Hello everyone! Discovered this problem while trying to fix https://github.com/joyent/node/issues/7704. Attached is a fix for it. Cheers, Fedor. -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 - From c6a4d5ff66cd886023f75780e876053f019ed8de Mon Sep 17 00:00:00 2001 From: Fedor Indutny fe...@indutny.com Date: Fri, 6 Jun 2014 14:33:10 -0700 Subject: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs Fix index overflow in bn_mul_mont and bn_mul_mont_gather5. - --- crypto/bn/asm/x86_64-mont.pl | 4 ++-- crypto/bn/asm/x86_64-mont5.pl | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crypto/bn/asm/x86_64-mont.pl b/crypto/bn/asm/x86_64-mont.pl index 3803928..7ec1654 100755 - --- a/crypto/bn/asm/x86_64-mont.pl +++ b/crypto/bn/asm/x86_64-mont.pl @@ -174,7 +174,7 @@ $code.=___; mulq$m1 # np[j]*m1 cmp $num,$j - - jne .L1st + jl .L1st add %rax,$hi1 mov ($ap),%rax # ap[0] @@ -240,7 +240,7 @@ $code.=___; mulq$m1 # np[j]*m1 cmp $num,$j - - jne .Linner + jl .Linner add %rax,$hi1 mov ($ap),%rax # ap[0] diff --git a/crypto/bn/asm/x86_64-mont5.pl b/crypto/bn/asm/x86_64-mont5.pl index c107df9..5cc0689 100755 - --- a/crypto/bn/asm/x86_64-mont5.pl +++ b/crypto/bn/asm/x86_64-mont5.pl @@ -207,7 +207,7 @@ $code.=___; mulq$m1 # np[j]*m1 cmp $num,$j - - jne .L1st + jl .L1st movq%xmm0,$m0 # bp[1] @@ -290,7 +290,7 @@ $code.=___; mulq$m1 # np[j]*m1 cmp $num,$j - - jne .Linner + jl .Linner movq%xmm0,$m0 # bp[i+1] - -- 1.9.3 -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQIcBAEBAgAGBQJTkjPhAAoJEPsOEJWxeXmZc00P/3KY+9WOVwZ2Ad+1uqRquxAQ zlLDmhRAae0Z7mGrsExW9EHOu0RD8sMgX901uy4CAKOPon86skxbe70TPJ+8ZmMv tGgHrJhGos6fe6x47r9KgpYFng3lltN/i6rgArDnK9fZa5+7n9/Txz/NjMUgVMd/ Fw0zAvqOqpy5Q5YxgD85tBMInZqliYiV2NGKa7hHlnCcWzqEEZThy0LpAltb4L4c bBAviH6EzxFWE4i7f9Id0e6V0taoN5LMM0Ia2UikCXlA1sPchwojirmXEVu/vvYu AnBsnBYLUSxHCnUFOT/EezAkiZlXnQEr8ZGOmoxfPWWrFJ4rMuxgZpjrZbZ6cIwf CTgP0SxUQkftbP354aAJGeP8c4AtRP/PLEFB1rZovbVC0v99wzAo9xb69yXy+hUR 3D2C1Xcj7TYS2wB+u4sbUvluaLIWLTFCRxiONqNhG3J7oQQHrkleWkhrktbcQBxi Rotqtn34zkPN6cYWjjc6j6Ww6fQpKRsyRb677fe695Jp/MgRcsg/QTz+uVg5U3HS tt/9XDm6fg6FKD7PgBNn5okoDkKtWSRUOEUddy28B/YSB/c7AVWvbH6cLW3dp0iF 20/YuGEWSogzOEcy8Tsn0I3owLSMOe5fSCtYMoFJRCD1hlgonXpbEMMq85dnF/vg JT5UiUwprJ54GkHFnhDX =j4Ho -END PGP SIGNATURE- 0001-x86_64-asm-fix-bn_mul_mont-on-odd-len-BNs.patch Description: Binary data
