[ASAN/AArch64] Fix kernel_old_Xid_t type

[christophe.lyon]

Summary

Some people have complained because libsanitizer for AArch64 no longer builds when using "old" kernel headers.

Indeed, the kernel was modified here:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34c65c4

In practice, I've noticed that the change appeared in linux-3.15.3,
but users have reported using 3.10 and 3.14 stable ones on aarch64
machines.

I want to fix this, and I have a patch ready, but it does not seem to
follow the libsanitizer conventions:
in sanitizer_platform_limits_posix.h, we currently have:
#if defined(powerpc) || defined(aarch64) || defined(mips)

typedef unsigned int __sanitizer___kernel_old_uid_t;
typedef unsigned int __sanitizer___kernel_old_gid_t;

#else

typedef unsigned short __sanitizer___kernel_old_uid_t;
typedef unsigned short __sanitizer___kernel_old_gid_t;

#endif

Why not simply have an unconditional
#if SANITIZER_LINUX
#include <linux/posix_types.h>
#endif

typedef kernel_old_uid_t sanitizer___kernel_old_uid_t;
typedef kernel_old_gid_t sanitizer___kernel_old_gid_t;

(I don't know what the corresponding FREEBSD code would be though).

Sorry if the question is obvious, but why are many types/sizes
hardcodes in sanitizer_platform_limits_posix.h rather than inherited?

Thanks,

Christophe.

[christophe.lyon]

I cannot test other architectures, but this inclusion of linux/posix_types.h would lead to further cleanup in sanitizer_platform_limits_posix.h, starting with the types being discussed here: kernel_old_gid_t and kernel_old_uid_t

Only:
typedef kernel_old_uid_t sanitizer___kernel_old_uid_t;
typedef kernel_old_gid_t sanitizer___kernel_old_gid_t;
would probably be sufficient for all architectures, instead of the current:
#if defined(powerpc) || defined(mips)

typedef unsigned int __sanitizer___kernel_old_uid_t;
typedef unsigned int __sanitizer___kernel_old_gid_t;

#else

typedef unsigned short __sanitizer___kernel_old_uid_t;
typedef unsigned short __sanitizer___kernel_old_gid_t;

#endif

[kcc]

We deliberately do not include system headers into this file.
I know this is causing a maintenance nightmare, but other choice are worse.

[christophe.lyon]

OK, I see.

I am not aware of any way of querying which definition should be used.
Maybe checking the kernel version, but I'm not sure if that's really supposed to be done this way.

[kcc]

Checking the kernel version in an #if would be fine.
Also, please add a static check to ensure that the definition is correct at lib/sanitizer_common/sanitizer_platform_limits_linux.cc,
this is the file where we can include system headers.

[christophe.lyon]

According to people working in the kernel, it is expected that the patch
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34c65c4
has been or will be ported to all stable kernel branches.

The makes a test against kernel version impracticable.

[eugenis]

http://llvm.org/bugs/show_bug.cgi?id=21476

Any ideas to how test this w/o relying on the kernel version?

[christophe.lyon]

In D6026#6, @kcc wrote:

We deliberately do not include system headers into this file.

Could you explain the reasons for this choice in more detail?
Maybe there is a design documentation publicly available?

I know this is causing a maintenance nightmare, but other choice are worse.

What other choices are you thinking of?

[christophe.lyon]

In D6026#11, @eugenis wrote:

http://llvm.org/bugs/show_bug.cgi?id=21476

Any ideas to how test this w/o relying on the kernel version?

As I suggested, rely on the kernel headers to import the definitions in use.

Or, add some form of configure tests to detect the properties of the target.

FYI, I've been informed of a similar problem in glibc where the patch
https://sourceware.org/ml/libc-alpha/2014-10/msg00558.html
now causes problems in sanitizer_platform_limits_posix.h

[eugenis]

We decided against inclusion of system headers into sanitizer interceptors because interceptors define functions with the same names as system functions (that's how interception works on linux). Such functions must have exactly the same signature to avoid compilation errors. Different version of libc define, for example, strchr with different signatures (with or without const on some of the arguments).

As for this aarch64 issue, my understanding is that there is no actual ABI change here. Just a bugfix in kernel headers. The actual syscall implementation has always been using 16-bit Xid_t. True?

In that case we could just relax the compile-time check under #ifdef aarch64. WDYT?

[christophe.lyon]

This also relates to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59009.

If we are not going to include <sys/syscall.h> to get the list of supported syscalls on the current platform, does this mean that I should insert a code fragment to mimic what is suggested in the mentioned bugzilla:
#if target is arm32 || x86-32 || sparc32 || sh || m68k
and libc5 || glibc-2.0 || glibc-2.1 || (glibc-2.2 to 2.15 built against linux-2.2 kernel headers)
#define SOME_PROPERTY
#endif
....
#if SOME_PROPERTY
PRE_SYSCALL(chown16)(const void *filename, long user, long group) {
...
#endif
and similarly for every *16 syscall in sanitizer_common_syscalls.inc
and sanitizer_platform_limits_posix.cc

but then again, how to check the glibc version in these 2 .inc files?

[christophe.lyon]

Here is an attempt to make a cleaner fix.

Since I believe we don't want to include any kernel/libc header in sanitizer_common_syscalls.inc I only matched the target filter described by Arnd (and I only include the targets currently supported by asan).

[rengolin]

Thanks Christophe,

This looks a lot cleaner and sensible. Kostya, does that look good?

cheers,
--renato

[kcc]

LGTM

[rengolin]

Committed in r230324

lib/sanitizer_common/sanitizer_common_syscalls.inc

 
 POST_SYSCALL(fchown)(long res, long fd, long user, long group) {}
 
+#if SANITIZER_USES_UID16_SYSCALLS
 PRE_SYSCALL(chown16)(const void *filename, long user, long group) {
   if (filename)
     PRE_READ(filename,

 PRE_SYSCALL(getegid16)() {}
 
 POST_SYSCALL(getegid16)(long res) {}
+#endif // SANITIZER_USES_UID16_SYSCALLS
 
 PRE_SYSCALL(utime)(void *filename, void *times) {}
 

lib/sanitizer_common/sanitizer_platform.h

 # endif
 #endif
 
+// udi16 syscalls can only be used when the following conditions are
+// met:
+// * target is one of arm32, x86-32, sparc32, sh or m68k
+// * libc version is libc5, glibc-2.0, glibc-2.1 or glibc-2.2 to 2.15
+//   built against > linux-2.2 kernel headers
+// Since we don't want to include libc headers here, we check the
+// target only.
+#if defined(__arm__) || SANITIZER_X32 || defined(__sparc__)
+#define SANITIZER_USES_UID16_SYSCALLS 1
+#else
+#define SANITIZER_USES_UID16_SYSCALLS 0
+#endif
+
 #ifdef __mips__
 # define SANITIZER_POINTER_FORMAT_LENGTH FIRST_32_SECOND_64(8, 10)
 #else

lib/sanitizer_common/sanitizer_platform_limits_posix.cc

 CHECK_TYPE_SIZE(__kernel_uid_t);
 CHECK_TYPE_SIZE(__kernel_gid_t);
 
-#if !defined(__aarch64__)
+#if SANITIZER_USES_UID16_SYSCALLS
 CHECK_TYPE_SIZE(__kernel_old_uid_t);
 CHECK_TYPE_SIZE(__kernel_old_gid_t);
 #endif