Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[ASAN] Fix validation size for dirent on FreeBSD

Authored by justincady on May 26 2023, 1:09 PM.



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.

Diff Detail

Event Timeline

justincady created this revision.May 26 2023, 1:09 PM
justincady requested review of this revision.May 26 2023, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 1:09 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

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 added a subscriber: fmayer.May 26 2023, 1:28 PM
fmayer added inline comments.
449 ↗(On Diff #526174)

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

justincady added inline comments.May 26 2023, 1:41 PM
449 ↗(On Diff #526174)

Sure. This formula follows FreeBSD's DIRSIZ implementation to ensure we can fit the name (plus round up):

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 added inline comments.May 29 2023, 6:21 PM
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

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 marked an inline comment as done.May 30 2023, 9:10 AM
justincady added inline comments.
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 added inline comments.May 30 2023, 6:18 PM
429 ↗(On Diff #526666)

sanitizer_platform_limits_posix.h hear __sanitizer_dirent is better place


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

justincady marked an inline comment as done.May 31 2023, 10:37 AM
justincady added inline comments.
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 added inline comments.Jun 2 2023, 1:20 PM
429 ↗(On Diff #526666)

Still not in the defs.h
maybe sanitizer_posix.h

  • 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 marked an inline comment as done.Jun 5 2023, 10:11 AM
justincady added inline comments.
429 ↗(On Diff #526666)

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

vitalybuka accepted this revision.Jun 6 2023, 4:11 PM
This revision is now accepted and ready to land.Jun 6 2023, 4:11 PM
vitalybuka added inline comments.Jun 6 2023, 4:13 PM
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 updated this revision to Diff 529300.Jun 7 2023, 7:46 AM
justincady marked an inline comment as done.
  • 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
This revision was landed with ongoing or failed builds.Jun 7 2023, 7:59 AM
This revision was automatically updated to reflect the committed changes.
ro added a subscriber: ro.Jun 12 2023, 4:57 AM

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/
    #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/
    #2 0x80c0f08 in scandir /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/../sanitizer_common/
    #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/ 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

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.

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