crypto/aes (and others): data race not detected due to assembly

[egonelbre]

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

$ go version
go version go1.21.3 windows/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

Race detector doesn't instrument assembly functions, hence some of the data races go undetected.

For example, here's an obviously racy crypto/aes example, but running it with -race does not detect it.

https://go.dev/play/p/dK-fJz-p5ey

This is definitely not isolated to crypto/aes, but any standard (or external) library package that uses assembly.

Going over the standard libary, assembly was used in:

$ go list -deps -f "{{if .SFiles}}{{.ImportPath}} {{.SFiles}}{{end}}" std
internal/abi [abi_test.s stub.s]
internal/cpu [cpu.s cpu_x86.s]
internal/bytealg [compare_amd64.s count_amd64.s equal_amd64.s index_amd64.s indexbyte_amd64.s]
runtime/internal/atomic [atomic_amd64.s]
runtime [asm.s asm_amd64.s duff_amd64.s memclr_amd64.s memmove_amd64.s preempt_amd64.s rt0_windows_amd64.s sys_windows_amd64.s test_amd64.s time_windows_amd64.s zcallback_windows.s]
internal/reflectlite [asm.s]
sync/atomic [asm.s]
math [dim_amd64.s exp_amd64.s floor_amd64.s hypot_amd64.s log_amd64.s]
reflect [asm_amd64.s]
hash/crc32 [crc32_amd64.s]
crypto/subtle [xor_amd64.s]
crypto/internal/boring/sig [sig_amd64.s]
crypto/aes [asm_amd64.s gcm_amd64.s]
math/big [arith_amd64.s]
crypto/internal/edwards25519/field [fe_amd64.s]
crypto/internal/nistec [p256_asm_amd64.s]
crypto/internal/bigmod [nat_amd64.s]
crypto/sha512 [sha512block_amd64.s]
crypto/internal/boring/bcache [stub.s]
crypto/md5 [md5block_amd64.s]
crypto/sha1 [sha1block_amd64.s]
crypto/sha256 [sha256block_amd64.s]
vendor/golang.org/x/crypto/internal/poly1305 [sum_amd64.s]
vendor/golang.org/x/sys/cpu [cpu_x86.s]
vendor/golang.org/x/crypto/chacha20poly1305 [chacha20poly1305_amd64.s]
runtime/debug [debug.s]
maps [maps.s]
os/signal [sig.s]
runtime/cgo [asm_amd64.s gcc_amd64.S]
runtime/coverage [dummy.s]
runtime/internal/startlinetest [func_amd64.s]

Of course, not all of them are problematic in terms of data races.

Similar search for x/crypto packages:

golang.org/x/sys/cpu [cpu_x86.s]
golang.org/x/crypto/blake2b [blake2bAVX2_amd64.s blake2b_amd64.s]
golang.org/x/crypto/argon2 [blamka_amd64.s]
golang.org/x/crypto/blake2s [blake2s_amd64.s]
golang.org/x/crypto/internal/poly1305 [sum_amd64.s]
golang.org/x/crypto/chacha20poly1305 [chacha20poly1305_amd64.s]
golang.org/x/crypto/curve25519/internal/field [fe_amd64.s]
golang.org/x/crypto/salsa20/salsa [salsa20_amd64.s]
golang.org/x/crypto/sha3 [keccakf_amd64.s]

What did you expect to see?

A data race being detected in packages that use assembly.

What did you see instead?

No data race in an obviously racy code.

---

So, there seem to be two main issues:

• packages using assembly should make their behavior visible to race detector
• the API that the packages use to make their behavior visible

I guess there are few ways to make the behavior visible to race detector:

• don't use assembly when -race is used, and use Go code instead
• call race detector functions directly
• annotate assembly to make the race detector aware

Not using assembly can be a significant performance hit in some scenarios, so switching off assembly doesn't seem ideal.

The other approach is to call the race detector functions directly, e.g. runtime.RaceReadRange, runtime.RaceWriteRange. One of the problems is that those functions are only exposed when race tag is present, making it more annoying to instrument third-party packages. Standard library does have access to internal/race, which is conditionally compiled with -race. It would be nice to use a similar package (maybe expose some of it in runtime/race) from third-party party libraries. Of course, it shouldn't expose things like Disable, Enable or Enabled.

Third approach is to add some magic incantation to assembly code to allow the compiler to add appropriate race detection, e.g. rough draft (ignore whether I got the content correct):

// func encryptBlockAsm(nr int, xk *uint32, dst, src *byte)
// race: readrange src, nr*4
// race: readrange dst, nr*4
// race: readrange xk, nr
TEXT ·encryptBlockAsm(SB),NOSPLIT,$0

Of course, writing those annotations in comments is significantly more annoying than using some nicer Go API.

There's a related issue #61204, about internal/bytes package which has some additional constraints the non-internal packages don't have.

[egonelbre]

PS: I can add the appropriate annotations to std library packages using internal/race as the first step; which of course can later be replaced by a better API at a later date.

[egonelbre]

@mauri870 I'm not quite sure that this is a security issue per se. Although, an undected race in crypto code could definitely lead to a security issue. I just happened to demonstrate the issue with crypto code.

[cherrymui]

@egonelbre as you already found #61204, which is similar and related, and this issue is not specific to crypto/aes, maybe we can combine this into #61204 (change "bytes:" to "all:")? Thanks.

[egonelbre]

@cherrymui yeah, that sounds good as well. Should I copy paste all the content from this to the other issue as well?

[cherrymui]

I updated the title there. Feel free to post any information there you think helpful. Thanks.

Closing this as a dup.