[ASAN] Fix validation size for dirent on FreeBSD

[justincady]

Summary

Typically the size required to represent a dirent is stored in d_reclen. But
this not always the case for FreeBSD (for example, when walking a directory
over NFS).

This leads to ASAN false positives for scandir and similar functions. Because
ASAN uses d_reclen for the range to validate, it can overrun when d_reclen is
incorrect (too large).

This change adds a SANITIZER_DIRSIZ macro which fixes the dirent size calculation
for FreeBSD. Other platforms continue to use d_reclen.

[justincady]

Some notes on testing:

• Without this diff, running the sanitized binary created by scandir.c fails on FreeBSD 13.1 when run on an NFS-mounted path (e.g. -DTEMP_DIR is set to said path). It passes with the diff in place.
• I ran ninja check-asan with this diff on both FreeBSD 13.1 and Linux (Ubuntu). There are some pre-existing failures on FreeBSD which were identical before and after my change.

[fmayer]

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

449 ↗

(On Diff #526174)

random driveby comment: could you explain the formula below a bit in a this comment as well?

[justincady]

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

449 ↗

(On Diff #526174)

Sure. This formula follows FreeBSD's DIRSIZ implementation to ensure we can fit the name (plus round up): https://github.com/freebsd/freebsd-src/blob/main/sys/sys/dirent.h#L113

I'd love to just include that header directly on FreeBSD and use it. But with the intentional avoidance of system headers and all of the __sanitizer_ versions of FreeBSD data structures, I didn't see a straightforward way to make that happen.

I can add a note here.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

447 ↗

(On Diff #526174)

can you please make this a function, with implementation in CPP files
freebsd one can probably include <dirent.h> and use DIRSIZ

[justincady]

Convert to a __sanitizer_dirsiz function to use FreeBSD's _GENERIC_DIRSIZ

This address the outstanding comments. We no longer have to reimplement functionality, and the required FreeBSD header is available.

All other platforms will continue to use d_reclen.

[justincady]

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

447 ↗

(On Diff #526174)

Done. I switched to using a function in the latest revision and now can use the appropriate FreeBSD DIRSIZ macro.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h

429 ↗

(On Diff #526666)

sanitizer_platform_limits_posix.h hear __sanitizer_dirent is better place

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.h

260

if you move first implementation into sanitizer_platform_limits_posix.h you can drop template<> here

[justincady]

compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h

429 ↗

(On Diff #526666)

I would have thought so too, but sanitizer_platform_limits_posix.h is surrounded by #if SANITIZER_LINUX || SANITIZER_APPLE. So putting __sanitizer_dirsiz there would mean either breaking that convention or repeating this function in sanitizer_platform_limits_netbsd.h and sanitizer_platform_limits_solaris.h (in addition to sanitizer_platform_limits_freebsd.h). All of those platforms intercept scandir.

Putting the function here makes it available to all platforms that will use it without redeclaration or redefinition (plus accounting for the dirent64 variants), and allows FreeBSD to specialize for its requirements. Though, it might be an unconventional location for code like this.

Could you please expand on your reasoning as to why sanitizer_platform_limits_posix.h would be better? I'm happy to make the change (even redeclaring/redefining per platform if you think it's preferable), but because of those downsides I want to first make sure I understand what you are suggesting.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h

429 ↗

(On Diff #526666)

Still not in the defs.h
maybe sanitizer_posix.h

[justincady]

• Move __sanitizer_dirsiz out of sanitizer_internal_defs.h
• Revert to using a macro for non-FreeBSD platforms now that there's no overloading

[justincady]

compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h

429 ↗

(On Diff #526666)

Got it. Moved to sanitizer_posix.h in the latest revision.

[vitalybuka]

compiler-rt/test/asan/TestCases/scandir.c

5 ↗

(On Diff #528494)

probably better to have this test in sanitizer_common, as the change affects other sanitizers

No need to FileCheck as the return 0 is a sign of the successes

[justincady]

• Move the test into sanitizer_common as suggested
• Make the FreeBSD __sanitizer_dirsiz parameter const. This was revealed when running the test under sanitizer_common on FreeBSD, where MSAN required it.
• Rebase

[emaste]

LGTM

[ro]

The new test FAILs on the Solaris/amd64 buildbot:

FAIL: SanitizerCommon-asan-i386-SunOS :: scandir.c (74789 of 76009)
[...]
=================================================================
==10526==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xfc2007a0 at pc 0x080c11df bp 0xfeffe608 sp 0xfeffe1d0
WRITE of size 4354 at 0xfc2007a0 thread T0
    #0 0x80c11de in scandir /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:3973:7
    #1 0x81268d4 in main /vol/llvm/src/llvm-project/dist/compiler-rt/test/sanitizer_common/TestCases/scandir.c:16:15
    #2 0x8087385 in _start (/var/llvm/dist-amd64-release-stage2-A-flang/tools/clang/stage2-bins/projects/compiler-rt/test/sanitizer_common/asan-i386-SunOS/Output/scandir.c.tmp+0x8087385)

0xfc2007a0 is located 0 bytes after 16-byte region [0xfc200790,0xfc2007a0)
allocated by thread T0 here:
    #0 0x80f0320 in malloc /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
    #1 0xfdf6f395 in _scandir (/lib/libc.so.1+0xef395)
    #2 0x80c0f08 in scandir /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:3964:13
    #3 0x81268d4 in main /vol/llvm/src/llvm-project/dist/compiler-rt/test/sanitizer_common/TestCases/scandir.c:16:15
    #4 0x8087385 in _start (/var/llvm/dist-amd64-release-stage2-A-flang/tools/clang/stage2-bins/projects/compiler-rt/test/sanitizer_common/asan-i386-SunOS/Output/scandir.c.tmp+0x8087385)

SUMMARY: AddressSanitizer: heap-buffer-overflow /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:3973:7 in scandir
Shadow bytes around the buggy address:
  0xfc200500: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0xfc200580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0xfc200600: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0xfc200680: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0xfc200700: fa fa fa fa fa fa 00 00 fa fa 00 00 fa fa 00 00
=>0xfc200780: fa fa 00 00[fa]fa 00 00 fa fa fa fa fa fa fa fa
  0xfc200800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0xfc200880: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0xfc200900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0xfc200980: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0xfc200a00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==10526==ABORTING

At the very least, the test contains a syntax error: REQUIRES needs a colon to make it anything but a random comment.

However, I wonder it there's anything else to be done on non-Linux/FreeBSD targets to make the scandir interceptor useful/non-broken, especially given that Solaris <sys/dirent.h> declares struct dirent as

typedef struct dirent {
	ino_t		d_ino;		/* "inode number" of entry */
	off_t		d_off;		/* offset of disk directory entry */
	unsigned short	d_reclen;	/* length of this record */
	char		d_name[1];	/* name of file */
} dirent_t;

d_name[1] is not currently included in sanitizer_platform_limits_solaris.h's struct __sanitizer_dirent declaration, but that form is explicitly allowed by XPG7.

[justincady]

@ro I apologize for the breakage. Thank you for diagnosing the syntax error. I'll send a review to correct that.

[justincady]

See: https://reviews.llvm.org/D152711

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

 
 INTERCEPTOR(__sanitizer_dirent *, readdir, void *dirp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, readdir, dirp);
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   __sanitizer_dirent *res = REAL(readdir)(dirp);
-  if (res) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, res, res->d_reclen);
+  if (res)
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, res, __sanitizer_dirsiz(res));
   return res;
 }
 
 INTERCEPTOR(int, readdir_r, void *dirp, __sanitizer_dirent *entry,
             __sanitizer_dirent **result) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, readdir_r, dirp, entry, result);
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(readdir_r)(dirp, entry, result);
   if (!res) {
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
     if (*result)
-      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *result, (*result)->d_reclen);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *result, __sanitizer_dirsiz(*result));
   }
   return res;
 }
 
 #define INIT_READDIR                  \
   COMMON_INTERCEPT_FUNCTION(opendir); \
   COMMON_INTERCEPT_FUNCTION(readdir); \
   COMMON_INTERCEPT_FUNCTION(readdir_r);
 #else
 #define INIT_READDIR
 #endif
 
 #if SANITIZER_INTERCEPT_READDIR64
 INTERCEPTOR(__sanitizer_dirent64 *, readdir64, void *dirp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, readdir64, dirp);
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   __sanitizer_dirent64 *res = REAL(readdir64)(dirp);
-  if (res) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, res, res->d_reclen);
+  if (res)
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, res, __sanitizer_dirsiz(res));
   return res;
 }
 
 INTERCEPTOR(int, readdir64_r, void *dirp, __sanitizer_dirent64 *entry,
             __sanitizer_dirent64 **result) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, readdir64_r, dirp, entry, result);
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(readdir64_r)(dirp, entry, result);
   if (!res) {
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
     if (*result)
-      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *result, (*result)->d_reclen);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *result, __sanitizer_dirsiz(*result));
   }
   return res;
 }
 #define INIT_READDIR64                  \
   COMMON_INTERCEPT_FUNCTION(readdir64); \
   COMMON_INTERCEPT_FUNCTION(readdir64_r);
 #else
 #define INIT_READDIR64

 typedef int (*scandir_compar_f)(const struct __sanitizer_dirent **,
                                 const struct __sanitizer_dirent **);
 
 static THREADLOCAL scandir_filter_f scandir_filter;
 static THREADLOCAL scandir_compar_f scandir_compar;
 
 static int wrapped_scandir_filter(const struct __sanitizer_dirent *dir) {
   COMMON_INTERCEPTOR_UNPOISON_PARAM(1);
-  COMMON_INTERCEPTOR_INITIALIZE_RANGE(dir, dir->d_reclen);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(dir, __sanitizer_dirsiz(dir));
   return scandir_filter(dir);
 }
 
 static int wrapped_scandir_compar(const struct __sanitizer_dirent **a,
                                   const struct __sanitizer_dirent **b) {
   COMMON_INTERCEPTOR_UNPOISON_PARAM(2);
   COMMON_INTERCEPTOR_INITIALIZE_RANGE(a, sizeof(*a));
-  COMMON_INTERCEPTOR_INITIALIZE_RANGE(*a, (*a)->d_reclen);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(*a, __sanitizer_dirsiz(*a));
   COMMON_INTERCEPTOR_INITIALIZE_RANGE(b, sizeof(*b));
-  COMMON_INTERCEPTOR_INITIALIZE_RANGE(*b, (*b)->d_reclen);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(*b, __sanitizer_dirsiz(*b));
   return scandir_compar(a, b);
 }
 
 INTERCEPTOR(int, scandir, char *dirp, __sanitizer_dirent ***namelist,
             scandir_filter_f filter, scandir_compar_f compar) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, scandir, dirp, namelist, filter, compar);
   if (dirp) COMMON_INTERCEPTOR_READ_RANGE(ctx, dirp, internal_strlen(dirp) + 1);
   scandir_filter = filter;
   scandir_compar = compar;
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(scandir)(dirp, namelist,
                           filter ? wrapped_scandir_filter : nullptr,
                           compar ? wrapped_scandir_compar : nullptr);
   scandir_filter = nullptr;
   scandir_compar = nullptr;
   if (namelist && res > 0) {
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, namelist, sizeof(*namelist));
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *namelist, sizeof(**namelist) * res);
     for (int i = 0; i < res; ++i)
       COMMON_INTERCEPTOR_WRITE_RANGE(ctx, (*namelist)[i],
-                                     (*namelist)[i]->d_reclen);
+                                     __sanitizer_dirsiz((*namelist)[i]));
   }
   return res;
 }
 #define INIT_SCANDIR COMMON_INTERCEPT_FUNCTION(scandir);
 #else
 #define INIT_SCANDIR
 #endif
 
 #if SANITIZER_INTERCEPT_SCANDIR64
 typedef int (*scandir64_filter_f)(const struct __sanitizer_dirent64 *);
 typedef int (*scandir64_compar_f)(const struct __sanitizer_dirent64 **,
                                   const struct __sanitizer_dirent64 **);
 
 static THREADLOCAL scandir64_filter_f scandir64_filter;
 static THREADLOCAL scandir64_compar_f scandir64_compar;
 
 static int wrapped_scandir64_filter(const struct __sanitizer_dirent64 *dir) {
   COMMON_INTERCEPTOR_UNPOISON_PARAM(1);
-  COMMON_INTERCEPTOR_INITIALIZE_RANGE(dir, dir->d_reclen);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(dir, __sanitizer_dirsiz(dir));
   return scandir64_filter(dir);
 }
 
 static int wrapped_scandir64_compar(const struct __sanitizer_dirent64 **a,
                                     const struct __sanitizer_dirent64 **b) {
   COMMON_INTERCEPTOR_UNPOISON_PARAM(2);
   COMMON_INTERCEPTOR_INITIALIZE_RANGE(a, sizeof(*a));
-  COMMON_INTERCEPTOR_INITIALIZE_RANGE(*a, (*a)->d_reclen);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(*a, __sanitizer_dirsiz(*a));
   COMMON_INTERCEPTOR_INITIALIZE_RANGE(b, sizeof(*b));
-  COMMON_INTERCEPTOR_INITIALIZE_RANGE(*b, (*b)->d_reclen);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(*b, __sanitizer_dirsiz(*b));
   return scandir64_compar(a, b);
 }
 
 INTERCEPTOR(int, scandir64, char *dirp, __sanitizer_dirent64 ***namelist,
             scandir64_filter_f filter, scandir64_compar_f compar) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, scandir64, dirp, namelist, filter, compar);
   if (dirp) COMMON_INTERCEPTOR_READ_RANGE(ctx, dirp, internal_strlen(dirp) + 1);
   scandir64_filter = filter;
   scandir64_compar = compar;
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res =
       REAL(scandir64)(dirp, namelist,
                       filter ? wrapped_scandir64_filter : nullptr,
                       compar ? wrapped_scandir64_compar : nullptr);
   scandir64_filter = nullptr;
   scandir64_compar = nullptr;
   if (namelist && res > 0) {
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, namelist, sizeof(*namelist));
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *namelist, sizeof(**namelist) * res);
     for (int i = 0; i < res; ++i)
       COMMON_INTERCEPTOR_WRITE_RANGE(ctx, (*namelist)[i],
-                                     (*namelist)[i]->d_reclen);
+                                     __sanitizer_dirsiz((*namelist)[i]));
   }
   return res;
 }
 #define INIT_SCANDIR64 COMMON_INTERCEPT_FUNCTION(scandir64);
 #else
 #define INIT_SCANDIR64
 #endif
 

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.h

 struct __sanitizer_dirent {
 #  if defined(__INO64)
   unsigned long long d_fileno;
   unsigned long long d_off;
 #  else
   unsigned int d_fileno;
 #  endif
   unsigned short d_reclen;
-  // more fields that we don't care about
+  u8 d_type;
+  u8 d_pad0;
+  u16 d_namlen;
+  u16 d_pad1;
+  char d_name[256];
 };
 
