llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.93k stars 11.94k forks source link

[clang] KCFI hash mismatches with anonymous structs and qualified typedefs #106593

Open cme opened 2 months ago

cme commented 2 months ago

Compiling the following files with -fsanitize=kcfi enabled incorrectly causes a KCFI violation, on current main as well as llvm16:

kcfi_anon_struct.h:

typedef volatile struct { int a; int b; } A;

kcfi_anon_struct_a.c:

typedef volatile struct { int a; int b; } B;
#include "kcfi_anon_struct.h"

B getB() { B v = { 0, 0 }; return v; }

extern void (*callee)(A *a);

int main(int argc, char *argv[]) {
  A a;
  callee(&a);
  B b = getB();
  b.a = a.a;
  return 0;
}

kcfi_anon_struct_b.c:

#include "kcfi_anon_struct.h"
void f(A *a) {
    a->b = a->a + 8;
}
void (*callee)(A *a) = f;

Compiling with

clang -fsanitize=kcfi kcfi_anon_struct_a.c kcfi_anon_struct_b.c

will produce an executable that fails with a KCFI hash mismatch.

This happens because the name mangling used to create the KCFI typeid hash uses '$_id' with a compilation-unique integer identifier in place of the anonymous struct. In compiling kcfi_anon_struct_a.c, B gets the identified $_0 and A gets the identifier $_1 so mangling the type for callee gets the string _ZTSFvPV3$_1E. In compiling kcfi_anon_struct_b.c there's no definition of B so A gets theidentified $_0 and so the mangled type for callee is _ZTSFvPV3$_0E, the typeid hashes differ between main() and f() and a trap is hit when it's executed.

When there's an unqualified typedef of an anonymous struct, say

typedef struct { int a; int b; } A;

the type name is used in the mangled name, retrieved by TagDecl::getTypedefNameForAnonDecl(). Qualified types fail the hasExtInfo() check here and so return nullptr.

Presumably the same issue exists if anonymous structs are used without any typedef, so while making getTypedefNameForAnonDecl return the names for qualified typedefs as well would solve this particular test case, it doesn't solve the general issue.

shafik commented 2 months ago

CC @samitolvanen

samitolvanen commented 2 months ago

This doesn't seem to be KCFI specific, Clang CFI uses the same mangling scheme and a quick test suggests it has the same problem:

$ clang -fsanitize=cfi -fno-sanitize-trap=cfi -fvisibility=default -flto kcfi_anon_struct_a.c kcfi_anon_struct_b.c
$ ./a.out 
kcfi_anon_struct_a.c:10:3: runtime error: control flow integrity check for type 'void (volatile struct (unnamed at ./kcfi_anon_struct.h:1:18) *)' failed during indirect function call
(a.out+0x2d2e0): note: f defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior kcfi_anon_struct_a.c:10:3 in 

@pcc any thoughts?

cme commented 2 months ago

Ah. Yes, if CFI also uses the same mangling, it will have the same issue.

pcc commented 2 months ago

When an anonymous struct is defined, it isn't generally possible to refer to the same struct by writing out the same struct definition again in the same translation unit (which is a prerequisite to making the types match for CFI purposes). That's because C/C++ do not have structural typing.

I thought that it might be possible to refer to an anonymous struct declared without a typedef, e.g. by using decltype in C++:

extern struct { int i; } foo;

void bar(decltype(foo));

void *p = (void *)bar;

But Clang rejects this example:

dt.cpp:3:6: error: function 'bar' is used but not defined in this translation unit, and cannot be defined in any other translation unit because its type does not have linkage
void bar(decltype(foo));
     ^
dt.cpp:5:19: note: used here
void *p = (void *)bar;
                  ^

So if this type of situation is not intended to be possible, maybe the right fix here is to just make getTypedefNameForAnonDecl look through qualifiers.

cme commented 2 months ago

Excellent! Looks like it's a little more complicated than I first thought in getTypedefForAnonDecl though as we don't actually have a TypedefNameDecl at that point, so probably have to do something in ::mangleType() before getting to the unqualified anonymous struct