Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto: unsafe x86 assembly writing SP #44269

Open
rsc opened this issue Feb 15, 2021 · 16 comments
Open

x/crypto: unsafe x86 assembly writing SP #44269

rsc opened this issue Feb 15, 2021 · 16 comments
Labels
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Feb 15, 2021

x/crypto/blake2s/blake2s_386.s masks and writes to SP during function execution.
This is not allowed by the Go ABI. All SP modifications on goroutine stacks must be constant additions or subtractions, or else the garbage collector cannot unwind the stack from that location.

Code used to get away with this due to cooperative scheduling, but now that we scan stacks preemptively, it is especially important that functions not do this.

New code in Go 1.17 will stop stack traces in such functions, which will make programs crash more loudly. But they are still broken in earlier versions and may just cause silent memory corruption instead of loud crashes.

hashBlocksSSE2 and hashBlocksSSSE3 should leave SP alone.

Edit: also blake2b, salsa20/salsa.

@rsc rsc added the NeedsFix label Feb 15, 2021
@rsc rsc added this to the Unreleased milestone Feb 15, 2021
@rsc
Copy link
Contributor Author

@rsc rsc commented Feb 15, 2021

This is true of blake2s_amd64.s as well - the SP write is hidden in a macro.

@rsc rsc changed the title x/crypto/blake2s: unsafe 386 assembly writing SP x/crypto/blake2s: unsafe x86 assembly writing SP Feb 15, 2021
@rsc
Copy link
Contributor Author

@rsc rsc commented Feb 15, 2021

Also blake2b. Also salsa20.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 15, 2021

Change https://golang.org/cl/292052 mentions this issue: blake2s: fix 386 assembly not to smash SP

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 15, 2021

Change https://golang.org/cl/292051 mentions this issue: blake2b: fix amd64 assembly not to smash SP

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 15, 2021

Change https://golang.org/cl/292050 mentions this issue: salsa20/salsa: fix amd64 assembly not to smash SP

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 15, 2021

Change https://golang.org/cl/292049 mentions this issue: blake2s: fix amd64 assembly not to smash SP

@rsc rsc changed the title x/crypto/blake2s: unsafe x86 assembly writing SP x/crypto: unsafe x86 assembly writing SP Feb 15, 2021
@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 15, 2021

This also applies to windows/svc/sys_*.s in x/sys.

@rsc
Copy link
Contributor Author

@rsc rsc commented Feb 15, 2021

@zx2c4 I don't believe that's true, since servicemain appears to run on a Windows stack called from Windows - it's not a Go function subject to the Go ABI rules.

zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Feb 15, 2021
For golang/go#44269.

Change-Id: I877a8056dbd8ab1dedadb562aa1b3d9e1e0d55da
zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Feb 15, 2021
For golang/go#44269.

Change-Id: Ica352261d696317addbdd422d4cde5bf07fef839
zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Feb 15, 2021
For golang/go#44269.

Change-Id: I7e405afd0b55c96ce0a4c6058ba01e8be1173a8c
zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Feb 15, 2021
For golang/go#44269.

Change-Id: I92e168674612af390bcb80a0579df5c777c26970
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 16, 2021

"go vet" has a checker that runs on the TryBots against clobbering the frame pointer. It's caught real examples for me before (FiloSottile/edwards25519#11). Could we add one for this, too, so we don't reintroduce the issue later?

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 16, 2021

go build -asmflags=all=-v will show a splat for it at build time. I agree go vet sounds like a nice place for this.

