Fix of crash in e_rc4_hmac_md5.c caused by rc4-md5-x86_64.S.

Set correct value for RC4_INT on x86_64

This caused caused crashes in the assembly versions of the rc4 algorithms, especially in rc4-md5-x86_64.S.

Also take back in rc4-x86_64.S since it will now work.

BUG=none

[haavardm, 2014-02-04 16:10:51]

Fixed a crash happening on https://microsoft.com (which uses the rc4 md5
combination) on 64 bit linux with openssl. 

Although I don't have access to issue 240674 referred in patch 26651005, I
assume this patch also fixes that.

[agl, 2014-02-04 16:22:45]

RC4_INT being unsigned int is standard on 64-bit and I guess we have to pay
attention to x86-64 now that we're switching over to OpenSSL.

However, we should have different configs for different platforms. We seem only
to have config/x64 at the moment and it doesn't look like it was actually
generated on a 64-bit system because then RC4_INT would be set to "unsigned int"
already.

[haavardm, 2014-02-04 16:28:12]

Yes, it would be more correct to fix it there. To be honest I set the define
in openssl.gyp as I haven't quite figured out how fix opensslconf.h
under current patch regime.


On 2014/02/04 16:22:45, agl wrote:
> RC4_INT being unsigned int is standard on 64-bit and I guess we have to pay
> attention to x86-64 now that we're switching over to OpenSSL.
> 
> However, we should have different configs for different platforms. We seem
only
> to have config/x64 at the moment and it doesn't look like it was actually
> generated on a 64-bit system because then RC4_INT would be set to "unsigned
int"
> already.

[wtc, 2014-02-04 18:20:19]

Håvard: thanks for the CL. I ran into this crash when I built Chromium on Linux
with use_openssl=1. It's great that you are looking at this.

https://codereview.chromium.org/153373003/diff/1/openssl.gyp
File openssl.gyp (right):

https://codereview.chromium.org/153373003/diff/1/openssl.gyp#newcode70
openssl.gyp:70: 'RC4_INT=unsigned int'

Summary: It seems better to change the RC4_INT definition in
third_party/openssl/config/x64/openssl/opensslconf.h, but I don't know what's
the right way to generate that header.

Details:

I'm not familiar with this part of OpenSSL. When I run ./config in an upstream
openssl tree on Linux x86_64, the generated opensslconf.h header contains these
RC4-related macro definitions:

=====
#if defined(HEADER_RC4_H)
#if !defined(RC4_INT)
/* using int types make the structure larger but make the code faster
 * on most boxes I have tested - up to %20 faster. */
/*
 * I don't know what does "most" mean, but declaring "int" is a must on:
 * - Intel P6 because partial register stalls are very expensive;
 * - elder Alpha because it lacks byte load/store instructions;
 */
#define RC4_INT unsigned int
#endif
#if !defined(RC4_CHUNK)
/*
 * This enables code handling data aligned at natural CPU word
 * boundary. See crypto/rc4/rc4_enc.c for further details.
 */
#define RC4_CHUNK unsigned long
#endif
#endif

...

#if defined(HEADER_RC4_LOCL_H) && !defined(CONFIG_HEADER_RC4_LOCL_H)
#define CONFIG_HEADER_RC4_LOCL_H
/* if this is defined data[i] is used instead of *data, this is a %20
 * speedup on x86 */
#undef RC4_INDEX
#endif
=====

RC4-related assembly code is assembled with these command lines:

=====
/usr/bin/perl asm/rc4-x86_64.pl elf > rc4-x86_64.s
gcc -c -I.. -I../.. -I../modes -I../asn1 -I../evp -I../../include 
-DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -Wa,--noexecstack -m64
-DL_ENDIAN -DTERMIO -O3 -Wall -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT
-DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM
-DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM
-DGHASH_ASM -c  -o rc4-x86_64.o rc4-x86_64.s
/usr/bin/perl asm/rc4-md5-x86_64.pl elf > rc4-md5-x86_64.s
gcc -c -I.. -I../.. -I../modes -I../asn1 -I../evp -I../../include 
-DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -Wa,--noexecstack -m64
-DL_ENDIAN -DTERMIO -O3 -Wall -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT
-DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM
-DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM
-DGHASH_ASM -c  -o rc4-md5-x86_64.o rc4-md5-x86_64.s
=====

I found three copies of opensslconf.h in the Chromium source tree:

third_party/openssl/openssl/crypto/opensslconf.h
third_party/openssl/openssl/include/openssl/opensslconf.h
third_party/openssl/config/x64/openssl/opensslconf.h

The first two copies are the same. The third copy seems to be x86 specific but
only has minor differences from the first two copies:

=====
$ diff -pu third_party/openssl/openssl/include/openssl/opensslconf.h
third_party/openssl/config/x64/openssl/opensslconf.h
--- third_party/openssl/openssl/include/openssl/opensslconf.h	2013-06-20
15:19:02.562863938 -0700
+++ third_party/openssl/config/x64/openssl/opensslconf.h	2013-06-20
15:19:03.322875471 -0700
@@ -220,14 +220,14 @@
 
 #if defined(HEADER_BN_H) && !defined(CONFIG_HEADER_BN_H)
 #define CONFIG_HEADER_BN_H
-#define BN_LLONG
+#undef BN_LLONG
 
 /* Should we define BN_DIV2W here? */
 
 /* Only one for the following should be defined */
-#undef SIXTY_FOUR_BIT_LONG
+#define SIXTY_FOUR_BIT_LONG
 #undef SIXTY_FOUR_BIT
-#define THIRTY_TWO_BIT
+#undef THIRTY_TWO_BIT
 #endif
 
 #if defined(HEADER_RC4_LOCL_H) && !defined(CONFIG_HEADER_RC4_LOCL_H)
=====

If I diff the opensslconf.h generated by "./config" and the x64 opensslconf.h in
the Chromium source tree, I see these interesting differences:
=====
$ diff -pu ~/tmp/openssl/include/openssl/opensslconf.h
third_party/openssl/config/x64/openssl/opensslconf.h
---
/usr/local/google/home/wtc/tmp/openssl/include/openssl/opensslconf.h	2014-02-04
09:51:32.850733107 -0800
+++ third_party/openssl/config/x64/openssl/opensslconf.h	2013-06-20
15:19:03.322875471 -0700
@@ -129,7 +199,7 @@
  * - Intel P6 because partial register stalls are very expensive;
  * - elder Alpha because it lacks byte load/store instructions;
  */
-#define RC4_INT unsigned int
+#define RC4_INT unsigned char
 #endif
 #if !defined(RC4_CHUNK)
 /*
@@ -169,7 +239,7 @@
 
 #if defined(HEADER_BF_LOCL_H) && !defined(CONFIG_HEADER_BF_LOCL_H)
 #define CONFIG_HEADER_BF_LOCL_H
-#undef BF_PTR
+#define BF_PTR
 #endif /* HEADER_BF_LOCL_H */
 
 #if defined(HEADER_DES_LOCL_H) && !defined(CONFIG_HEADER_DES_LOCL_H)
=====

We should also look into why the x64 opensslconf.h in the Chromium source tree
defines the BF_PTR macro.

[wtc, 2014-02-04 23:15:58]

On 2014/02/04 16:28:12, Håvard Molland wrote:
> Yes, it would be more correct to fix it there. To be honest I set the define
> in openssl.gyp as I haven't quite figured out how fix opensslconf.h
> under current patch regime.

Hi Håvard,

Please try this patch. I modified import_from_android.sh and then run it to
re-generate config/x64/openssl/opensslconf.h. According to the comments in
README.chromium, this is how config/x64/openssl/opensslconf.h should be
generated.

You can omit the BF_PTR change if you only want to fix the crash in RC4 assembly
code.

Index: config/x64/openssl/opensslconf.h
===================================================================
--- config/x64/openssl/opensslconf.h	(revision 248805)
+++ config/x64/openssl/opensslconf.h	(working copy)
@@ -199,7 +199,7 @@
  * - Intel P6 because partial register stalls are very expensive;
  * - elder Alpha because it lacks byte load/store instructions;
  */
-#define RC4_INT unsigned char
+#define RC4_INT unsigned int
 #endif
 #if !defined(RC4_CHUNK)
 /*
@@ -239,7 +239,7 @@
 
 #if defined(HEADER_BF_LOCL_H) && !defined(CONFIG_HEADER_BF_LOCL_H)
 #define CONFIG_HEADER_BF_LOCL_H
-#define BF_PTR
+#undef BF_PTR
 #endif /* HEADER_BF_LOCL_H */
 
 #if defined(HEADER_DES_LOCL_H) && !defined(CONFIG_HEADER_DES_LOCL_H)