+u16 __sanitizer_dirsiz(const __sanitizer_dirent *dp);
+
 // 'clock_t' is 32 bits wide on x64 FreeBSD
 typedef int __sanitizer_clock_t;
 typedef int __sanitizer_clockid_t;
 
 #  if defined(_LP64) || defined(__x86_64__) || defined(__powerpc__) || \
       defined(__mips__)
 typedef unsigned __sanitizer___kernel_uid_t;
 typedef unsigned __sanitizer___kernel_gid_t;

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.cpp

uptr __sanitizer_in_addr_sz(int af) {
   if (af == AF_INET)
     return sizeof(struct in_addr);
   else if (af == AF_INET6)
     return sizeof(struct in6_addr);
   else
     return 0;
 }
 
+// For FreeBSD the actual size of a directory entry is not always in d_reclen.
+// Use the appropriate macro to get the correct size for all cases (e.g. NFS).
+u16 __sanitizer_dirsiz(const __sanitizer_dirent *dp) {
+  return _GENERIC_DIRSIZ(dp);
+}
+
 unsigned struct_ElfW_Phdr_sz = sizeof(Elf_Phdr);
 int glob_nomatch = GLOB_NOMATCH;
 int glob_altdirfunc = GLOB_ALTDIRFUNC;
 const int wordexp_wrde_dooffs = WRDE_DOOFFS;
 
 unsigned path_max = PATH_MAX;
 
 int struct_ttyent_sz = sizeof(struct ttyent);

