This is an archive of the discontinued LLVM Phabricator instance.

Clear the stack protector after checking it
Needs ReviewPublic

Authored by Flakebi on Mar 4 2018, 7:11 AM.

Details

Reviewers
lattner
eugenis
Summary

SSPs cannot be leaked from the stack through uninitialized memory anymore, because they are removed after they are used.
This makes it in some cases harder for attackers to circumvent ssps and it has no (measurable) performance costs.

I developed this patch as part of my bachelor thesis. Therefore I measured the performance impact in a microbenchmark, which called a function with a ssp an a loop
(the benchmark executable did nothing else, the function wrote a single value into a stack local array so the ssp was generated).
In this benchmark, no change in performance was visible, it took as long as before.

Diff Detail

Event Timeline

Flakebi created this revision.Mar 4 2018, 7:11 AM
probinson added subscribers: eugenis, probinson.

+ @eugenis as this seems like his area.

eugenis added subscribers: pcc, kcc.Mar 16 2018, 4:58 PM

@pcc, @kcc I'm not sure about this. It adds one extra instruction per function, which is not exactly free. Do you have any estimates on how much additional protection does it provide? My gut feeling is that it would be only a minor speed bump in most cases.

It took a while, now the code, figures and a more detailed explanation are online: https://flakebi.de/uni-items/ba/#clear-ssp
Even with this microbenchmark, I was not able to measure a difference in performance in comparison to llvm without this patch.

This looks fine to me in general. @timshen @etienneb @rnk

Please do the same thing in SDAG stack protector, otherwise you are missing some random configurations like glibc on aarch64, or arm32 on android.
Please add tests.

lib/CodeGen/SafeStack.cpp
464 ↗(On Diff #136937)

Add a comment /*isVolatile*/ here and below.

Flakebi updated this revision to Diff 200146.May 18 2019, 5:33 AM

The ssp is now set to zero in the SDAG variant and when a stack guard check function is used. I am not familiar with the selection DAG so please correct me if it can be improved.
I also added a test as requested.

Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2019, 5:33 AM

Hi,

is there anything missing for this pull request?

Sorry, but I'm not convinced that the overhead of this change is justified by the security benefit it provides.
I've measured code size overhead (using Chromium on Android as a benchmark) at 0.4%, which is not huge, but still significant.
On the other hand, I'm not at all sure that this would be anything but an inconvenience for an attacker. There are multiple copies of the cookie on the stack anyway (one per every live frame!). Also, taking advantage of the cookies left below SP will become even harder with the new -ftrivial-auto-var-init feature.