This is an archive of the discontinued LLVM Phabricator instance.

[CMSE] Clear padding bits of struct/unions/fp16 passed by value
ClosedPublic

Authored by chill on Mar 18 2020, 9:52 AM.

Details

Summary

When passing a value of a struct/union type from secure to non-secure
state (that is returning from a CMSE entry function or passing an
argument to CMSE-non-secure call), there is a potential sensitive
information leak via the padding bits in the structure. It is not
possible in the general case to ensure those bits are cleared by using
Standard C/C++.

This patch makes the compiler emit code to clear such padding
bits. Since type information is lost in LLVM IR, the code generation
is done by Clang.

For each interesting record type, we build a bitmask, in which all the
bits, corresponding to user declared members, are set. Values of
record types are returned by coercing them to an integer. After the
coercion, the coerced value is masked (with bitwise AND) and then
returned by the function. In a similar manner, values of record types
are passed as arguments by coercing them to an array of integers, and
the coerced values themselves are masked.

For union types, we effectively clear only bits, which aren't part of
any member, since we don't know which is the currently active one.
The compiler will issue a warning, whenever a union is passed to
non-secure state.

Values of half-precision floating-point types are passed in the least
significant bits of a 32-bit register (GPR or FPR) with the most
significant bits unspecified. Since this is also a potential leak of
sensitive information, this patch also clears those unspecified bits.

Diff Detail

Event Timeline

chill created this revision.Mar 18 2020, 9:52 AM
chill edited the summary of this revision. (Show Details)

I'm not sure how you expect unions to work correctly; in general, you will end up with uninitialized bits in the union, unless the user takes unusual steps to initialize the union's padding.

chill added a comment.Mar 25 2020, 4:59 AM

I'm not sure how you expect unions to work correctly; in general, you will end up with uninitialized bits in the union, unless the user takes unusual steps to initialize the union's padding.

Indeed, it would not work for all unions, in that sense it's rather a mitigation than a solution; I couldn't think of another approach.

Perhaps, it's worth issuing a warning when a union is passed across the security boundary?

Yes, a warning probably makes sense.

chill updated this revision to Diff 252908.Mar 26 2020, 11:10 AM

Added a warning when passing a union value from secure to non-secure state.
I'd prefer for the diagnostics location to point at the parameter/return type, instead at the attribute, but
couldn't figure out how.

I'd prefer for the diagnostics location to point at the parameter/return type, instead at the attribute, but couldn't figure out how.

Given a TypeSourceInfo, you should be able to find the locations of the return and parameter types for a function or function pointer. You can grab the TypeSourceInfo from a declaration.

That said, it might get a little complicated to figure out where to point in certain edge cases, like a typedef'ed function type.

clang/lib/Sema/SemaDeclAttr.cpp
2002

Please don't use getStorageClass on a function; we have functions for checking linkage.

chill updated this revision to Diff 254798.Apr 3 2020, 7:31 AM

Moved the non-secure union warning form declarations to return statement and call expressions. Easier to
get the source location and handles function types with no prototype.

chill marked an inline comment as done.Apr 3 2020, 7:31 AM

Diagnosing call/return statements seems nicer.

clang/lib/CodeGen/CGCall.cpp
2953

This came up recently elsewhere, but getBitWidthValue() can include padding. Should get the actual width from the CGRecordLayout.

3046

What is the format of the data in the "Bits" vector? Is each element a "char"? It's not explicitly documented anywhere in the patch.

clang/lib/Sema/SemaExpr.cpp
6418 ↗(On Diff #254798)

Do you also need to check for a struct that contains a union?

chill marked an inline comment as done.Apr 6 2020, 9:17 AM
chill added inline comments.
clang/lib/Sema/SemaExpr.cpp
6418 ↗(On Diff #254798)

Right, good point!

Does it make sense to add one more bit is-union-or-contains-a-union to RecordDeclBitfields ?

efriedma added inline comments.Apr 6 2020, 11:34 AM
clang/lib/Sema/SemaExpr.cpp
6418 ↗(On Diff #254798)

Given how rarely we expect to actually check this, probably doesn't make sense to compute it in advance.
Maybe stick it in a map in ASTContext if you think if you need to cache it.

chill marked an inline comment as done.Apr 6 2020, 12:10 PM
chill added inline comments.
clang/lib/Sema/SemaExpr.cpp
6418 ↗(On Diff #254798)

TBH, I'm not so concerned with efficiency, but rather wondering where to put a function to check that property, without duplicating it in the two files, where the check is made.

You can stick a recursive helper function on RecordType/RecordDecl, if that's the concern.

chill updated this revision to Diff 255659.Apr 7 2020, 6:26 AM
chill marked 4 inline comments as done.
efriedma added inline comments.Apr 7 2020, 10:35 AM
clang/lib/AST/Decl.cpp
4400 ↗(On Diff #255659)

I suspect you have to call RecordDecl::getDefinition() somewhere in here. Otherwise you might run into trouble with forward-declared structs. Not completely sure, though.

chill updated this revision to Diff 255915.Apr 8 2020, 12:48 AM
chill marked 2 inline comments as done.Apr 8 2020, 12:50 AM
chill added inline comments.
clang/lib/AST/Decl.cpp
4400 ↗(On Diff #255659)

Fair enough. Probably not an issue with the current usage where we need complete types, but doesn't hurt.

efriedma added inline comments.Apr 8 2020, 11:47 AM
clang/lib/AST/Decl.cpp
4405 ↗(On Diff #255915)

No need to guard the getDefinition() call with isCompleteDefinition(); might as well just do it unconditionally.

4400 ↗(On Diff #255659)

I'd be more concerned with something like this:

typedef struct S S;
void foo(S);
struct S { int x,y; };
chill marked 3 inline comments as done.Apr 8 2020, 11:43 PM
chill added inline comments.
clang/lib/AST/Decl.cpp
4405 ↗(On Diff #255915)

Yeah, the alternative would be to compare the return of getDefinition to this (to prevent infinite recurison),
but I find this variant somewhat more descriptive (it's plain English!) and it also saves a function call.

chill marked an inline comment as done.Apr 8 2020, 11:44 PM
efriedma added inline comments.Apr 9 2020, 12:40 PM
clang/lib/AST/Decl.cpp
4405 ↗(On Diff #255915)

I was thinking you'd just do something like if (const RecordDecl *Def = getDefinition()) { for (const FieldDecl *FD : Def->fields()) { [...] } }

chill updated this revision to Diff 257222.Apr 14 2020, 1:12 AM
chill edited the summary of this revision. (Show Details)
chill marked 2 inline comments as done.
This revision is now accepted and ready to land.Apr 14 2020, 12:08 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 9:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript