Opened 4 years ago

Closed 4 years ago

#16666 closed defect (fixed)

ed25519-donna doesn't build on arm without warnings with the stack protector

Reported by: yawning Owned by: yawning
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.2-alpha
Severity: Keywords: tor-core, tor-crypto
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

From Jenkins (https://jenkins.torproject.org/job/tor-ci-linux-master-extra-arm/147/ARCHITECTURE=armhf,SUITE=jessie/consoleFull):

13:54:27 ../tor/src/ext/ed25519/donna/ed25519_tor.c: In function 'curved25519_scalarmult_basepoint_donna':
13:54:27 ../tor/src/ext/ed25519/donna/ed25519_tor.c:107:12: error: stack protector not protecting local variables: variable length buffer [-Werror=stack-protector]
13:54:27  ED25519_FN(curved25519_scalarmult_basepoint) (curved25519_key pk, const curved25519_key e) {
13:54:27             ^
13:54:27 ../tor/src/ext/ed25519/donna/ed25519_tor.c:33:32: note: in definition of macro 'ED25519_FN3'
13:54:27  #define ED25519_FN3(fn,suffix) fn##suffix

Commit d8352646900c6316af8e2967742ce6a438b04221 should have fixed this, but apparently it's not enough. I'm just going to change the donna code to not align to 16 byte boundaries on ARM since that's only there for the SSE2 code in the first place.

On the off chance that the code does gain NEON support the penalty for unaligned access isn't that severe (there are load/store forms that require alignment, in which case the code will crash), but when that happens we can revisit this issue.

Child Tickets

Change History (3)

comment:1 Changed 4 years ago by yawning

Status: newneeds_review

https://github.com/Yawning/tor/compare/bug16666

Yes, this should be fine even if GCC chooses to generate NEON code (and if it's not fine, that would be a GCC bug). Yes, this will need changes if someone adds NEON code that requires alignment (but the changes required are trivial, and the performance penalty for non-aligned access isn't massive).

Marking as needs-review, though it's laughably trivial. If this is merged and fixes the issue, then d8352646900c6316af8e2967742ce6a438b04221 probably can be reverted to keep our ed25519-donna code closer to upstream as well.

comment:2 Changed 4 years ago by yawning

Ugh. I don't know wtf I was thinking when I wrote that first branch. This does the right thing, sorry. I went and changed it to only align to 16 byte boundaries for SSE2 builds since it beats special casing SSP builds on every single non-x86 target. Tested on x86_64, but we never build with SSE2 due to performance penalty so the code does work.

https://github.com/Yawning/tor/compare/bug16666_take2

comment:3 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks!

Note: See TracTickets for help on using tickets.