compiler-rt/lib/sanitizer_common/sanitizer_posix.h

 // Platforms should implement at most one of these.
 // 1. Provide a pre-decorated file descriptor to use instead of an anonymous
 // mapping.
 int GetNamedMappingFd(const char *name, uptr size, int *flags);
 // 2. Add name to an existing anonymous mapping. The caller must keep *name
 // alive at least as long as the mapping exists.
 void DecorateMapping(uptr addr, uptr size, const char *name);
 
+#  if !SANITIZER_FREEBSD
+#    define __sanitizer_dirsiz(dp) ((dp)->d_reclen)
+#  endif
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_POSIX
 
 #endif  // SANITIZER_POSIX_H

compiler-rt/test/sanitizer_common/TestCases/scandir.c

+// REQUIRES (linux && !android) || freebsd
+
+// RUN: rm -rf %t-dir
+// RUN: mkdir -p %t-dir
+// RUN: touch %t-dir/a %t-dir/b %t-dir/c
+
+// RUN: %clang %s -DTEMP_DIR='"'"%t-dir"'"' -o %t && %run %t 2>&1
+
+#include <dirent.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+int main(int argc, char **argv) {
+  struct dirent **dirpp = NULL;
+  int count = scandir(TEMP_DIR, &dirpp, NULL, NULL);
+  fprintf(stderr, "count is %d\n", count);
+  if (count >= 0) {
+    for (int i = 0; i < count; ++i) {
+      fprintf(stderr, "found %s\n", dirpp[i]->d_name);
+      free(dirpp[i]);
+    }
+    free(dirpp);
+  }
+  return 0;
+}