[clang] [Sema] Mark alias/ifunc targets used and consider mangled names (PR #87130)

Fangrui Song via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 5 13:26:18 PDT 2024


MaskRay wrote:

> > I really appreciate the suggestions. `alias-unused.cpp` and `alias-unused-win.cpp` contain test improvement that should be pre-commited once they look good enough. Then this PR can be changed to show the difference.
> > On a separate note, I wanted to clarify that `-Wunused-function` false positives/negatives shouldn't be automatically considered security bugs. Categorizing all these warning improvements as "security bugs" would dilute the meaning of "security bugs" and make it harder to prioritize real vulnerabilities.
> > (
> > ```
> > What is a GCC security bug?
> > ===========================
> > 
> >     A security bug is one that threatens the security of a system or
> >     network, or might compromise the security of data stored on it.
> >     In the context of GCC, there are multiple ways in which this might
> >     happen and some common scenarios are detailed below.
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > )
> > (An attacker can easily bypass warnings: remove `-Werror` (uncommon in distros anyway), remove `-Wall` (which covers `-Wunused-function`, or use a pragma to disable `-Wunused-function` locally. )
> > The description contains an example about name mangling differences ([#87130 (files)](https://github.com/llvm/llvm-project/pull/87130/files#r1554029811)) and I mentioned that "This inconsistency makes alias/ifunc difficult to use in C++ with portability."
> > ```
> > extern "C" {
> > static void f0() {}
> > // GCC: void g0() __attribute__((alias("_ZL2f0v")));
> > // Clang: void g0() __attribute__((alias("f0")));
> > }
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > I added microsoftDemangle tests to show the current behavior. Since the feature that demangles to the function name without parameters (f3 instead of f3(int)) appears to be missing, I cannot address -Wunused-function false positives for microsoftDemangle with reasonable time complexity.
> 
> I don't believe we ARE trying to make that change to security definitions. However, we are all being particularly security conscious of this patch because the reporter is the person who JUST did the xz exploit over years. IMO, this bug report (and the original C only fix) were used in part to help cover his tracks, so being particularly careful here is paramount.

The description might derail from the primary purpose of the patch.

The -Wunused-function false positive was not used in part to cover the issue.
"Hans Jansen" claimed 5% improvement for his app that called the crc func and contributed that ifunc code.
Lasse Collin's commit (https://git.tukaani.org/?p=xz.git;a=commit;h=a37a2763383e6c204fe878e1416dd35e7711d3a9), confirmed with the author on IRC, aimed to fix a configure issue with Clang -Wall.
Lasse did not report the issue. "Jia Tan" did.

The attacker did not need to care about the -Wunused-function false positive.
They could remove `static` from the resolver function to avoid the -Wunused-function false positive.

Anyhow, the backdoor doesn't target Clang (`if test "x$CC" != 'xgcc' > /dev/null`), which might be related to other codegen discrepancy the attacker did not intend to control.


https://github.com/llvm/llvm-project/pull/87130


More information about the cfe-commits mailing list