[libcxx-commits] [PATCH] D155873: [libc++][hardening] Categorize more assertions.

Mon Jul 24 14:55:43 PDT 2023

var-const added inline comments.

================
Comment at: libcxx/include/__hash_table:1112
     {
-        _LIBCPP_ASSERT_UNCATEGORIZED(__n < bucket_count(),
+        _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__n < bucket_count(),
             "unordered container::begin(n) called with n >= bucket_count()");
----------------
Mordante wrote:
> ldionne wrote:
> > It would be nice to make sure that all the non-internal assertions we add are tested. In some cases this will mean removing `XFAIL` on an existing test, in other cases we'll need to add some tests but it should be easy.
> +1
As discussed offline -- I will do this in a follow-up, simply due to the release timing. I did a quick survey and counted 83 untested assertions (among the ones currently categorized), so unfortunately it will take a while.

================
Comment at: libcxx/include/__tree:171
 {
-    _LIBCPP_ASSERT_UNCATEGORIZED(__x != nullptr, "Root node shouldn't be null");
+    _LIBCPP_ASSERT_INTERNAL(__x != nullptr, "Root node shouldn't be null");
     while (__x->__left_ != nullptr)
----------------
Mordante wrote:
> var-const wrote:
> > Mordante wrote:
> > > As mentioned in D155906 I really would like to have these kind of cheap internal sanity checks enabled in `_LIBCPP_ENABLE_HARDENED_MODE`. They are not user errors, but they still cause potential issues for users, including users in safety-critial places. When there's not enough time to get that done before LLVM 17, I'm happy to defer that to LLVM 18.
> > Thanks for starting this discussion!
> > 
> > My current thinking has been to keep the hardened mode as lightweight as possible. I have a big concern about performance penalty blocking adoption; also, while I'm not 100% sure this can happen, my ideal would be if we could make the hardened mode the default at some point (of course, users would still be able to opt out and use the unchecked mode). Because of that, so far I have been generally erring on the side of performance and using the following criteria for enabling an assertion:
> > 1. Is the failure directly triggered by user input?
> > 2. Is the failure //directly// exploitable?
> > 3. Is the failure condition relatively cheap to check?
> > 
> > Personally, I feel #2 is the most important distinction. Almost any error, including logic errors that don't trigger UB, can lead to very incorrect results and also contribute to a security vulnerability. I envision the hardened mode as only preventing errors that create a security vulnerability //directly// (e.g. an out-of-bounds read) -- the kind of errors where we can confidently say "if your program has one of those, it is vulnerable, regardless of any other context" (and thus we can justify the extra penalty for checking those).
> > 
> > From that perspective, it follows that checks for null pointers should not be a part of the hardened mode (FWIW, it was somewhat surprising to me, or at least I found it counterintuitive). From what I know (and I'd be happy to be corrected on this if I'm missing something), dereferencing a null pointer on any relatively modern OS leads to a crash and is not directly exploitable (it might contribute to a more complicated exploit arising from context, but so can many other types of failures).
> > 
> > The upside I'm hoping we get from limiting the number of checks we perform is wider adoption. The intention behind hardened mode is not that it provides complete safety or entirely eliminates UB -- rather, it makes your program measurably //safer// at a "negligible" cost; it relatively cheaply protects against the most catastrophic stuff. I'm hoping something like the 80/20 principle applies here (meaning that just a fraction of the cost that would be necessary to protect against all UB is enough to prevent 80% of exploitable vulnerabilities). The "perfect" hardened mode (aspirational, not realistic) is the mode where if you were to enable it, you would be guaranteed that an attacker could not use your program to achieve arbitrary code execution, //and// it's cheap enough to use in production.
> > 
> > We currently have the hardened mode and the debug mode. While I'd prefer to keep this as simple as possible and stick with the 2 modes, I'd be open to exploring adding a 3rd mode in-between those two that would contain more checks that the hardened mode while still aiming at some reasonable level of performance (which for the debug mode is almost a non-goal).
> > 
> > Tagging @ldionne as well in case he has a different perspective.
> I'm not sure whether deferring a null pointer will always leads to a crash on a modern OS. I can imagine some cases where that might be an issue. Embedded systems may use less well know OSes. Are kernel drivers allowed to modify address 0 or with low values? (This assumes a nullptr is mapped to address 0, which is not required by the Standard.)
> 
> Maybe we should ask this to security experts. They probably can tell us how often deferring a nullptr may lead to real-world issues.
> 
> Even when it's not a security-issue I think it would be good to test a nullptr when not allowed. Deferring nullptr is known to be quite a large issue in both C and C++. I'm also open to a third mode. Maybe we should measure what the overhead for adding null checks is. When we annotate it with likelihood attribute telling the compiler to expect a valid pointer the branch predictor will always guess correctly. In that case the overhead should be low. The nullptr case will be more expense, but I in that case the performance is no longer relevant.
> 
> I think we can move forward this way. When we put everything in the right category we can measure the overhead of every category. That way we can have number of the performance overhead and can make an informed decision in this regard.
> 
> I think it would be good to discuss this on Discord/Discourse to get input of more people in the LLVM communicate. Again I'm fine to do that for LLVM 18 and keep the current way in LLVM 17.
> 
> 
> Maybe we should ask this to security experts. They probably can tell us how often deferring a nullptr may lead to real-world issues.

Absolutely. I think these are all great questions to discuss.

> I'm not sure whether deferring a null pointer will always leads to a crash on a modern OS.

That's my understanding but I would absolutely love a domain expert to confirm.

> Embedded systems may use less well know OSes. Are kernel drivers allowed to modify address 0 or with low values? (This assumes a nullptr is mapped to address 0, which is not required by the Standard.)

IMO, the hardened mode should only contain checks that apply to almost every user; it shouldn't make users of widely-used systems to pay a performance penalty for the sake of more niche OSs. I'd much rather think of a way to let vendors of these kind of systems to enable additional null pointer checks on top of the hardened mode, whether via a third mode or some finer-grained approach. My overarching concern is users avoiding the hardened mode due to performance concerns, whether real or perceived; I'm afraid that enabling too many checks might backfire and result in less security due to a lack of use.

> Maybe we should measure what the overhead for adding null checks is.

+1.

> When we annotate it with likelihood attribute telling the compiler to expect a valid pointer the branch predictor will always guess correctly. In that case the overhead should be low.

There was recently a [[ https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609/18 | Discourse thread ]] about how the assume attribute can prevent certain optimizations, so I'm not sure we can always rely on that.

> I think we can move forward this way. When we put everything in the right category we can measure the overhead of every category. That way we can have number of the performance overhead and can make an informed decision in this regard.
> 
> I think it would be good to discuss this on Discord/Discourse to get input of more people in the LLVM communicate. Again I'm fine to do that for LLVM 18 and keep the current way in LLVM 17.

Agreed -- once the release is done, I'd like to get some performance data and start a wider discussion.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155873/new/

https://reviews.llvm.org/D155873