A few implementation strategies:

  1. Assemble with -v, and scrape the output. :(
  2. Parse the assembly file with regexes, like x/tools/go/analysis/passes/framepointer/framepointer.go does. :(
  3. Look at the SPWRITE bit in pclntbl after building.
  4. Somehow reuse the assembler code from the compiler in the analyzer to re-run the pass that the assembler is doing.

3 seems most appealing, but I don't know if vet can look at post-build artifacts or how that works. I've never looked at this code before.

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 16, 2021

https://go-review.googlesource.com/c/tools/+/292529 is strategy (2). Kind of meh, but perhaps this is just how analyzer works.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 16, 2021

Change https://golang.org/cl/292529 mentions this issue: go/analysis: add SPWRITE verifier

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 16, 2021

It apparently works too!

zx2c4@thinkpad ~/Projects/golang-dev/crypto $ GOARCH=386 ~/Projects/golang-dev/go/bin/go vet -stackpointer ./...
# golang.org/x/crypto/blake2s
blake2s/blake2s_386.s:304:1: stack pointer is manually modified
blake2s/blake2s_386.s:358:1: stack pointer is manually modified
blake2s/blake2s_386.s:373:1: stack pointer is manually modified
blake2s/blake2s_386.s:434:1: stack pointer is manually modified
zx2c4@thinkpad ~/Projects/golang-dev/crypto $ GOARCH=amd64 ~/Projects/golang-dev/go/bin/go vet -stackpointer ./...
# golang.org/x/crypto/blake2s
blake2s/blake2s_amd64.s:423:1: stack pointer is manually modified
# golang.org/x/crypto/blake2b
blake2b/blake2bAVX2_amd64.s:288:1: stack pointer is manually modified
blake2b/blake2bAVX2_amd64.s:369:1: stack pointer is manually modified
blake2b/blake2bAVX2_amd64.s:591:1: stack pointer is manually modified
blake2b/blake2bAVX2_amd64.s:749:1: stack pointer is manually modified
blake2b/blake2b_amd64.s:125:1: stack pointer is manually modified
blake2b/blake2b_amd64.s:280:1: stack pointer is manually modified
# golang.org/x/crypto/salsa20/salsa
salsa20/salsa/salsa20_amd64.s:23:1: stack pointer is manually modified
salsa20/salsa/salsa20_amd64.s:877:1: stack pointer is manually modified
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 16, 2021

Awesome, I'd support turning this on by default in "go vet" like #43014

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 16, 2021

I've got that code written, but it looks like it needs to be in x/tools first before I can vendor it and push a CL.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 16, 2021

Change https://golang.org/cl/292569 mentions this issue: cmd/vet: add stackpointer analysis run

gopherbot pushed a commit to golang/crypto that referenced this issue Feb 18, 2021
For golang/go#44269.

Change-Id: I877a8056dbd8ab1dedadb562aa1b3d9e1e0d55da
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/292049
Trust: Russ Cox <rsc@golang.org>
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
gopherbot pushed a commit to golang/crypto that referenced this issue Feb 18, 2021
For golang/go#44269.

Change-Id: Ica352261d696317addbdd422d4cde5bf07fef839
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/292050
Trust: Russ Cox <rsc@golang.org>
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
gopherbot pushed a commit to golang/crypto that referenced this issue Feb 18, 2021
For golang/go#44269.

Change-Id: I7e405afd0b55c96ce0a4c6058ba01e8be1173a8c
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/292051
Trust: Russ Cox <rsc@golang.org>
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
gopherbot pushed a commit to golang/crypto that referenced this issue Feb 18, 2021
For golang/go#44269.

Change-Id: I92e168674612af390bcb80a0579df5c777c26970
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/292052
Trust: Russ Cox <rsc@golang.org>
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
AZ-X added a commit to AZ-X/pique that referenced this issue Feb 20, 2021
**salsa20 is used by V1 protocol of DNSCrypt**
golang/go#44269
quote:
x/crypto/*.s masks and writes to SP during function execution.
This is not allowed by the Go ABI. All SP modifications on goroutine stacks must be constant additions or subtractions, or else the garbage collector cannot unwind the stack from that location.

Code used to get away with this due to cooperative scheduling, but now that we scan stacks preemptively, it is especially important that functions not do this.

New code in Go 1.17 will stop stack traces in such functions, which will make programs crash more loudly. But they are still broken in earlier versions and may just cause silent memory corruption instead of loud crashes.

hashBlocksSSE2 and hashBlocksSSSE3 should leave SP alone.
mikroskeem pushed a commit to mikroskeem/golang-blake2s that referenced this issue Apr 4, 2021
For golang/go#44269.

Change-Id: I877a8056dbd8ab1dedadb562aa1b3d9e1e0d55da
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/292049
Trust: Russ Cox <rsc@golang.org>
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
mikroskeem pushed a commit to mikroskeem/golang-blake2s that referenced this issue Apr 4, 2021
For golang/go#44269.

Change-Id: I92e168674612af390bcb80a0579df5c777c26970
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/292052
Trust: Russ Cox <rsc@golang.org>
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants