crypto/bn256/google: fix receiver aliasing in point doubling - #35558
Open
4waan wants to merge 3 commits into
Open
crypto/bn256/google: fix receiver aliasing in point doubling#355584waan wants to merge 3 commits into
4waan wants to merge 3 commits into
Conversation
curvePoint.Double and twistPoint.Double compute z3 = 2*y1*z1 after they have already written c.y. When the receiver aliases the operand that reads back y3 instead of y1, so z3 is wrong and the result is not on the curve. Add routes to Double whenever both operands are the same point, so g.Add(g, g) returned garbage in both G1 and G2. Doubling into a separate receiver was already correct, which is why the existing tests, the cross-implementation fuzzer and the ECADD precompile all missed it. Reorder to match the cloudflare backend.. the formulas are unchanged. twistPoint.Negative had the same bug, zeroing c.y before reading a.y. Its only caller passes distinct arguments, so nothing depended on it. The BUG(agl) notes on G1.Add and G2.Add describe an a==b limitation that never applied here: Add has carried the doubling branch since this code was vendored, and cloudflare has that branch and no such note. No consensus impact: ECADD uses fresh points for both operands and the result, and crypto/bn256 resolves to gnark on amd64/arm64.
Author
|
since theres not much active work going on in this part @gballet could you just do a quick review it'd be really helpful for me to continue a few other work around this! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
curvePoint.DoubleandtwistPoint.Doublein thegooglebn256 backend break when the receiver aliases the operand.g.Add(g, g)comes back off the curve, in both G1 and G2.The bug
Both functions end like this:
z3 = 2*y1*z1is the last use ofa.y, and it runs afterc.yis overwritten. Whencandaare the same point,a.yholdsy3by then, so you getz3 = 2*y3*z1.Addhands both-operands-equal straight toDouble(if xEqual && yEqual { c.Double(a, pool); return }), sog.Add(g, g)walks right into it. On master:IsOnCurverejects the last one in both groups. The fix computesc.zbefore writingc.y, which is whatcloudflarealready does. Statement move, formulas untouched,gnarkunaffected.Scope
Every
curvePoint/twistPointoperation, swept for receiver aliasing on master:curvePoint.Double(c, c)twistPoint.Double(c, c)Add(c, c, c)(G1 and G2)DoubletwistPoint.Negative(c, c)SetZerobefore readinga.y)Add(c, b)/Add(a, c), distinct pointsAdd(a, a)into a distinct receivercurvePoint.Negativebig.Int.Negis alias-safe)Mulwith an aliased receivertwistPoint.Negativeis the same defect and a one-line change, so it is fixed here too. Its only caller passes distinct arguments, so it was latent.Additself is fine as written:c.xandc.yare written after the last reads ofa.x/a.y, andc.zcomes last froma.z/b.z, which the earlier writes do not touch.The BUG(agl) notes
G1.AddandG2.Addcarry// BUG(agl): this function is not complete: a==b fails.Inherited from upstream, and never true here: the doubling branch has been inAddsince the code was vendored in 10a57fc, soa == bworked whenever the receiver was distinct. What failed was aliasing, which the note does not mention.cloudflarehas the same branch and no such note. Replaced with the aliasing guarantee, happy to drop that hunk.No consensus impact
all of the 49 bn256 vectors in
core/vm/testdata/precompiles/hit their expected output through this backend.. the SHA-256 over every output byte does not moveNot included
Add's doubling branch returns early without putting back its pooled temporaries: 10 leakedbig.Ints incurvePoint.Add, 20 intwistPoint.Add, per call. Plain free list, so it costs allocations and nothing else, and it is unrelated to this fix. Here or in a follow-up, your call.