Use getauxval on Android with API level > 18

[larsimmisch-adobe]

One of the android applications we work on received analytics that devices of the device family Oppo A37x were crashing with SIGILL when trying to load a bundled libcrypto.so built from OpenSSL_1_1_1c.

This device family has a SnapDragon 410 processor: https://www.qualcomm.com/products/snapdragon-processors-410, which is based upon an ARM Cortex A53

These crashes were fixed by using the system-supplied getauxval function.

[petrovr]

Kurt Roeckx wrote:

kroeckx approved this pull request.

- 1 preferred is runtime detection . OpenSSL has long history with this approach. Roumen

[kroeckx]

This is runtime detection. We ask the Linux kernel what it detected, rather than running some instructions to see if the CPU supports that instruction or not. We prefer to ask the kernel. We already use getauxval() if you have a recent enough glibc.

I'm not sure when the SIGILL is being seen, but I assume that either it's running in a debugger, or Android somehow doesn't allow us to catch the SIGILL.

[petrovr]

Kurt Roeckx wrote:

This is runtime detection. We ask the Linux kernel what it detected, rather than running some instructions to see if the CPU supports that instruction or not. We prefer to ask the kernel. We already use getauxval() if you have a recent enough glibc. I'm not sure with the SIGILL is being seen, but I assume that either it's running in a debugger, or Android somehow doesn't allow us to catch the SIGILL.

Function getauxval() does not exist in API < 18. My point it to detect getauxval() at run time.

[petrovr]

Tim Hudson wrote:

t-j-h approved this pull request.

My 2$: Android is not Linux distribution! On Linux where C library and perhaps kernel remain mostly the same for a release and so application could be build with #ifdef feature. On Android application target many SDK(=API =Android releases) so dynamic check for function is must suitable. This solution OpenSSL uses on Microsoft Windows and more recently on Linux for entropy. Regards Roumen

[levitte]

@petrovr, PR welcome!

[t-j-h]

This check is perfectly valid in this context - when building an application at that API level the syscall is available. Sure you could add a dynamic solution to detect at runtime - and naturally a PR with that in it would be welcome - although testing that would be a "fun" exercise to organise. However this improves the situation for those targeting most currently active android releases.

Keep in mind that Android API level 18 is from 2013 (Aka Android release 4.3).

[petrovr]

Richard Levitte wrote:

@petrovr, PR welcome!

:) which model : based on (1) global lookup (mallopt) or (2) weak alias (getentropy) ? One is part of dso module and another is code related to random data. Regards, Petrov

[levitte]

which model

Whichever you think is more suitable. It really depends on if you're going to code this for Android only (where it's probably easier to specify weak symbols consistently) or in a more generic way (it may be easier to check for symbols with DSO functions).

Your pick

[petrovr]

The test show that "global lookup" does not work - YAAL (yet another android limitation).
Solution with weak alias is fine. Test case:

#include <stdio.h>
#include <errno.h>

#  define HWCAP                  16


static unsigned long
my1_getauxval(unsigned long type) {
  extern unsigned long getauxval(unsigned long type) __attribute__((weak));
  if (getauxval != NULL)  return getauxval(type) ;
  /**/
  errno = ENOENT;
  return 0;
}

#define getauxval my1_getauxval
void test1() {
   unsigned long flags = getauxval(HWCAP);
   fprintf(stderr, "1-getauxval(HWCAP): 0x%lx, errno: %d\n", flags, errno);
}
#undef getauxval


#include <dlfcn.h>
static unsigned long
my2_getauxval(unsigned long type) {
  static unsigned long (*getauxval_f)(unsigned long type) = NULL;
  static int init = 1;

  if (init) {
    void *handle;
    init = 0;
    handle = dlopen(NULL, RTLD_LOCAL);
    if (handle != NULL) {
      union {
          void *v;
          unsigned long (*f)(unsigned long);
      } t;
      t.v = dlsym(handle, "getauxval");
      getauxval_f = t.f;
      dlclose(handle);
    }
  }

  if (getauxval_f != NULL)  return getauxval_f(type) ;
  /**/
  errno = ENOENT;
  return 0;
}

#define getauxval my2_getauxval
void test2() {
   unsigned long flags = getauxval(HWCAP);
   fprintf(stderr, "2-getauxval(HWCAP): 0x%lx, errno: %d\n", flags, errno);
}
#undef getauxval


int
main() {
  test1();
  test2();
  return 0;
}

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[kroeckx]

@iamamoose: I think the "PR has been updated" text is confusing. I assume you mean someone commented on it?

[levitte]

Hoooold on!

I just noticed that the commit message marks this as trivial. I'm not sure I agree with that, and would like to hear what others think.

This is NOT ready to merge at this point

[kroeckx]

Why wasn't the CLA: trivial label set earlier?

[iamamoose]

@iamamoose: I think the "PR has been updated" text is confusing. I assume you mean someone commented on it?

It currently means a comment or a label change but should also mean a further push (it just doesn't check for that yet). Do you have any preferred wording "but this PR has had a comment or change" perhaps?

[mattcaswell]

Why wasn't the CLA: trivial label set earlier?

AFAIK there is no automatic labelling if a commit contains "CLA: trivial" in the header.It's purely manual. Perhaps @iamamoose could work his magic...?

[iamamoose]

Why wasn't the CLA: trivial label set earlier?

AFAIK there is no automatic labelling if a commit contains "CLA: trivial" in the header.It's purely manual. Perhaps @iamamoose could work his magic...?

openssl/tools#63

[openssl-machine]

This PR has the label 'hold: cla required' and is stale: it has not been updated in 61 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html

[paulidale]

CLA is in: the commit needs to have it's author changed.

[larsimmisch-adobe]

Reopening PR with Author fixed to match iCLA

[paulidale]

I don't think this should be merged to 1.1.1.

[t8m]

I don't think this should be merged to 1.1.1.

That's IMO debatable. But yeah, it is not a straight bug fix, rather a cleanup thing or improved platform support.

[t8m]

IMO OTC (or OMC? I never know who gives these exception approvals) would have to approve this as an exception to be merged to 1.1.1

[paulidale]

OTC would need to clear this going into 1.1.1.

[t8m]

Also I think this can be merged to master straight away as this was approved looong time ago and the only thing to be resolved was the CLA.

[t8m]

@larsimmisch-adobe if you care about having this change in 1.1.1 could you please open a separate pull request against that branch?

[t8m]

Merged to master. I've reformatted the commit message and supplied proper subject during merging. Thank you for the contribution!

[larsimmisch-adobe]

@t8m It makes sense to have this in 1.1.1, so I have opened #15763.

Thanks!

[petrovr]

Tomáš Mráz wrote:

Merged to master. I've reformatted the commit message and supplied proper subject during merging. Thank you for the contribution!

Newbie! Dear Linux only developers, Android uses Linux kernel but package distribution model is different. I will not enter into details as issue tracking system is not learning tool. Thanks for bloody commit, Roumen Petrov