This is an archive of the discontinued LLVM Phabricator instance.

[libc++][hardening] Add back the safe mode.
ClosedPublic

Authored by var-const on Aug 25 2023, 1:22 AM.

Details

Reviewers
jdoerfert
ldionne
Group Reviewers
Restricted Project
Commits
rGb85e1862c119: [libc++][hardening] Add back the safe mode.
Summary

The safe mode is in-between the hardened and the debug modes, extending
the checks contained in the hardened mode with certain checks that are
relatively cheap and prevent common sources of errors but aren't
security-critical. Thus, the safe mode trades off some performance for
a wider set of checks, but unlike the debug mode, it can still be used
in production.

Diff Detail

Event Timeline

var-const created this revision.Aug 25 2023, 1:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 1:22 AM
ldionne published this revision for review.Aug 25 2023, 12:56 PM
ldionne added subscribers: thakis, ldionne.
ldionne added inline comments.
libcxx/CMakeLists.txt
51

We need tests.

53

@thakis We brainstormed on some names here:

_LIBCPP_ENABLE_HARDENED_PLUS_MODE
_LIBCPP_ENABLE_EXTENDED_HARDENED_MODE
_LIBCPP_ENABLE_STRONG_HARDENED_MODE
_LIBCPP_ENABLE_STRICT_MODE
_LIBCPP_ENABLE_PARANOID_MODE
_LIBCPP_ENABLE_FORTIFIED_MODE
_LIBCPP_ENABLE_SAFE_MODE

Do you have any thoughts? Our thoughts so far:

HARDENED_PLUS, EXTENDED_HARDENED, STRONG_HARDENED // those are kind of heavyweight names
PARANOID // has negative connotation and doesn't make it clear whether it is stronger than DEBUG
STRICT // could be confused with the notion of not having non-standard extensions
FORTIFIED // not clear whether it is stronger than HARDENED or not
SAFE // our current preference

In fact, before LLVM 17 we had something called the SAFE mode, and I think what we discovered with Chromium's use case is that it still had its place. Hence, I think what we should do is call this the SAFE mode, backport this change, and rework the way we announced our 17 release notes not to say that we "replaced' the safe mode, but instead that we added new modes and that we changed how the safe mode is enabled. This is IMO a superior design and a superior way of rolling it out based on our experience so far.

