crypto: disable NSS AES-NI support when AVX is disabled by OS.

crypto: disable NSS AES-NI support when AVX is disabled by OS.

When running under Xen, or with certain kernel configurations, it's possible
for the CPU to support AVX but for the operating system not to have configured
it. In this case, CPUID indicates that AVX support exists and NSS will try to
use it for AES-GCM. However, the first AVX instruction will cause an illegal
instruction exception.

This change works around the problem by disabling AES-NI support when AVX
support exists but is not supported by the OS. Sadly this also means that plain
AES instructions are also disabled in this case, but that's better than
crashing.

https://bugzilla.mozilla.org/show_bug.cgi?id=940794

BUG=320524
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236794

[Ryan Sleevi, 2013-11-20 21:46:28]

https://codereview.chromium.org/79283002/diff/1/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/79283002/diff/1/crypto/nss_util.cc#newcode638
crypto/nss_util.cc:638: // that |has_avx()| is false (which includes the XSAVE
test).
Let's include an NSS version check (or softoken version check) here so that
we're not always disabling it on Linux.

The softoken version is exposed via PKCS#11 as part of the library version
.major/.minor, which will handle the cases (such as RHEL) where softoken may be
older than NSS.

bool use_hw_aes_if_present = true;
CK_TOKEN_INFO token_info;
ScopedPK11Slot slot(PK11_GetInternalSlot());
if (!slot.get() || PK11_GetTokenInfo(slot, &token_info) != SECSuccess) {
  use_hw_aes_if_present = false;
}
if (token_info.hardwareVersion.major < XXX ||
    (token_info.hardwareVersion.major == XXX &&
     token_info.hardwareVersion.minor < YYY)) {
  use_hw_aes_if_present = cpu.has_avx_hardware() && cpu.has_avx();
}
if (!use_hw_aes_if_present) {
  setenv(...)
}

[agl, 2013-11-20 22:55:53]

Note that we'll only be disabling AES in rare cases (when running in some
versions of Xen or when noxsave is configured on the kernel command line.)

If we want the version check - how do I find the needed version numbers?

[Ryan Sleevi, 2013-11-21 00:41:38]

On 2013/11/20 22:55:53, agl wrote:
> Note that we'll only be disabling AES in rare cases (when running in some
> versions of Xen or when noxsave is configured on the kernel command line.)
> 
> If we want the version check - how do I find the needed version numbers?

Ah, fair enough. I thought this was gonna be a larger impact. FWIW, the softoken
version # is part of
http://mxr.mozilla.org/nss/source/lib/softoken/softkver.h#29 , but the patch
number isn't even exposed, which is what would likely be bumped anyways.

SGTM to go w/o the check though

[agl, 2013-11-21 18:40:18]

+ajwong to review changes in base/

[wtc, 2013-11-21 21:03:43]

Patch set 2 LGTM.

https://codereview.chromium.org/79283002/diff/1/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/79283002/diff/1/crypto/nss_util.cc#newcode638
crypto/nss_util.cc:638: // that |has_avx()| is false (which includes the XSAVE
test).

In this particular case, it is enough to check the version of the main NSS
library.

We can safely assume this bug will be fixed in NSS 3.15.4. Since Softoken 3.15.3
is not FIPS 140-2 validated, there is no reason for RHEL to use a newer version
of NSS with Softoken 3.15.3.

We can use the test
    NSS_VersionCheck("3.15") && !NSS_VersionCheck("3.15.4")
to detect the buggy version range (3.15 <= version < 3.15.4). Although the bug
was introduced in NSS 3.14.2, there were no NSS PK11_ functions to access
AES-GCM until NSS 3.15.

https://codereview.chromium.org/79283002/diff/230001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/79283002/diff/230001/base/cpu.cc#newcode129
base/cpu.cc:129: * See
http://software.intel.com/en-us/blogs/2011/04/14/is-avx-enabled */

Use C++ comment //.

https://codereview.chromium.org/79283002/diff/230001/base/cpu.cc#newcode132
base/cpu.cc:132: (cpu_info[2] & 0x08000000) == 0x08000000 /* OSXSAVE */ &&

Nit: since 0x08000000 has only one bit, you can just test != 0.

https://codereview.chromium.org/79283002/diff/230001/base/cpu.cc#newcode133
base/cpu.cc:133: (_xgetbv(0) & 6) == 6 /* XSAVE enabled by kernel */;

