Page MenuHomePhabricator

Fix interaction between clang and some inline builtins from glibc under _FORTIFY_SOURCE
Needs ReviewPublic

Authored by serge-sans-paille on Dec 4 2020, 7:18 AM.

Details

Summary

Clang considers trivially recursive a function that may call itself, not a function that always call itself.
This leads to some inline definition of fortified libc builtins no being emitted, and thus ignored.

Work around the situation by detecting the pattern and generate a combination of builtin / nobuiltin attributes to have it work as expected.

The basic problem to solve is the compilation of the following code, which exhibits the behavior used in the glibc. Note that currently clang generates a call to wmemcpy ad not __wmemcpy_chk as it should, leading to a less secure code.

typedef long unsigned int size_t;
typedef int wchar_t;

extern wchar_t *__wmemcpy_chk (wchar_t *__restrict __s1, const wchar_t
                               *__restrict __s2, size_t __n, size_t __ns1)
__attribute__ ((__nothrow__ ));

extern wchar_t *__wmemcpy_alias (wchar_t *__restrict __s1,
                                 const wchar_t *__restrict __s2, size_t __n)
__asm__("wmemcpy") __attribute__ ((__nothrow__ ));

extern __inline __attribute__ ((__always_inline__))
__attribute__ ((__gnu_inline__))
__attribute__ ((__nothrow__ ))
wchar_t *
wmemcpy (wchar_t *__restrict __s1, const wchar_t *__restrict __s2, size_t __n)

{
  if (__builtin_object_size (__s1, 0) != (size_t) -1
      && (!__builtin_constant_p (__n)
          || __n > __builtin_object_size (__s1, 0) / sizeof (wchar_t)))
    {
        return __wmemcpy_chk (__s1, __s2, __n,
                              (__builtin_object_size (__s1, 0)
                               / sizeof (wchar_t)));
    }
  return __wmemcpy_alias (__s1, __s2, __n);
}

wchar_t wbuf[10];

int
main (int argc, char **argv)
{
  wmemcpy (wbuf + 1, L"abcdefghij", 10);
  return 0;
}

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Dec 4 2020, 7:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 7:18 AM

Update documentation and tests

serge-sans-paille retitled this revision from [WIP] Fix interaction between clang and some inline builtins from glibc under _FORTIFY_SOURCE to Fix interaction between clang and some inline builtins from glibc under _FORTIFY_SOURCE.Dec 4 2020, 2:15 PM
serge-sans-paille edited the summary of this revision. (Show Details)

@nickdesaulniers I added you to the review because you bumped into the non-inling issue at the kernel level at some point.

nickdesaulniers added inline comments.Dec 4 2020, 3:06 PM
llvm/lib/Analysis/InlineCost.cpp
2638–2640

shouldn't we do something here with RecursiveIsViable?

2679

should the second parameter here be true? If so, implies missing test coverage.

2680

rather than splitting this up and adding parameters, I think we can just check for the alwaysinline fn attr in llvm::isInlineViable.

Improve trivially recursive function detection

No modification on LLVM side, only at clang level

serge-sans-paille marked 2 inline comments as done.Dec 7 2020, 9:17 AM
serge-sans-paille added inline comments.
llvm/lib/Analysis/InlineCost.cpp
2638–2640

Updated to match the change at clang level.

serge-sans-paille edited the summary of this revision. (Show Details)Dec 7 2020, 9:40 AM
rnk requested changes to this revision.Dec 7 2020, 11:51 AM
rnk added a subscriber: rjmccall.
rnk added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
38