libcxx/include/__algorithm/three_way_comp_ref_type.h
53 ↗(On Diff #553406)

I think this should be a separate (almost NFC) patch. There might be more of those, we should fix all.

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 25 2023, 12:56 PM
var-const updated this revision to Diff 553866.Aug 28 2023, 2:07 AM

Address feedback:

  • "hardened-plus" -> "safe";
  • add tests.
var-const retitled this revision from DRAFT [libc++][hardening] Add the hardened-plus mode. to [libc++][hardening] Add back the safe mode..Aug 28 2023, 2:08 AM
var-const edited the summary of this revision. (Show Details)
var-const marked 2 inline comments as done.Aug 28 2023, 2:15 AM
var-const added inline comments.
libcxx/include/__algorithm/three_way_comp_ref_type.h
53 ↗(On Diff #553406)
var-const updated this revision to Diff 553870.Aug 28 2023, 2:17 AM
var-const marked an inline comment as done.

Fix formatting and rebase.

var-const updated this revision to Diff 553874.Aug 28 2023, 2:33 AM

Add a note about the deprecated macro.

ldionne added inline comments.Aug 28 2023, 7:44 AM
libcxx/docs/Hardening.rst
58
libcxx/docs/ReleaseNotes/17.rst
104–105
104–105

We should replace all these references to libcxx/docs/Hardening.rst by an internal link to the rst page so that people can actually click on it.

114–115
libcxx/include/__config
281–286

No strong preference, but we could also do #if HARDENED_MODE + SAFE_MODE + DEBUG_MODE > 1.

libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp
11–13

I don't really understand this REQUIRES anymore. Can you explain?

Mordante added inline comments.
libcxx/cmake/caches/Generic-safe-mode.cmake
2

I really would like to enable modules in this build too. Maybe it would be better to do in a followup when we want to backport this.

libcxx/utils/ci/buildkite-pipeline.yml
508–509
libcxx/utils/libcxx/test/params.py
304–306

I start to feel slightly uncomfortable with these names. To me they are not very descriptive and they are now 4 options. I don't directly have better suggestions, but I think we should spend a bit of time on this.

For example, which is more expensive "safe" or "hardened"?

ldionne added inline comments.Aug 28 2023, 8:58 AM
libcxx/CMakeLists.txt
53

@Mordante Those are the other names we considered.

libcxx/utils/libcxx/test/params.py
304–306

I agree, this is definitely not perfect. This is user facing too, so we need to find something good.

We thought that safe was good given that it is the name we used for this mode historically (in LLVM 15 and LLVM 16). We also thought that it was "reasonably" clear that it was more expensive than hardened, but maybe it isn't. I'll CC you on the comment above that discusses naming.

Mordante added inline comments.Aug 28 2023, 9:38 AM
libcxx/CMakeLists.txt
53

Thanks. If this list SAFE sounds the best to me too.

libcxx/utils/libcxx/test/params.py
304–306

Thanks. As said I don't have a better suggestion either.

I believe this 3-level thing is in general a good idea, and in principle the words "hardened < safe < debug" fell ok-ish to me in terms of expectations how much gets checked.
However, in light of a 3-level mode I may want to rethink for example the asserts in mdspan, and tailor better what is enabled in hardened, vs safe mode. I think the current choices were good in a binary world, in this more nuanced approach I am not sure.

Mordante added inline comments.Aug 28 2023, 10:02 AM
libcxx/utils/libcxx/test/params.py
304–306

I agree, this is definitely not perfect. This is user facing too, so we need to find something good.

We thought that safe was good given that it is the name we used for this mode historically (in LLVM 15 and LLVM 16). We also thought that it was "reasonably" clear that it was more expensive than hardened, but maybe it isn't. I'll CC you on the comment above that discusses naming.

Based on this information and Discord I think we should go with safe. I still think it's not great, but it's consistent with what we did in the past.

var-const updated this revision to Diff 554126.Aug 28 2023, 5:51 PM
var-const marked 10 inline comments as done.

Address feedback

I believe this 3-level thing is in general a good idea, and in principle the words "hardened < safe < debug" fell ok-ish to me in terms of expectations how much gets checked.
However, in light of a 3-level mode I may want to rethink for example the asserts in mdspan, and tailor better what is enabled in hardened, vs safe mode. I think the current choices were good in a binary world, in this more nuanced approach I am not sure.

Thanks a lot for your comments! Re. mdspan -- IIRC, we classified most of the assertions in mdspan as valid-element-access, which would enable them in all modes. The only exceptions are:

  • layout_left/right::mapping checks -- IMO those should be enabled in the safe mode;
  • mdspan: size() is not representable as size_type -- I also think this should be on in the safe mode.

(FWIW, my current informal criteria for the safe mode is "any check as long as it's for UB triggered by user input and not too expensive". With that mindset, none of the checks I remember in mdspan strike me as debug-only -- i.e., all of them make sense in the safe mode as well)

libcxx/CMakeLists.txt
53

@Mordante Thank you for the feedback! We spent quite a bit of time on this but unfortunately I don't have a "slam dunk" suggestion. FWIW, I'll outline my reasoning:

  • The first presumption is that we'd rather keep the current naming of "hardened" and "debug" (debug in particular seems like a good descriptive name);
  • In that case, the new mode would be in-between "hardened" and "debug" modes, and two broad alternatives are to either call it the "hardened-something" mode (to stress that it's an extension of the hardened mode, e.g. "hardened extended mode") or by a single unrelated word. The former approach seemed to produce heavyweight, cumbersome names. The problem with the latter approach is that it's hard to find a single word that makes it obvious that this mode provides more guarantees than the hardened mode.
  • My preference for "safe" is partially because it focuses on the (desired) result, not how it is achieved (I think that "hardened" vs "safe" is a little like "optimized" vs "fast"), but in large part because we already have shipped the "safe" mode. I honestly doubt it's possible to find a single word that would unambiguously convey that this mode is stronger than the hardened mode, but "safe" has precedent in its favor, so to speak.
libcxx/docs/ReleaseNotes/17.rst
114–115

Applied (with a few tweaks, please let me know if they look reasonable to you).

libcxx/include/__config
281–286

Thanks! I think it's better (I don't really expect another mode at this point, but if that ever happens, the new form of this check is much easier to modify).

libcxx/utils/libcxx/test/params.py
304–306

(I'm marking these comments as "Done" just so that we can keep this conversation in one thread)

Address feedback and rebase.

var-const added inline comments.Aug 28 2023, 10:15 PM
libcxx/cmake/caches/Generic-safe-mode.cmake
2

Thanks! I'll do it in a follow-up like you suggest.

libcxx/include/__config
281–286

Update: unfortunately, with this approach the error is also triggered when setting one of the macros to a bad value (e.g. 2). Reverted to the previous state.

libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp
11–13

If another hardening mode is enabled (e.g. the safe mode), the assertions will still fire -- basically, we would end up in a situation where _LIBCPP_ENABLE_DEBUG_MODE is 0 but _LIBCPP_ENABLE_SAFE_MODE is 1.

(I changed the assertion type from "uncategorized" to "valid-element-access", btw -- thanks for calling out this test!)

var-const marked 3 inline comments as done.Aug 28 2023, 10:20 PM
var-const added inline comments.
libcxx/cmake/caches/Generic-safe-mode.cmake
2

@thakis It would be awesome if you could take a look at this patch and see if naming and the approach make sense for Chromium's use case. Thanks!

This looks perfect for us, thanks!

libcxx/CMakeLists.txt
53

As long as it's documented, any of those work for me. Naming tends to be a bikeshed. Here's some more ideas (for the cmake arg), but go with what you like best:

some, most, debug
safe, strict, debug
basic, thorough, debug
minimal, robust, debug
fast, conservative, debug

i.e. maybe renaming the "hardened" level could make the progression clearer. If you want to keep "hardened" as base level, maybe "thorough" or "conservative" are still contenders. But as I said, I don't mind "safe".

libcxx/include/__config
313

TODO when? After a security review of these asserts? Something else? Do you have a rough time frame when this might happen?

ldionne added inline comments.Aug 29 2023, 1:01 PM
libcxx/docs/ReleaseNotes/17.rst
89–90

I would prefer removing this release note entirely. It is misleading, I don't know why I added it in the first place. OOB checks in iterators for string_view require an ABI flag, which needs to be set by vendors. This is not interesting to point out in our release notes IMO.

libcxx/include/__config
225–226
313

IMO this TODO is not necessary. We know we want to remove _LIBCPP_ASSERT_UNCATEGORIZED anyway so it would become moot.

libcxx/test/libcxx/algorithms/alg.sorting/assert.min.max.pass.cpp
13
libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp
1

For another patch: we need to add tests that ensure that we can enable/disable the various hardening modes on a per-TU basis even if the default configured mode is another mode. For example, configure the library w/ the hardened mode by default, but enable the safe or debug mode inside a single TU via the user macros.

Or the other way around: enable safe mode by default via vendor but enable the hardened mode via users: the result should be that the safe mode set-by-default is disabled and the hardened mode selected by the user is enabled.

Potential way to achieve this?

#if _LIBCPP_ENABLE_HARDENED_MODE
#  define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_HARDENED
#elif _LIBCPP_ENABLE_SAFE_MODE
#  define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_SAFE
#elif _LIBCPP_ENABLE_DEBUG_MODE
#  define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_DEBUG
#endif

#define _LIBCPP_UNCHECKED 31
#define _LIBCPP_HARDENED 32
#define _LIBCPP_SAFE 33
#define _LIBCPP_DEBUG 34

#ifndef _LIBCPP_SELECT_HARDENING_MODE
#  define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_HARDENING_MODE_DEFAULT /* from __config_site */
#endif

_LIBCPP_SELECT_HARDENING_MODE would be internal-only, unless we want to migrate our users again in the next release :/

And if we want to make _LIBCPP_SELECT_HARDENING_MODE user-visible without migrating our users, we could probably do something like:

#ifndef _LIBCPP_SELECT_HARDENING_MODE
#  if _LIBCPP_ENABLE_HARDENED_MODE
#    define _LIBCPP_SELECT_HARDENING_MODE _LIBCPP_HARDENED
#  elif [...]
#  endif
#endif
9

This comment is misleading otherwise.

11–13

IMO this should be REQUIRES: libcpp-hardening-mode=debug since the test is basically meaningless in unchecked mode.

libcxx/test/libcxx/assertions/modes/debug_mode_enabled_in_tu.pass.cpp
23

UNCATEGORIZED?

var-const updated this revision to Diff 554540.Aug 29 2023, 6:07 PM
var-const marked 6 inline comments as done.

Address feedback

var-const marked 2 inline comments as done.Aug 29 2023, 6:08 PM
var-const added inline comments.
libcxx/include/__config
313

@thakis I certainly want to do this before the next release (i.e., LLVM 18), and ideally even earlier. And yes, that would include a security review.

libcxx/test/libcxx/algorithms/alg.sorting/assert.min.max.pass.cpp
13

Thanks!

var-const marked an inline comment as done.Aug 31 2023, 10:20 PM
var-const marked an inline comment as done.Sep 5 2023, 12:10 PM
var-const added inline comments.
libcxx/docs/ReleaseNotes/17.rst
89–90
var-const marked an inline comment as done.Sep 5 2023, 12:10 PM
ldionne accepted this revision.Sep 5 2023, 12:13 PM
This revision is now accepted and ready to land.Sep 5 2023, 12:13 PM

CI issues are flakes, this should be good to go.

ldionne added inline comments.Sep 5 2023, 12:17 PM
libcxx/docs/ReleaseNotes/17.rst
82

Note that this patch needs to be rebased on top of another patch that brings the release notes between LLVM 17 and main back into sync.

var-const marked an inline comment as done.Sep 8 2023, 11:47 AM
var-const added inline comments.
libcxx/docs/ReleaseNotes/17.rst
82
var-const marked an inline comment as done.Sep 8 2023, 11:48 AM
var-const updated this revision to Diff 556295.Sep 8 2023, 11:56 AM

Fix up the release notes

ldionne accepted this revision.Sep 8 2023, 12:08 PM

LGTM with green CI.

var-const updated this revision to Diff 556305.Sep 8 2023, 2:55 PM

Fix the CI.

This revision was automatically updated to reflect the committed changes.