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

Jump to bottom

x/tools/go/analysis: framepointer doesn't report BP clobbering on Windows #47027

Open
egonelbre opened this issue Jul 2, 2021 · 5 comments
Open

x/tools/go/analysis: framepointer doesn't report BP clobbering on Windows #47027

egonelbre opened this issue Jul 2, 2021 · 5 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@egonelbre
Copy link
Contributor

egonelbre commented Jul 2, 2021 •
edited

https://go-review.googlesource.com/c/tools/+/248686 introduced frame pointer check for clobbering BP register, however the check is only enabled for linux and darwin.

// func mixBlocksSSE2(out, a, b, c *block)
TEXT ·mixBlocksSSE2(SB), 4, $0-32
	MOVQ out+0(FP), DX
	MOVQ a+8(FP), AX
	MOVQ b+16(FP), BX
	MOVQ c+24(FP), CX
	MOVQ $128, BP
	RET

As an example doesn't report the clobbering on windows. https://github.com/golang/crypto/blob/5ff15b29337e062d850872081bcd9f4d784f4c25/argon2/blamka_amd64.s#L204

It's not clear why other OS-s were excluded from this analysis.

I would expect the clobbering to be reported on files that are compiled for linux/darwin on windows as well.
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Jul 2, 2021
@dmitshur dmitshur added this to the Unreleased milestone Jul 2, 2021
@dmitshur dmitshur changed the title tools/go/analysis: framepointer doesn't report BP clobbering on Windows x/tools/go/analysis: framepointer doesn't report BP clobbering on Windows Jul 2, 2021
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 2, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Jul 2, 2021

CC @randall77, @timothy-king.

@randall77
Copy link
Contributor

I don't remember why I excluded other OSes. It's worth trying to enable them and see what happens.
Does Windows ever try to traceback user code? Does it use BP to do so? If the answer is no to either, then it may just not matter on Windows.

@egonelbre
Copy link
Contributor Author

If the answer is no to either, then it may just not matter on Windows.

I'll have to check the BP usage on Windows, I suspect no. But, even if BP usage is valid on Windows it might not be on Linux/Darwin -- so, I would still expect the warning for assembly that is for all OS-s when running go vet on Windows. If the assembly file is tagged only for Windows, then not reporting BP clobbering is understandable.

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
@pilinux
Copy link

pilinux commented Aug 18, 2023

Even the latest crypto package v0.12.0 does not provide option to use secret for argon2. Hence yesterday I started developing my own argon2 package on top of the latest crypto package from Go. Since I need to access the deriveKey function to achieve the gaol easily, I decided to clone argon2 from official Go repo and everything went fine. On mac (arm), I faced no issue. But then I ran static analysis on other machines and encountered these problems.

On Ubuntu 22.04 (amd64):

/blamka_amd64.s:203:1: [amd64] mixBlocksSSE2: invalid offset a+24(FP); expected a+8(FP)
./blamka_amd64.s:226:1: [amd64] xorBlocksSSE2: invalid offset a+24(FP); expected a+8(FP)
./blamka_amd64.s:204:1: frame pointer is clobbered before saving
./blamka_amd64.s:227:1: frame pointer is clobbered before saving

On Windows 11 (intel/amd64):

/blamka_amd64.s:203:1: [amd64] mixBlocksSSE2: invalid offset a+24(FP); expected a+8(FP)
./blamka_amd64.s:226:1: [amd64] xorBlocksSSE2: invalid offset a+24(FP); expected a+8(FP)

Then I tried to understand the assembly codes for Go assembler and I got confused here:

https://cs.opensource.google/go/x/crypto/+/refs/tags/v0.12.0:argon2/blamka_amd64.s

  • line 201: MOVQ a+8(FP), AX
  • line 203: MOVQ a+24(FP), CX
  • line 224: MOVQ a+8(FP), AX
  • line 226: MOVQ a+24(FP), CX

In my understanding, line 203 should be MOVQ c+24(FP), CX and line 226 should be MOVQ c+24(FP), CX.

Also, the original BP was not restored properly, hence the error on Ubuntu. Later I did modify the code this way:
pilinux/argon2@e374b3d

And all those errors were solved. I did static analysis for arm64 and amd64 and for win, mac and linux using Github actions (you can check the results in the repo). Today I used this package in an app and can confirm that everything worked as they should.

I wanted to create an issue but then found this one, hence I am writing here.

Perhaps someone from the official Go dev team will look into this.

Thank you.

@gopherbot
Copy link

Change https://go.dev/cl/332289 mentions this issue: argon2: avoid clobbering BP

gopherbot pushed a commit to golang/crypto that referenced this issue Nov 27, 2023
@egonelbre @gopherbot
argon2: avoid clobbering BP
bda2f3f 
go vet was reporting

  blamka_amd64.s:203:1: [amd64] mixBlocksSSE2: invalid offset a+24(FP); expected a+8(FP)
  blamka_amd64.s:226:1: [amd64] xorBlocksSSE2: invalid offset a+24(FP); expected a+8(FP)
  blamka_amd64.s:204:1: frame pointer is clobbered before saving
  blamka_amd64.s:227:1: frame pointer is clobbered before saving

Also fix a similar naming issue in sha3:

  sha3\keccakf_amd64.s:325:1: [amd64] keccakF1600: unknown variable state; offset 0 is a+0(FP)

Updates golang/go#47027

Change-Id: Ia74852cdb0721ae0216787054197b0cac9e1c0f8
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/332289
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@egonelbre @dmitshur @adonovan @randall77 @gopherbot @pilinux