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.

Sign up for GitHub

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

Jump to bottom

runtime: fix over-eager SPWRITE check #44269

Closed
rsc opened this issue Feb 15, 2021 · 18 comments
Closed

runtime: fix over-eager SPWRITE check #44269

rsc opened this issue Feb 15, 2021 · 18 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.17

Comments

@rsc
Copy link
Contributor

rsc commented Feb 15, 2021 •
edited

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 The path to resolution is known, but the work has not been done. label Feb 15, 2021
@rsc rsc added this to the Unreleased milestone Feb 15, 2021
@rsc
Copy link
Contributor Author

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 commented Feb 15, 2021

Also blake2b. Also salsa20.

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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 commented Feb 15, 2021

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

@rsc
Copy link
Contributor Author

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
@rsc
blake2s: fix amd64 assembly not to smash SP
d8b00f7 
For golang/go#44269.

Change-Id: I877a8056dbd8ab1dedadb562aa1b3d9e1e0d55da
zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Feb 15, 2021
@rsc
salsa20/salsa: fix amd64 assembly not to smash SP
451f944 
For golang/go#44269.

Change-Id: Ica352261d696317addbdd422d4cde5bf07fef839
zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Feb 15, 2021
@rsc
blake2b: fix amd64 assembly not to smash SP
b3900e3 
For golang/go#44269.

Change-Id: I7e405afd0b55c96ce0a4c6058ba01e8be1173a8c
zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Feb 15, 2021
@rsc
blake2s: fix 386 assembly not to smash SP
d274665 
For golang/go#44269.

Change-Id: I92e168674612af390bcb80a0579df5c777c26970
@FiloSottile
Copy link
Contributor

"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 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 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

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

@zx2c4
Copy link
Contributor

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

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

@zx2c4
Copy link
Contributor

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

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
@rsc
blake2s: fix amd64 assembly not to smash SP
86c0c3e 
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
@rsc
salsa20/salsa: fix amd64 assembly not to smash SP
042588c 
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
@rsc
blake2b: fix amd64 assembly not to smash SP
1f2b32a 
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
@rsc
blake2s: fix 386 assembly not to smash SP
b8e89b7 
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
@AZ-X
salsa20/salsa: fix amd64 assembly not to smash SP
9e9751d 
**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.
@tmthrgd tmthrgd mentioned this issue Mar 4, 2021
Merged
mikroskeem pushed a commit to mikroskeem/golang-blake2s that referenced this issue Apr 4, 2021
@rsc
blake2s: fix amd64 assembly not to smash SP
f5d19e4 
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
@rsc
blake2s: fix 386 assembly not to smash SP
88c0e5a 
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>
@gopherbot
Copy link

Change https://golang.org/cl/317669 mentions this issue: runtime: fix handling of SPWRITE functions in traceback

@rsc
Copy link
Contributor Author

rsc commented May 6, 2021

On further analysis it appears that the old x/crypto code was dodgy but technically okay, in the sense that the runtime can work around it reasonably well. I have relaxed the check in the runtime in CL 317669, so now there is no need for a vet check nor to go looking for other dodgy-but-technically-okay assembly code in the wild.

CL 317669 will close this issue when submitted.

@rsc rsc changed the title x/crypto: unsafe x86 assembly writing SP runtime: fix over-eager SPWRITE check May 6, 2021
@rsc rsc modified the milestones: Unreleased, Go1.17 May 6, 2021
@bcmills bcmills mentioned this issue Oct 7, 2021
Closed
@bcmills bcmills mentioned this issue Nov 9, 2021
Closed
@golang golang locked and limited conversation to collaborators May 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
4 participants
@zx2c4 @rsc @FiloSottile @gopherbot