Support for stack protectors. #60933

eeckstein · 2022-09-02T18:03:00Z

The main part of this PR is a new StackProtection optimization and changes in the stdlib

The optimization decides which functions need stack protection. It sets the needStackProtection flags on all function which contain stack-allocated values for which an buffer overflow could occur.
In the stdlib there are new underscored _withUnprotected..Pointer functions. These functions work the same way as their "unprotected" counterparts, except that they don't trigger stack protection for the pointers. They can be used to opt out of stack protection.
Also, in the stdlib, opt out of stack protection in places where there can't be buffer overflows.
We trust the internal implementation of the stdlib to not cause any unintentional buffer overflows. In such cases we can use the "unprotected" address-to-pointer conversions. This avoids inserting stack protections where it's not needed.

For details see the commit messages.

Note: this PR does not enable stack protection by default, yet. It still needs to be enabled with the -Xfrontend -enable-stack-protector option.

rdar://93677524

eeckstein · 2022-09-02T18:07:45Z

@swift-ci test

eeckstein · 2022-09-02T18:07:57Z

@swift-ci benchmark

aschwaighofer · 2022-09-02T18:15:50Z

Now that Swift decides whether or not we need stack protection we should force LLVM to emit stack protection when we request it:

--- a/lib/IRGen/IRGenModule.cpp
+++ b/lib/IRGen/IRGenModule.cpp
@@ -1265,7 +1265,7 @@ void IRGenModule::constructInitialFnAttributes(
     Attrs.removeAttribute(llvm::Attribute::OptimizeForSize);
   }
   if (stackProtector == StackProtectorMode::StackProtector) {
-    Attrs.addAttribute(llvm::Attribute::StackProtectStrong);
+    Attrs.addAttribute(llvm::Attribute::StackProtectReq);
     Attrs.addAttribute("stack-protector-buffer-size", llvm::utostr(8));
   }
 }

lorentey

The stdlib parts look good! 👍

(Will stack protection also trigger on functions that use withUnsafeTemporaryAllocation?)

eeckstein · 2022-09-02T18:55:06Z

Will stack protection also trigger on functions that use withUnsafeTemporaryAllocation?

Good point! It should, but I need to add handling of the "stackAlloc" builtin to make it work.

eeckstein · 2022-09-07T14:30:45Z

@lorentey I added support for withUnsafeTemporaryAllocation.
Also, I added #if $BuiltinUnprotectedAddressOf around uses of the new builtins in inlinable functions.

eeckstein · 2022-09-07T14:30:55Z

@swift-ci test

karwa · 2022-09-07T15:14:20Z

stdlib/public/core/KeyPath.swift

@@ -1951,7 +1951,8 @@ func _modifyAtWritableKeyPath_impl<Root, Value>(
      keyPath: _unsafeUncheckedDowncast(keyPath,
        to: ReferenceWritableKeyPath<Root, Value>.self))
  }
-  return keyPath._projectMutableAddress(from: &root)
+  let ptr = UnsafePointer<Root>(Builtin.unprotectedAddressOf(&root))


Can this use the scoped _withUnprotectedUnsafePointer function rather than using the Builtin to get an unscoped pointer?

Same comment for Random and StringSwitch. Is there a reason not to use the scoped versions?

yeah, that's a good idea

It doesn't make sense to let getAccessPathWithScope return an `EnclosingScope` as the second tuple element, because in case it's a `base`, it duplicates the `AccessBase` (which is returned in the first tuple element). Instead just return an optional `BeginAccessInst` which is not nil if such an "scope" is found.

Indicates that stack protectors are inserted into this function to detect stack related buffer overflows.

…ndex_addr` instructions. Also add new "unprotected" variants of the `addressof` builtins: * `Builtin.unprotectedAddressOf` * `Builtin.unprotectedAddressOfBorrow`

…LOptions. ... because we need it in the SIL pass pipeline, too. Also, add Swift bridging for that option.

Those functions work the same way as their "unprotected" counterparts, except that they don't trigger stack protection for the pointers. * `_withUnprotectedUnsafeMutablePointer` * `_withUnprotectedUnsafePointer` * `_withUnprotectedUnsafeBytes` (two times)

…ffer overflows. We trust the internal implementation of the stdlib to not cause any unintentional buffer overflows. In such cases we can use the "unprotected" address-to-pointer conversions. This avoids inserting stack protections where it's not needed.

And make all the case identifiers lowercase.

It decides which functions need stack protection. It sets the `needStackProtection` flags on all function which contain stack-allocated values for which an buffer overflow could occur. Within safe swift code there shouldn't be any buffer overflows. But if the address of a stack variable is converted to an unsafe pointer, it's not in the control of the compiler anymore. This means, if there is any `address_to_pointer` instruction for an `alloc_stack`, such a function is marked for stack protection. Another case is `index_addr` for non-tail allocated memory. This pattern appears if pointer arithmetic is done with unsafe pointers in swift code. If the origin of an unsafe pointer can only be tracked to a function argument, the pass tries to find the root stack allocation for such an argument by doing an inter-procedural analysis. If this is not possible, the fallback is to move the argument into a temporary `alloc_stack` and do the unsafe pointer operations on the temporary. rdar://93677524

…tack protection. Now that Swift decides whether or not we need stack protection we should force LLVM to emit stack protection when we request it.

…tectedAddressOf`

eeckstein · 2022-09-08T06:43:28Z

@swift-ci test

eeckstein · 2022-09-08T06:44:05Z

@swift-ci benchmark

The pass to decide which functions should get stack protection was added in swiftlang#60933, but was disabled by default. This PR enables stack protection by default, but not the possibility to move arguments into temporaries - to keep the risk low. Moving to temporaries can be enabled with the new frontend option `-enable-move-inout-stack-protector`. rdar://93677524

eeckstein requested review from aschwaighofer, lorentey, atrick and rjmccall September 2, 2022 18:04

lorentey approved these changes Sep 2, 2022

View reviewed changes

eeckstein force-pushed the stack-protectors branch from eaf3dcd to 1fd6554 Compare September 7, 2022 14:28

karwa reviewed Sep 7, 2022

View reviewed changes

eeckstein added 11 commits September 8, 2022 08:37

SIL: add the SILFunction.needsStackProtection flag

fdca208

Indicates that stack protectors are inserted into this function to detect stack related buffer overflows.

SIL: add needsStackProtection flags for address_to_pointer and `i…

97b2354

…ndex_addr` instructions. Also add new "unprotected" variants of the `addressof` builtins: * `Builtin.unprotectedAddressOf` * `Builtin.unprotectedAddressOfBorrow`

AST: add a BuiltinUnprotectedAddressOf language feature

d8aa7a3

IRGen: move the EnableStackProtector option from IRGenOptions to SI…

3e1ff0a

…LOptions. ... because we need it in the SIL pass pipeline, too. Also, add Swift bridging for that option.

Swift SIL: add BuiltinInst.ID.stackAlloc

0eddce0

And make all the case identifiers lowercase.

IRGen: add sppreq instead of sspstrong for functions which need s…

1ae1c51

…tack protection. Now that Swift decides whether or not we need stack protection we should force LLVM to emit stack protection when we request it.

stdlib: use _withUnprotected... functions instead of `Builtin.unpro…

5eff906

…tectedAddressOf`

eeckstein force-pushed the stack-protectors branch from 1fd6554 to 5eff906 Compare September 8, 2022 06:43

eeckstein merged commit 1ba25d0 into swiftlang:main Sep 8, 2022

eeckstein deleted the stack-protectors branch September 8, 2022 10:43

fibrechannelscsi mentioned this pull request Sep 29, 2022

KeyPath performance improvement: Omit projection across trivially-typed memory. #60758

Merged

eeckstein mentioned this pull request Nov 11, 2022

SILOptimizer: enable stack protection by default #62057

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support for stack protectors. #60933

Support for stack protectors. #60933

Uh oh!

eeckstein commented Sep 2, 2022 •

edited

Loading

Uh oh!

eeckstein commented Sep 2, 2022

Uh oh!

eeckstein commented Sep 2, 2022

Uh oh!

aschwaighofer commented Sep 2, 2022

Uh oh!

lorentey left a comment

Uh oh!

eeckstein commented Sep 2, 2022

Uh oh!

eeckstein commented Sep 7, 2022

Uh oh!

eeckstein commented Sep 7, 2022

Uh oh!

karwa Sep 7, 2022 •

edited

Loading

Uh oh!

eeckstein Sep 7, 2022

Uh oh!

eeckstein commented Sep 8, 2022

Uh oh!

eeckstein commented Sep 8, 2022

Uh oh!

Uh oh!

Support for stack protectors. #60933

Support for stack protectors. #60933

Uh oh!

Conversation

eeckstein commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eeckstein commented Sep 2, 2022

Uh oh!

eeckstein commented Sep 2, 2022

Uh oh!

aschwaighofer commented Sep 2, 2022

Uh oh!

lorentey left a comment

Choose a reason for hiding this comment

Uh oh!

eeckstein commented Sep 2, 2022

Uh oh!

eeckstein commented Sep 7, 2022

Uh oh!

eeckstein commented Sep 7, 2022

Uh oh!

karwa Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eeckstein Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

eeckstein commented Sep 8, 2022

Uh oh!

eeckstein commented Sep 8, 2022

Uh oh!

Uh oh!

eeckstein commented Sep 2, 2022 •

edited

Loading

karwa Sep 7, 2022 •

edited

Loading