Index: import_from_android.sh
===================================================================
--- import_from_android.sh	(revision 248805)
+++ import_from_android.sh	(working copy)
@@ -456,9 +456,11 @@
 dump "Generating 64-bit configuration header file."
 mkdir -p $PROGDIR/config/x64/openssl/
 sed \
+  -e 's|^#define RC4_INT unsigned char|#define RC4_INT unsigned int|g' \
   -e 's|^#define BN_LLONG|#undef BN_LLONG|g' \
   -e 's|^#define THIRTY_TWO_BIT|#undef THIRTY_TWO_BIT|g' \
   -e 's|^#undef SIXTY_FOUR_BIT_LONG|#define SIXTY_FOUR_BIT_LONG|g' \
+  -e 's|^#define BF_PTR|#undef BF_PTR|g' \
   $PROGDIR/openssl/include/openssl/opensslconf.h \
   > $PROGDIR/config/x64/openssl/opensslconf.h

[haavardm, 2014-02-05 13:05:23]

Thanks for the tip wtc,
I've applied your patch and adjusted openssl.gyp accordingly. 

I did add the BF_PTR change just for the sake of it. But as far as I know
this is only for blow fish, which isn't included in the cipher list.

On 2014/02/04 23:15:58, wtc wrote:
> On 2014/02/04 16:28:12, Håvard Molland wrote:
> > Yes, it would be more correct to fix it there. To be honest I set the define
> > in openssl.gyp as I haven't quite figured out how fix opensslconf.h
> > under current patch regime.
> 
> Hi Håvard,
> 
> Please try this patch. I modified import_from_android.sh and then run it to
> re-generate config/x64/openssl/opensslconf.h. According to the comments in
> README.chromium, this is how config/x64/openssl/opensslconf.h should be
> generated.
> 
> You can omit the BF_PTR change if you only want to fix the crash in RC4
assembly
> code.
> 
> Index: config/x64/openssl/opensslconf.h
> ===================================================================
> --- config/x64/openssl/opensslconf.h	(revision 248805)
> +++ config/x64/openssl/opensslconf.h	(working copy)
> @@ -199,7 +199,7 @@
>   * - Intel P6 because partial register stalls are very expensive;
>   * - elder Alpha because it lacks byte load/store instructions;
>   */
> -#define RC4_INT unsigned char
> +#define RC4_INT unsigned int
>  #endif
>  #if !defined(RC4_CHUNK)
>  /*
> @@ -239,7 +239,7 @@
>  
>  #if defined(HEADER_BF_LOCL_H) && !defined(CONFIG_HEADER_BF_LOCL_H)
>  #define CONFIG_HEADER_BF_LOCL_H
> -#define BF_PTR
> +#undef BF_PTR
>  #endif /* HEADER_BF_LOCL_H */
>  
>  #if defined(HEADER_DES_LOCL_H) && !defined(CONFIG_HEADER_DES_LOCL_H)
> Index: import_from_android.sh
> ===================================================================
> --- import_from_android.sh	(revision 248805)
> +++ import_from_android.sh	(working copy)
> @@ -456,9 +456,11 @@
>  dump "Generating 64-bit configuration header file."
>  mkdir -p $PROGDIR/config/x64/openssl/
>  sed \
> +  -e 's|^#define RC4_INT unsigned char|#define RC4_INT unsigned int|g' \
>    -e 's|^#define BN_LLONG|#undef BN_LLONG|g' \
>    -e 's|^#define THIRTY_TWO_BIT|#undef THIRTY_TWO_BIT|g' \
>    -e 's|^#undef SIXTY_FOUR_BIT_LONG|#define SIXTY_FOUR_BIT_LONG|g' \
> +  -e 's|^#define BF_PTR|#undef BF_PTR|g' \
>    $PROGDIR/openssl/include/openssl/opensslconf.h \
>    > $PROGDIR/config/x64/openssl/opensslconf.h

[haavardm, 2014-02-05 13:28:58]

If I understand this correctly, opensslconf.h is per cpu, not per 
platform (as in OS).

Thus, the two existing configs for x86 and x86_64 should be enough 
to cover the desktop versions of windows, mac and linux. Not sure about 
android/arm here though.

The more fine grained differences between the cpus is be handled in
runtime by  X86(_64)cpuid.S

[wtc, 2014-02-05 17:46:51]

Patch set 2 LGTM. Please shorten or update the CL's description based on our new
understanding of the problem. Thanks!

On 2014/02/05 13:28:58, Håvard Molland wrote:
> If I understand this correctly, opensslconf.h is per cpu, not per 
> platform (as in OS).

