Fix illegal instructions on x86 CPU that have MMX but not SSE #6828
Conversation
…not SSE On x86, CRYPTO_gcm128_init() uses a MMX-enabled version if the CPU has the MMX flag. Unfortunately, the code contains the pinsrw instruction, which is only available when the CPU has the SSE flag. A CPU such as Geode GX1 has MMX but not SSE. Since it only checks MMX flag OpenSSL uses the unimplemented pinsrw instruction and the program crashes. This change makes sure the offending code is only used when the CPU has both MMX and SSE.
openssl-machine
added
the
hold: cla required
| if (OPENSSL_ia32cap_P[0] & (1 << 23)) { /* check MMX bit */ | ||
| # endif | ||
| if (OPENSSL_ia32cap_P[0] & (1 << 25) && /* check SSE bit */ | ||
| OPENSSL_ia32cap_P[0] & (1 << 23)) { /* check MMX bit */ |
There was a problem hiding this comment.
There seem to be misunderstanding. The [original] conditions above basically mean that if you configure without no-sse2 (which is the default), then it will check for SSE flag alone. At the same time code generated by ghash-x86.pl will indeed contain pinsrw, but it won't be executed on Geode, because SSE flag is checked. But[!] if you configure with no-sse2, then mmx code generated by ghash-x86.pl will not contain pinsrw, and check for MMX flag would be correct. If you in fact experience crash, then something went wrong somewhere. One possibility can be that you first configured without no-sse2, executed make, then re-configured with no-sse, but didn't execute make clean...
There was a problem hiding this comment.
Yes, I just figure that reporter deserves a chance to do so... Or maybe there is something more to it... Otherwise I'll close it after some grace period...
There was a problem hiding this comment.
This happens with NetBSD-8.0/i386 release, where some build operations (such as running configure and ghash-x86.pl) are done when an OpenSSL release is imported in the NetBSD tree, and not at build time. You answer suggest the import operation has been done badly.
The source tree is there: http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssl/
import-time generated ghash-x86.S contains pinsrw:
http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssl/lib/libcrypto/arch/i386/ghash-x86.S?rev=1.7
If I understand correctly, it is only compatible with code built with OPENSSL_IA32_SSE2. It seems a -DOPENSSL_IA32_SSE2 is missing in http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssl/lib/libcrypto/Makefile?rev=1.16
Is that the problem root?
There was a problem hiding this comment.
You should not import a configured build tree. You should run config on the target system.
There was a problem hiding this comment.
import-time generated ghash-x86.S contains pinsrw: ... If I understand correctly, it is only compatible with code built with OPENSSL_IA32_SSE2.
Correct. To summarize, it's problem with NetBSD, not OpenSSL.
There was a problem hiding this comment.
But what if I don't know what the target system is (I am creating x86 release binaries to be used on any IA32 system) and I want the best performance to be determined at runtime? I.e. should I be compiling with the minimal set of features for IA32?
There was a problem hiding this comment.
I want the best performance to be determined at runtime
Irregardless on what one does, if ghash-x86.S was generated with -DOPENSSL_IA32_SSE2, then gcm128.c has to be compiled with same option. It's just the way things are. Whether or not it's optimal choice [for specific corner case] is actually a different question. Currently ghash-x86.pl generates either vanilla+sse+pcmulqdq or vanilla+mmx(*). The rationale is that vast majority will use sse or pclmuldq, so that vanilla and mmx are kind of dead code. I wish we could omit both, but one can't and has to choose. Choice is obvious, keep vanilla as absolutely least common denominator. And vanilla+mmx option is kept for tailored builds, when you actually know that sse+pclmulqdq would be dead code, and just has have to score on mmx processor.
(*) Actually there is 3rd option, vanilla without bswap, so that code would be executable on pre-486.
There was a problem hiding this comment.
Well, one can probably argue that mmx omission is kind of cheap. Because mmx code size is not larger than 200 bytes... [To give perspective vanilla code is >700 bytes large.]
There was a problem hiding this comment.
Thanks for the explanation; I added the extra defines.
openssl/openssl#6828 When ghash-x86.S is generated with -DOPENSSL_IA32_SSE2 we need to compile gcm128.c with the same flags. Reported by manu@
openssl/openssl#6828 When ghash-x86.S is generated with -DOPENSSL_IA32_SSE2 we need to compile gcm128.c with the same flags. Reported by manu@
crypto/external/bsd/openssl/lib/libcrypto/arch/i386/modes.inc: revision 1.2 crypto/external/bsd/openssl/lib/libcrypto/arch/i386/modes.inc: revision 1.3 Add missing defines: openssl/openssl#6828 When ghash-x86.S is generated with -DOPENSSL_IA32_SSE2 we need to compile gcm128.c with the same flags. Reported by manu@ remove -DGHASH_ASM_X86; it is already defined.
crypto/external/bsd/openssl/lib/libcrypto/arch/i386/modes.inc: revision 1.2 crypto/external/bsd/openssl/lib/libcrypto/arch/i386/modes.inc: revision 1.3 Add missing defines: openssl/openssl#6828 When ghash-x86.S is generated with -DOPENSSL_IA32_SSE2 we need to compile gcm128.c with the same flags. Reported by manu@ remove -DGHASH_ASM_X86; it is already defined.
crypto/external/bsd/openssl/lib/libcrypto/arch/i386/modes.inc: revision 1.2 crypto/external/bsd/openssl/lib/libcrypto/arch/i386/modes.inc: revision 1.3 Add missing defines: openssl/openssl#6828 When ghash-x86.S is generated with -DOPENSSL_IA32_SSE2 we need to compile gcm128.c with the same flags. Reported by manu@ remove -DGHASH_ASM_X86; it is already defined.
crypto/external/bsd/openssl/lib/libcrypto/arch/i386/modes.inc: revision 1.2 crypto/external/bsd/openssl/lib/libcrypto/arch/i386/modes.inc: revision 1.3 Add missing defines: openssl/openssl#6828 When ghash-x86.S is generated with -DOPENSSL_IA32_SSE2 we need to compile gcm128.c with the same flags. Reported by manu@ remove -DGHASH_ASM_X86; it is already defined.
On x86, CRYPTO_gcm128_init() uses a MMX-enabled version if the CPU has
the MMX flag. Unfortunately, the code contains the pinsrw instruction,
which is only available when the CPU has the SSE flag.
A CPU such as Geode GX1 has MMX but not SSE. Since it only checks MMX
flag OpenSSL uses the unimplemented pinsrw instruction and the program
crashes.
This change makes sure the offending code is only used when the CPU
has both MMX and SSE.
Checklist