I see a dep from clangCodeGen to clangAnalysis in the CMakeLists, but this is the first include of clang/Analysis/* from CodeGen. I think we should consider this new dependency more carefully.

@rjmccall, can you please advise about the new dependency?

2938–2940

These are both really expensive. Is this really necessary?

clang/test/CodeGen/memmove-always-inline-definition-used.c
2

I'd like to see the generated IR before LLVM optimization passes run. Please add a RUN line with -disable-llvm-passes and add checks that show what attributes get applied where. From the code, I think noinline goes on the call site, but I wanted to check my understanding in the test.

This revision now requires changes to proceed.Dec 7 2020, 11:51 AM

Update test case

serge-sans-paille marked an inline comment as done.Dec 7 2020, 12:27 PM
serge-sans-paille added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
2938–2940

I'd be very happy to remove them. I could remove these use by designing an ad-hoc analysis to detect basic always recursive functions patterns (basically some kind of CFG-like analysis that would build the CFG on the fly with early branch exit when it finds a call to self)

serge-sans-paille edited the summary of this revision. (Show Details)

Updated approach, much less costly: match the pattern of functions forwarding to self, as detecting if they're recursive doesn't match the reality of inline builtins.
The rational could be that if it's doing anything other than forwarding to self, then it's there on purpose and we shouldn't skip it.

@rnk the new approach doesn't have the compilation time impact the previous had, still passes validation and a new test case has been added.

rnk added a comment.Dec 9 2020, 5:59 PM

This code was originally added to solve https://llvm.org/pr9614, which has a fair amount of context for how we got here today.

clang/test/CodeGen/memmove-always-inline-definition-used.c
18–20

Even if this happens to work in practice, memmove is still mutually recursive, and some other IPO transform could break this. The code pattern seems fragile.

If we are serious about implementing glibc fortify, which, to be clear, is a new direction for us, we should think about what IR pattern we really want to see out of clang.

I think this IR better represents the intention of the original program, and is better for optimizations and other IR analysis consumers:

define void @usercode() {
...
  call void @__fortify_memmove(...)
...
}
define i8* @__fortify_memmove(...) {
...
  call i8* @llvm.memcpy(...)
...
  call i8* @memmove(...)
...
}
declare i8* @memmove(...)

If we're going to have to live with this glibc fortify code pattern, maybe clang should try to spot it and rewrite it into more helpful and analyzable LLVM IR.


If people agree that this is a reasonable direction, let's think about how to implement it.

One way to get there would be to implement asm labels by waiting until after IRGen to do some value replacement. So, clang would emit code for the inline definition of @memmove, and then later, it would rename @memmove_alias to @memmove. At that point, it would invent a new name for the original @memmove. The least effort thing to do would be to rename it to @memmove.1 using the normal value renaming. We would never see this symbol in the ELF symbol table, because these are usually marked always inline.

As suggested by @rnk, spot the pattern more accurately at clang level, and use a combination of nobuiltin / builtin attributes to flag it at LLVM IR level.
What I like with that approach is that we're not preventing the emission a function Body, but just handling it in a decent way.

That's still only a possible approach, I'm of course open to other approaches!

serge-sans-paille edited the summary of this revision. (Show Details)Dec 14 2020, 5:54 AM

@rnk what do you find of this approach?

Rebased on main.

@rnk : are you happy with how the patch looks now?

@rnk I don't want that patch to bitrot , as it addresses a defect in _FORTIFY_SOURCE, i.e. it fills an (arguably small) security defect. Can you please have a look?

I apologize for taking an unreasonably long time to respond, this feature doesn't fill me with a sense of joy and fulfillment, but I do want to help get things unblocked.

Let me try getting more input and visibility from some folks: @bkramer @efriedma @mcgrathr

My basic input is that the frontend should try to untangle the complexity of the glibc fortify implementation so that the middle-end can remain blissfully unaware of it.

clang/test/CodeGen/memmove-always-inline-definition-used.c
18–20

I think my concern about having an infinitely recursive function in the IR is still here. I still think the right way to handle this is to rename the fortify wrappers to something else, memmove.1 or something. You'd also have to change the linkage to internal from available_externally.

I think the main case where that doesn't work is when you take the address of a fortify wrapper. In this case, you will end up with the address of the wrapper, and the compiler is forced to emit a standalone copy of the implementation.

llvm/lib/Transforms/IPO/Inliner.cpp
624

Changing the inliner doesn't seem like the right way to address this.

I'm not really happy with this approach. I'm concerned at the IR level about functions that appear to be recursive, but aren't really supposed to recurse.

I'd recommend teaching clang to rename the inline function. So in the LLVM IR, you have the definition of the inline (with internal linkage), and the declaration of the external function. Then the users refer to the actual function they want (either the inline, or the original), depending on the context. If it's a recursive call, or not a call, refer to the external declaration. If it's a call, it refers to the inline function. That makes the semantics of the IR obvious, and preserves the intended functionality.

(gcc can make the choice to inline after certain optimizations run, so it can optimize certain edge cases involving function pointers to library functions. But that's unlikely to matter for normal usage.)