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

crypto/internal/nistec: p256_asm_s390x.s causes SIGFPE: floating-point exception on older s390 #58465

Closed
JanZerebecki opened this issue Feb 10, 2023 · 9 comments
Closed

crypto/internal/nistec: p256_asm_s390x.s causes SIGFPE: floating-point exception on older s390 #58465

JanZerebecki opened this issue Feb 10, 2023 · 9 comments
Assignees
@Vishwanatha-HD
Labels
arch-s390x Issues solely affecting the s390x architecture. NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.21

Comments

@JanZerebecki
Copy link

What version of Go are you using (go version)?

$ go version
go1.19.5

Bug was introduced by: d63865b

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

Older s390 before x13 did not have support for vx. Instructions like VL, etc. where added in x13.

I'm not sure if that is intentional and this requirement is just not documented. Otoh maybe it is ok to switch to the generic version when pu.S390X.HasVX is false.

Can be emulated with: qemu-s390x-static -cpu qemu,vx=off

What did you do?

Try a https request that uses this.
Local small reproducer with instructions on how to emulate: https://github.com/djoreilly/reprod-s390-go-bug

What did you expect to see?

No crash.

What did you see instead?

# /usr/bin/qemu-s390x-static -cpu qemu,vx=off /loadcerts 
qemu-s390x-static: warning: 'vxeh' requires 'vx'.
qemu-s390x-static: warning: 'vxeh' requires 'vx'.
qemu-s390x-static: warning: 'vxeh' requires 'vx'.
qemu-s390x-static: warning: 'vxeh' requires 'vx'.
qemu-s390x-static: warning: 'vxeh' requires 'vx'.
qemu-s390x-static: warning: 'vxeh' requires 'vx'.
go version: go1.19.5
cpu.S390X.HasVX: false
SIGFPE: floating-point exception
PC=0xf0d92 m=0 sigcode=0
instruction bytes: 0xe7 0x20 0x20 0x10 0x0 0x6 0xe7 0x22 0x20 0x0 0x40 0x84 0xe7 0x33 0x30 0x0

goroutine 1 [running]:
crypto/internal/nistec.p256BigToLittle(0xc000061560, 0xc0001ca0df)
	/usr/lib64/go/1.19/src/crypto/internal/nistec/p256_asm_s390x.s:74 +0x12 fp=0xc0000613d8 sp=0xc0000613d8 pc=0xf0d92
crypto/internal/nistec.(*P256Point).SetBytes(0xc00010e480, {0xc0001ca0de, 0x41, 0xe4})
	/usr/lib64/go/1.19/src/crypto/internal/nistec/p256_asm.go:95 +0x11c fp=0xc000061620 sp=0xc0000613d8 pc=0xeda7c
crypto/elliptic.(*nistCurve[...]).Unmarshal(0x225df0, {0xc0001ca0de, 0x41, 0xe4})
	/usr/lib64/go/1.19/src/crypto/elliptic/nistec.go:257 +0xa0 fp=0xc000061690 sp=0xc000061620 pc=0xf2db0
crypto/elliptic.(*nistCurve[...]).Unmarshal({0xc0001ca0de, 0x41, 0xe4})
	<autogenerated>:1 +0x7c fp=0xc0000616d0 sp=0xc000061690 pc=0xf34bc
crypto/elliptic.Unmarshal({0x18c118, 0x225df0}, {0xc0001ca0de, 0x41, 0xe4})
	/usr/lib64/go/1.19/src/crypto/elliptic/elliptic.go:117 +0x3e8 fp=0xc000061748 sp=0xc0000616d0 pc=0xf14e8
crypto/x509.parsePublicKey(0x3, 0xc000061c98)
	/usr/lib64/go/1.19/src/crypto/x509/parser.go:258 +0xa2a fp=0xc000061838 sp=0xc000061748 pc=0x10e88a
crypto/x509.parseCertificate({0xc0001ca000, 0x1ba, 0x1c2})
	/usr/lib64/go/1.19/src/crypto/x509/parser.go:911 +0x826 fp=0xc000061d30 sp=0xc000061838 pc=0x113886
crypto/x509.ParseCertificate({0xc0001ca000, 0x1ba, 0x1c2})
	/usr/lib64/go/1.19/src/crypto/x509/parser.go:972 +0x3c fp=0xc000061d68 sp=0xc000061d30 pc=0x1144bc
crypto/x509.(*CertPool).AppendCertsFromPEM(0xc000061f50, {0xc000180000, 0x35839, 0x3583a})
	/usr/lib64/go/1.19/src/crypto/x509/cert_pool.go:219 +0x1aa fp=0xc000061e88 sp=0xc000061d68 pc=0x10c33a
main.main()
	/root/loadcerts/loadcerts.go:22 +0x216 fp=0xc000061f80 sp=0xc000061e88 pc=0x117686
runtime.main()
	/usr/lib64/go/1.19/src/runtime/proc.go:250 +0x26e fp=0xc000061fd8 sp=0xc000061f80 pc=0x4f2de
runtime.goexit()
	/usr/lib64/go/1.19/src/runtime/asm_s390x.s:749 +0x2 fp=0xc000061fd8 sp=0xc000061fd8 pc=0x800e2

goroutine 2 [force gc (idle)]:
runtime.gopark(0x168070, 0x22ea70, 0x11, 0x14, 0x1)
	/usr/lib64/go/1.19/src/runtime/proc.go:363 +0x128 fp=0xc000034fb0 sp=0xc000034f98 pc=0x4f788
runtime.goparkunlock(...)
	/usr/lib64/go/1.19/src/runtime/proc.go:369
runtime.forcegchelper()
	/usr/lib64/go/1.19/src/runtime/proc.go:302 +0xc2 fp=0xc000034fd8 sp=0xc000034fb0 pc=0x4f5d2
runtime.goexit()
	/usr/lib64/go/1.19/src/runtime/asm_s390x.s:749 +0x2 fp=0xc000034fd8 sp=0xc000034fd8 pc=0x800e2
created by runtime.init.5
	/usr/lib64/go/1.19/src/runtime/proc.go:290 +0x30

goroutine 3 [GC sweep wait]:
runtime.gopark(0x168070, 0x22ec80, 0xc, 0x14, 0x1)
	/usr/lib64/go/1.19/src/runtime/proc.go:363 +0x128 fp=0xc0000357a0 sp=0xc000035788 pc=0x4f788
runtime.goparkunlock(...)
	/usr/lib64/go/1.19/src/runtime/proc.go:369
runtime.bgsweep(0xc000052000)
	/usr/lib64/go/1.19/src/runtime/mgcsweep.go:278 +0xa0 fp=0xc0000357c8 sp=0xc0000357a0 pc=0x388a0
runtime.gcenable.func1()
	/usr/lib64/go/1.19/src/runtime/mgc.go:178 +0x5e fp=0xc0000357d8 sp=0xc0000357c8 pc=0x2b35e
runtime.goexit()
	/usr/lib64/go/1.19/src/runtime/asm_s390x.s:749 +0x2 fp=0xc0000357d8 sp=0xc0000357d8 pc=0x800e2
created by runtime.gcenable
	/usr/lib64/go/1.19/src/runtime/mgc.go:178 +0xa8

goroutine 4 [GC scavenge wait]:
runtime.gopark(0x168070, 0x22ed80, 0xd, 0x14, 0x2)
	/usr/lib64/go/1.19/src/runtime/proc.go:363 +0x128 fp=0xc000035f80 sp=0xc000035f68 pc=0x4f788
runtime.goparkunlock(...)
	/usr/lib64/go/1.19/src/runtime/proc.go:369
runtime.(*scavengerState).park(0x22ed80)
	/usr/lib64/go/1.19/src/runtime/mgcscavenge.go:389 +0x72 fp=0xc000035fa8 sp=0xc000035f80 pc=0x36332
runtime.bgscavenge(0xc000052000)
	/usr/lib64/go/1.19/src/runtime/mgcscavenge.go:617 +0x5a fp=0xc000035fc8 sp=0xc000035fa8 pc=0x369ea
runtime.gcenable.func2()
	/usr/lib64/go/1.19/src/runtime/mgc.go:179 +0x5e fp=0xc000035fd8 sp=0xc000035fc8 pc=0x2b2de