Nit: indent by four spaces

https://codereview.chromium.org/79283002/diff/230001/base/cpu.h
File base/cpu.h (right):

https://codereview.chromium.org/79283002/diff/230001/base/cpu.h#newcode52
base/cpu.h:52: bool has_avx_hardware() const { return has_avx_hardware_; }

I don't think has_avx_hardware() will be useful, except for some sort of CPU
info UI. Does such UI exist in Chrome? For Chrome, has_avx() should be
sufficient. So I recommend not adding has_avx_hardware().

Update: I see that we need has_avx_hardware() for our workaround for the NSS
bug. Perhaps we should add a comment to discourage people from using
has_avx_hardware() in a general setting.

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:15: #include <stdlib.h>

This header isn't necessary.

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:31: #include "base/environment.h"

Delete this line (a duplicate).

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:622: // that |has_avx()| is false (which includes the XSAVE
test).

Add the NSS bug number or URL here so we will know when it is safe to remove
this workaround.

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:623:
base::Environment::Create()->SetVar("NSS_DISABLE_HW_AES", "1");

We can also consider just turning off the AES-GCM cipher suites in TLS (and
probably QUIC). This will allow us to use the AES-CBC cipher suites with AES-NI
hardware acceleration. But this doesn't seem like a good trade-off.

[awong, 2013-11-22 01:02:46]

Going to rubber stamp LGTM base since there are no API changes.

[awong, 2013-11-22 01:03:31]

On 2013/11/22 01:02:46, awong wrote:
> Going to rubber stamp LGTM base since there are no API changes.

I mean no *subtle* API changes...Also, your last patch set is corrupt. Reupload?

[agl, 2013-11-22 16:22:35]

(retrying failed upload.)

https://codereview.chromium.org/79283002/diff/230001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/79283002/diff/230001/base/cpu.cc#newcode129
base/cpu.cc:129: * See
http://software.intel.com/en-us/blogs/2011/04/14/is-avx-enabled */
On 2013/11/21 21:03:44, wtc wrote:
> 
> Use C++ comment //.

Done.

https://codereview.chromium.org/79283002/diff/230001/base/cpu.cc#newcode132
base/cpu.cc:132: (cpu_info[2] & 0x08000000) == 0x08000000 /* OSXSAVE */ &&
On 2013/11/21 21:03:44, wtc wrote:
> 
> Nit: since 0x08000000 has only one bit, you can just test != 0.

Done.

https://codereview.chromium.org/79283002/diff/230001/base/cpu.cc#newcode133
base/cpu.cc:133: (_xgetbv(0) & 6) == 6 /* XSAVE enabled by kernel */;
On 2013/11/21 21:03:44, wtc wrote:
> 
> Nit: indent by four spaces

Done.

https://codereview.chromium.org/79283002/diff/230001/base/cpu.h
File base/cpu.h (right):

https://codereview.chromium.org/79283002/diff/230001/base/cpu.h#newcode52
base/cpu.h:52: bool has_avx_hardware() const { return has_avx_hardware_; }
On 2013/11/21 21:03:44, wtc wrote:
> 
> I don't think has_avx_hardware() will be useful, except for some sort of CPU
> info UI. Does such UI exist in Chrome? For Chrome, has_avx() should be
> sufficient. So I recommend not adding has_avx_hardware().
> 
> Update: I see that we need has_avx_hardware() for our workaround for the NSS
> bug. Perhaps we should add a comment to discourage people from using
> has_avx_hardware() in a general setting.

Done.

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:15: #include <stdlib.h>
On 2013/11/21 21:03:44, wtc wrote:
> 
> This header isn't necessary.

Done.

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:31: #include "base/environment.h"
On 2013/11/21 21:03:44, wtc wrote:
> 
> Delete this line (a duplicate).

Done.

https://codereview.chromium.org/79283002/diff/230001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:622: // that |has_avx()| is false (which includes the XSAVE
test).
On 2013/11/21 21:03:44, wtc wrote:
> 
> Add the NSS bug number or URL here so we will know when it is safe to remove
> this workaround.

Done.

[commit-bot: I haz the power, 2013-11-22 16:24:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/79283002/490001

[commit-bot: I haz the power, 2013-11-22 18:35:08]

Change committed as 236794