Add a comment about ecp_nistz256_point_add_affine's limitations.
ecp_nistz256_point_add_affine does not support the doubling case and,
unlike ecp_nistz256_point_add which does a tail call, computes the wrong
answer. Note TestPointAdd in the unit tests skips this case.
This works fine because we only use ecp_nistz256_point_add_affine for
the g_scalar term, which is fully computed before the p_scalar term.
(Additionally it requires that the windowing pattern never hit the
doubling case for single multiplication.)
But this is not obvious from reading the multiplication functions, so
leave a comment at the call site to point this out.
Change-Id: I08882466d98030cdc882a5be9e702ee404e80cce
Reviewed-on: https://boringssl-review.googlesource.com/c/33945
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
diff --git a/crypto/fipsmodule/ec/p256-x86_64.c b/crypto/fipsmodule/ec/p256-x86_64.c
index ef1ccef..013afd3 100644
--- a/crypto/fipsmodule/ec/p256-x86_64.c
+++ b/crypto/fipsmodule/ec/p256-x86_64.c
@@ -378,6 +378,9 @@
ecp_nistz256_neg(t.p.Z, t.a.Y);
copy_conditional(t.a.Y, t.p.Z, wvalue & 1);
+ // Note |ecp_nistz256_point_add_affine| does not work if |p.p| and |t.a|
+ // are the same non-infinity point, so it is important that we compute the
+ // |g_scalar| term before the |p_scalar| term.
ecp_nistz256_point_add_affine(&p.p, &p.p, &t.a);
}
}
@@ -432,6 +435,9 @@
ecp_nistz256_neg(t.a.Y, t.a.Y);
}
+ // Note |ecp_nistz256_point_add_affine| does not work if |p.p| and |t.a|
+ // are the same non-infinity point, so it is important that we compute the
+ // |g_scalar| term before the |p_scalar| term.
ecp_nistz256_point_add_affine(&p.p, &p.p, &t.a);
}