runtime.goexit()
	/usr/lib64/go/1.19/src/runtime/asm_s390x.s:749 +0x2 fp=0xc000035fd8 sp=0xc000035fd8 pc=0x800e2
created by runtime.gcenable
	/usr/lib64/go/1.19/src/runtime/mgc.go:179 +0x10e

goroutine 17 [finalizer wait]:
runtime.gopark(0x168070, 0x26c0e0, 0x10, 0x14, 0x1)
	/usr/lib64/go/1.19/src/runtime/proc.go:363 +0x128 fp=0xc000034718 sp=0xc000034700 pc=0x4f788
runtime.goparkunlock(...)
	/usr/lib64/go/1.19/src/runtime/proc.go:369
runtime.runfinq()
	/usr/lib64/go/1.19/src/runtime/mfinal.go:180 +0x132 fp=0xc0000347d8 sp=0xc000034718 pc=0x2a0a2
runtime.goexit()
	/usr/lib64/go/1.19/src/runtime/asm_s390x.s:749 +0x2 fp=0xc0000347d8 sp=0xc0000347d8 pc=0x800e2
created by runtime.createfing
	/usr/lib64/go/1.19/src/runtime/mfinal.go:157 +0x64

r0   0x41	r1   0xc000061560
r2   0xc0001ca0df	r3   0xc000061560
r4   0xc0001ca0de	r5   0x4
r6   0xc00010e4e0	r7   0x26c340
r8   0x1	r9   0x4027c95218
r10  0x26c604	r11  0x0
r12  0x18bd50	r13  0xc0000021a0
r14  0xeda7c	r15  0xc0000613d8
pc   0xf0d92	link 0xeda7c
@JanZerebecki
Copy link
Author

@laboger Do you know if dropping of support for pre z13 was intentional?

@randall77
Copy link
Contributor

Our current support matrix says z196+. How is that related to x13?

@laboger
Copy link
Contributor

laboger commented Feb 12, 2023

@billotosyr or @jonathan-albrecht-ibm can you answer this question?

@JanZerebecki
Copy link
Author

Ah, thx, was trying to find that support matrix (probably failed due to #58464). According to https://en.wikipedia.org/wiki/IBM_z196 it is the 2 generations before https://en.wikipedia.org/wiki/IBM_z13_(microprocessor) thus it is a bug on a supported architecture.

@billotosyr
Copy link

billotosyr commented Feb 13, 2023 •
edited

You are correct. Starting with Go1.19, z13 is the minimum machine level for running Go on LoZ. The proximal cause of this change was the refactoring of elliptic curve to internal/nistec. The new code structures made it difficult to dynamically switch implementations at runtime, so it became necessary (in order to continue to use the accelerated implementation) to require z13 as the minimum. For many years (five years perhaps?) the minimum had been z12. The support matrix should be updated.

@randall77
Copy link
Contributor

Ok, then can we:

  • Update the minimum requirements wiki
  • Have Go programs, when run on unsupported hardware, crash on startup with a nice message instead of dying in crypto code (see runtime/asm_amd64.s:rt0_go for an example)

@billotosyr
Copy link

I have updated the wiki (sorry for that oversight). I'll look into your excellent suggestion regarding startup.

@bcmills bcmills added the arch-s390x Issues solely affecting the s390x architecture. label Feb 13, 2023
@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 14, 2023
@prattmic prattmic added this to the Go1.21 milestone Feb 14, 2023
@Vishwanatha-HD Vishwanatha-HD self-assigned this Apr 25, 2023
@gopherbot
Copy link

Change https://go.dev/cl/499495 mentions this issue: crypto: Handle SIGPE exception for p256 crypto/internal on older s390x

@Vishwanatha-HD Vishwanatha-HD added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 8, 2023
@Vishwanatha-HD
Copy link
Contributor

Working on this issue. I have made the code changes to take care of this issue on s390x arch.
Link for CL is: https://go-review.googlesource.com/c/go/+/499495

CL is still under review and once the review comments are fine, this CL will be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Vishwanatha-HD Vishwanatha-HD

Labels
arch-s390x Issues solely affecting the s390x architecture. NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
8 participants
@prattmic @bcmills @randall77 @JanZerebecki @gopherbot @laboger @billotosyr @Vishwanatha-HD