I don't know the answer to this question. My inspection of opensslconf.h shows
it has very little OS-specific stuff. I only saw two pathnames (/usr/lib/...)
and a OPENSSL_UNISTD macro for <unistd.h>.

It would be safer to just run upstream OpenSSL's "config" script on the
platforms we support and compare the generated opensslconf.h files to validate
this assumption.

> Thus, the two existing configs for x86 and x86_64 should be enough 
> to cover the desktop versions of windows, mac and linux.

I think this is true. Just to clarify: there is no existing opensslconf.h file
for x86 (32-bit) in the Chromium source tree, right?

[haavardm, 2014-02-06 16:03:30]

On 2014/02/05 17:46:51, wtc wrote:
> Patch set 2 LGTM. Please shorten or update the CL's description based on our
new
> understanding of the problem. Thanks!
> 
Thanks for reviewing! I'll do that.

> On 2014/02/05 13:28:58, Håvard Molland wrote:
> > If I understand this correctly, opensslconf.h is per cpu, not per 
> > platform (as in OS).
> 
> I don't know the answer to this question. My inspection of opensslconf.h shows
> it has very little OS-specific stuff. I only saw two pathnames (/usr/lib/...)
> and a OPENSSL_UNISTD macro for <unistd.h>.
> 
> It would be safer to just run upstream OpenSSL's "config" script on the
> platforms we support and compare the generated opensslconf.h files to validate
> this assumption.
> 
> > Thus, the two existing configs for x86 and x86_64 should be enough 
> > to cover the desktop versions of windows, mac and linux.
> 
> I think this is true. Just to clarify: there is no existing opensslconf.h file
> for x86 (32-bit) in the Chromium source tree, right?

I ran the openssl's configure script for osx 32 bit, and it did actually 
add a OPENSSL_SYSNAME_MACOSX define. So it does seem to be somewhat OS dependent
after all.

If the existing openssl/crypto/opensslconf.h and openssl/include/opensslconf.h
(the include copy) is for android then I cannot find any for x86 for 32 bit.

[haavardm, 2014-02-06 16:09:47]

The CQ bit was checked by haavardm@opera.com

[haavardm, 2014-02-06 16:09:49]

The CQ bit was unchecked by haavardm@opera.com

[haavardm, 2014-02-06 16:09:50]

The CQ bit was checked by haavardm@opera.com

[haavardm, 2014-02-06 16:09:52]

The CQ bit was unchecked by haavardm@opera.com

[haavardm, 2014-02-06 16:10:28]

The CQ bit was checked by haavardm@opera.com

[haavardm, 2014-02-07 09:15:30]

The CQ bit was unchecked by haavardm@opera.com

[haavardm, 2014-02-07 09:24:33]

The CQ bit was checked by haavardm@opera.com

[haavardm, 2014-02-07 12:48:30]

On 2014/02/07 09:24:33, Håvard Molland wrote:
> The CQ bit was checked by mailto:haavardm@opera.com

wtc,
Nothing seems to happen here after ticking of the CQ bit. Is it because this is
a third_party library?

[agl, 2014-02-07 16:15:49]

On 2014/02/07 12:48:30, Håvard Molland wrote:
> wtc,
> Nothing seems to happen here after ticking of the CQ bit. Is it because this
is
> a third_party library?

Not because it's third_party, but because it's a DEPS. Some directories in
Chromium are not part of the main repo, but are pulled in from different repos,
controlled by the DEPS file.

I've landed this change in the openssl DEPS repo in r249698. In
https://codereview.chromium.org/144463007 I'm testing the DEPS update on the
trybots.

The current third_party/openssl made sense when it was an Android thing, but
since we're going to make it more primary we may wish to reevaluate it.
Certainly the whole "patches of patches" is a complete nightmare.

[haavardm, 2014-02-07 17:32:55]

On 2014/02/07 16:15:49, agl wrote:
> On 2014/02/07 12:48:30, Håvard Molland wrote:

> I've landed this change in the openssl DEPS repo in r249698. In
> https://codereview.chromium.org/144463007 I'm testing the DEPS update on the
> trybots.

That's great, Thanks.

> 
> The current third_party/openssl made sense when it was an Android thing, but
> since we're going to make it more primary we may wish to reevaluate it.
> Certainly the whole "patches of patches" is a complete nightmare.

Yes, that bit seemed a bit complicated. Also, it's seems a bit weird to 
convert the android opensslconf.h  into the desktop versions instead of 
regenerating it directly.