This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Implement checksumming functions
ClosedPublic

Authored by cryptoad on Mar 7 2019, 3:37 PM.

Details

Summary

This CL implements the checksumming functions. This departs from the
current Scudo code in one aspect: the software version is no longer
CRC32 but a BSD checksum. This is because the software CRC32 was too
impactful in terms of performances, the BSD checksum has no array
lookup which is better (and saves 1KB of data).

As with the current version, we only flip the CRC compiler flag for
a single compilation unit by default, to allow for a library compiled
with HW CRC32 to work on a system without HW CRC32.

There is some platform & hardware specific code in those files, but
since departs from a mere platform split, it felt right to me to have
it that way.

Diff Detail

Event Timeline

cryptoad created this revision.Mar 7 2019, 3:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 7 2019, 3:37 PM
Herald added subscribers: Restricted Project, jdoerfert, jfb and 2 others. · View Herald Transcript
mgorny added a comment.Mar 7 2019, 8:07 PM

If you have to bundle a new hash, have you considered using one of the modern fast hashes (such as xxHash, CityHash, …)?

If you have to bundle a new hash, have you considered using one of the modern fast hashes (such as xxHash, CityHash, …)?

For Scudo, we need something that will checksum 96bits on 32-bit & 160 bits on 64-bit with as few cycles as possible, and that can (hopefully) detect corruption.
CityHash & xxHash are, in my opinion, way too complex for our use case, and likely slower than the BSD checksum for software. HW CRC32 ends up being a couple of asm instructions and is super quick for our purpose.
So in the end, I do not think that those hashes would be worth using here.

Could this be done with a target("sse4.2") function attribute instead of cmake conditions?

hctim added a comment.Mar 8 2019, 11:20 AM

Is it possible to update the document at llvm/docs/ScudoHardenedAllocator.rst to reflect these changes?

morehouse added inline comments.Mar 8 2019, 11:51 AM
lib/scudo/standalone/checksum.cc
69

Perhaps comments with the matching ifdefs would be helpful here, especially since this last one matches way up at line 27.

lib/scudo/standalone/checksum.h
11

SCUDO_CHECKSUM_H

21

The outer if-defined looks unnecessary.

39

s/That/BSD

46

Are these static_casts necessary?

Could this be done with a target("sse4.2") function attribute instead of cmake conditions?

I have had no success with early tries with attribute target, mostly due to discrepancies between clang & gcc (depending on versions). Like target("+crc") doesn't seem to work with my clangs (https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html).
Additional concern about this is the resolver function that is used on every invocation of the function (at least with g++).
While this could be addressed with a function pointer, I am trying to avoid a writable function pointer in a critical path of the allocator as it could be leveraged for exploitation purposes.

Is it possible to update the document at llvm/docs/ScudoHardenedAllocator.rst to reflect these changes?

In theory, this still doc still applies to the "current" version of Scudo which is coexisting with the standalone one.
A solution would be to write a standalone-doc where this would go, but as of now the repo doesn't have a functional standalone allocator so this might be too early.

cryptoad updated this revision to Diff 189908.Mar 8 2019, 12:21 PM
cryptoad marked 6 inline comments as done.

Addressing review comments.

cryptoad added inline comments.Mar 8 2019, 12:36 PM
lib/scudo/standalone/checksum.h
46

Apparently the result of the operations were promoted (I wouldn't have thought) and lead to the following (with strict W options):

error: implicit conversion loses integer precision: 'int' to 'scudo::u16' (aka 'unsigned short') [-Werror,-Wimplicit-int-conversion]
error: conversion from 'scudo::uptr' {aka 'long unsigned int'} to 'scudo::u16' {aka 'short unsigned int'} may change value [-Werror=conversion]
This revision is now accepted and ready to land.Mar 8 2019, 12:38 PM
This revision was automatically updated to reflect the committed changes.