Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cpu feature detection #392

Closed
wants to merge 17 commits into from
Closed

Cpu feature detection #392

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b1fb0a5
Support for parsing /proc/self/auxv binary format
Dec 24, 2016
52eca58
Parse /proc/cpuinfo into a map, and add some more auxval tests for ba…
Dec 24, 2016
f7e046d
Port the arm-specific feature detection logic, with guesses at how to
Dec 24, 2016
e8e1253
Move auxv and cpuinfo parsing into arm_linux submodule
Dec 26, 2016
aaaa73b
Shift platform specific code into arm_linux module
Dec 26, 2016
7b6ac18
Eliminate libc dependency. Could not re-use `c::long` C helpers
Dec 26, 2016
cae8c11
Make search_auxv take a Path not a BufRead since in practice it alway…
Dec 26, 2016
9ad928a
Expose a parse_cpuinfo() that does the File-opening for you
Dec 26, 2016
e914243
Add a few tests for the top level armcap calculation
Dec 26, 2016
042b8cb
Rework `unsigned long` handling to allow tests to always run in
Dec 26, 2016
ce050f9
Add a C wrapper around getauxval and invoke it from Rust
Dec 27, 2016
0843d84
Incorporate getauxval into armcap logic
Dec 27, 2016
97f0261
Remove linux-only cfg for some tests that could run anywhere; tidy up…
Dec 27, 2016
dfb508a
In the real callback, map failure to read /proc/self/auxv as an empty…
Dec 27, 2016
4db4eb8
Tidy up the armcap tests to reduce duplicated verbosity
Dec 27, 2016
1695809
Add sample files from 32-bit x86 linux plus a few more tests
Dec 27, 2016
71667e6
Remove C for arm cpu feature detection
Dec 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ include = [
"crypto/cipher/e_aes.c",
"crypto/cipher/internal.h",
"crypto/constant_time_test.c",
"crypto/cpu.c",
"crypto/cpu-aarch64-linux.c",
"crypto/cpu-arm-linux.c",
"crypto/cpu-arm.c",
Expand Down Expand Up @@ -261,6 +262,7 @@ name = "ring"

[dependencies]
untrusted = "0.3.2"
byteorder = "0.5.3"

[target.'cfg(unix)'.dependencies]
lazy_static = "0.2.1"
Expand Down
4 changes: 0 additions & 4 deletions crypto/cpu-arm-linux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@
#define HWCAP2_SHA1 (1 << 2)
#define HWCAP2_SHA2 (1 << 3)

/* |getauxval| is not available on Android until API level 20. Link it as a weak
* symbol and use other methods as fallback. */
unsigned long getauxval(unsigned long type) __attribute__((weak));

static int open_eintr(const char *path, int flags) {
int ret;
do {
Expand Down
19 changes: 19 additions & 0 deletions crypto/cpu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#include <openssl/cpu.h>

#ifdef __linux__
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internet claims this is the most widely portable flavor of #define for linux. Unfortunately there doesn't seem to be a "only glibc" define, but even if we had that we'd still have to deal with the weak linkage anyway...

Copy link
Owner

@briansmith briansmith Jan 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that seems right to me. However, I'd prefer to keep this in the cpu-arm-linux.c file, since it really is specific to ARM Linux.

unsigned long getauxval_wrapper(unsigned long type, char *success) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use this signature:

int getauxval_wrapper(unsigned long type, unsigned long *out_value) {

The idea is that success/failure is indicated by the return value and the output value is made an out parameter. This is consistent with other stuff in ring and BoringSSL.

I suggest we document this function's return value: It returns 1 if getauxval() is available and 0 if getauxval() is not available. If getauxval() is available, it also sets out_value to the result of getauxval(type). That result might be zero, which indicates that getauxval(type) didn't recognize type.

I think it's also good to document that this is a workaround for the fact that weak symbols aren't supported well in (stable) Rust.

if (getauxval == NULL) {
*success = 0;
return 0;
}

unsigned long result = getauxval(type);
if (errno != 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU, it isn't legal to check errno unless a function returns an error code first. I suggest that we just remove this if(...) { ... } completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is weird, and felt wrong even with my pretty rusty C. Unfortunately, 0 is a valid value, so the original getauxval API was simply broken in that case... In the manpage, they have this:

BUGS
Before the addition of the ENOENT error in glibc 2.19, there was no way to unambiguously distinguish the case where type could not be found from the case where the value corresponding to type was zero.

This is the implementation in glibc's misc/auxval.c:

unsigned long int
__getauxval (unsigned long int type)
{
#ifdef HAVE_AUX_VECTOR
  ElfW(auxv_t) *p;
#endif

  if (type == AT_HWCAP)
    return GLRO(dl_hwcap);
  else if (type == AT_HWCAP2)
    return GLRO(dl_hwcap2);

#ifdef HAVE_AUX_VECTOR
  for (p = GLRO(dl_auxv); p->a_type != AT_NULL; p++)
    if (p->a_type == type)
      return p->a_un.a_val;
#endif

  __set_errno (ENOENT);
  return 0;
}

They also have elf/tst-auxv.c:

static int
do_test (int argc, char *argv[])
{
  errno = 0;
  const char *execfn = (const char *) getauxval (AT_NULL);

  if (errno != ENOENT)
    {
      printf ("errno is %d rather than %d (ENOENT) on failure\n", errno,
          ENOENT);
      return 1;
    }

  if (execfn != NULL)
    {
      printf ("getauxval return value is nonzero on failure\n");
      return 1;
    }

  errno = 0;
  execfn = (const char *) getauxval (AT_EXECFN);

  if (execfn == NULL)
    {
      printf ("No AT_EXECFN found, AT_EXECFN test skipped\n");
      return 0;
    }

  if (errno != 0)
    {
      printf ("errno erroneously set to %d on success\n", errno);
      return 1;
    }

  if (strcmp (argv[0], execfn) != 0)
    {
      printf ("Mismatch: argv[0]: %s vs. AT_EXECFN: %s\n", argv[0], execfn);
      return 1;
    }

  return 0;
}

I took this to mean we should check errno. https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=6619179 seems to agree that while regrettable, this is how it has to be done when every possible return value is potentially valid. Thoughts?

*success = 0;
return 0;
}

*success = 1;
return result;
}
#endif
10 changes: 10 additions & 0 deletions include/openssl/cpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,16 @@ extern "C" {

/* Runtime CPU feature support */

#ifdef __linux__

/* |getauxval| is not available on Android until API level 20. Link it as a weak
* symbol and use other methods as fallback. */
unsigned long getauxval(unsigned long type) __attribute__((weak));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't figure out how to make weak linkage work on macOS's compiler toolchain, so this and all tests that hit the C bits are linux-only unfortunately.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's OK. These are really Linux-specific and ARM-specific anyway, AFAICT.

However, let's just put this declaration in cpu-arm-linux.c. There's no reason for it to be in a header file, especially these "public" header files.


#include <errno.h>
unsigned long getauxval_wrapper(unsigned long type, char *success);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Don't include errno.h here. Instead include it in the .c file.
  2. The convention we've established in ring is that any function that is only called by Rust code should have the declaration in the C file, like this:
/* Prevent -Wmissing-prototypes warnings. */
unsigned long getauxval_wrapper(unsigned long type, char *success);

So, basically, move these to cpu-arm-linux.c.


#endif

#if defined(OPENSSL_X86) || defined(OPENSSL_X86_64)
/* GFp_ia32cap_P contains the Intel CPUID bits when running on an x86 or
Expand Down
1 change: 1 addition & 0 deletions mk/ring.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ RING_SRCS = $(addprefix $(RING_PREFIX), \
crypto/bn/random.c \
crypto/bn/shift.c \
crypto/cipher/e_aes.c \
crypto/cpu.c \
crypto/crypto.c \
crypto/curve25519/curve25519.c \
crypto/ec/ecp_nistz.c \
Expand Down
3 changes: 1 addition & 2 deletions src/c.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ macro_rules! define_metrics_tests {
// We can't use `size_t` because we need to test that our
// definition of `size_t` is correct using this code! We use `u16`
// because even 8-bit and 16-bit microcontrollers have no trouble
// with it, and because `u16` is always as smaller or smaller than
// with it, and because `u16` is always as small as or smaller than
// `usize`.
static $c_align: u16;
static $c_size: u16;
Expand Down Expand Up @@ -91,7 +91,6 @@ define_type!(long, i32, test_long_metrics, GFp_long_align, GFp_long_size,
define_type!(long, i64, test_long_metrics, GFp_long_align, GFp_long_size,
"The C `long` type. Equivalent to `libc::c_long`.");


define_type!(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a spurious change that should be undone or done in a separate commit specific to fixing formatting.

size_t, usize, test_size_t_metrics, GFp_size_t_align, GFp_size_t_size,
"The C `size_t` type from `<stdint.h>`.
Expand Down
Loading