I'm not sure why make update wants to remove SM2 stuff, should this have been removed in a previous PR?

@richsalz am I doing something wrong here? Should I use something more specific to update the error codes, rather than a full make update to avoid removing this SM2 stuff?

I'm not sure what's happening. Maybe you're not on the current master? Curious.

I am on latest master, but apparently it's "dirty" and apparently make update does not remove superfluous unused error codes unless new error codes are detected:

~/repos/openssl >>> git log -1
commit 03a5e5ae6304c75f611d3613bedf2019524ff0d6 (HEAD -> master, upstream/master)
Author: Patrick Steuer <patrick.steuer@de.ibm.com>
Date:   Thu Jul 12 13:19:07 2018 -0400

    Fix undefined behavior in s390x aes-gcm/ccm

    Reviewed-by: Rich Salz <rsalz@openssl.org>
    Reviewed-by: Andy Polyakov <appro@openssl.org>
~/repos/openssl >>> ack EC_F_PKEY_SM2_CTRL
crypto/ec/ec_err.c
264:    {ERR_PACK(ERR_LIB_EC, EC_F_PKEY_SM2_CTRL, 0), "pkey_sm2_ctrl"},
265:    {ERR_PACK(ERR_LIB_EC, EC_F_PKEY_SM2_CTRL_STR, 0), "pkey_sm2_ctrl_str"},

include/openssl/ecerr.h
186:#  define EC_F_PKEY_SM2_CTRL                               284
187:#  define EC_F_PKEY_SM2_CTRL_STR                           285

EC_F_PKEY_SM2_CTRL is one of the error codes that gets removed on my make update step, and as you can see there is no function currently using it: the SM2 functions now are using their own "submodule", e.g., SM2_F_PKEY_SM2_CTRL.

Fixed following @dot-asm advice!

I'd like to suggest to add a space here, to align it with group, to denote the fact that they are in same expression.

Same as above.

Explicit new_ctx != NULL is preferred nowadays.

In such case you can remove new_ctx altogether. This comment applies even below.

EC_POINT_free checks for NULL, no need to do it here.

Formatting nit, two lines are one space too short.

thanks for catching this! I will fix both of them during the rebase!

Formatting nit. Two lines are one space too short.

Formatting nit, two lines are one space too long.

Fixed!

Even though it's kind of obvious from the context I'd like to see "LD" expanded. Even elsewhere.

I fixed this in the code, it might be useful to rewrite the message for commit 84e3bdd722cc0, e.g.:

EC2M Lopez-Dahab ladder: use it also for ECDSA verify

By default `ec_mul_consttime` (which uses the ladder implementation) is
used only for k*Generator or k*GenericPoint.
ECDSA verification uses a*Generator+b*GenericPoint: this commit
forces the use of `ec_mul_consttime` (and the Lopez-Dahab ladder) also
for the ECDSA verification path, while using the default wNAF
implementation for any other case.

With this commit ec_mul_consttime loses the static attribute, and is
added to ec_lcl.h so EC_METHODs can directly use it.

While working on a new custom EC_POINTs_mul implementation, I realized
that many checks (e.g. all the points being compatible with the given
group, creating ctx if NULL, scalar==NULL && num==0) were duplicated
again and again in every single implementation (and actually some
implementations lacked some tests). I thought that it makes way more
sense for those checks that are independent from the actual
implementation and should always be done, to be moved in the
EC_POINTs_mul wrapper: so this commit also includes these changes.

Sytle nit: spaces in front of and after "=" are required; i.e. int ret = 0;

Style nit: space needed after ","

There seems to be inconsistency in this function about whether we call ECerr or not.

The "fixup!" commits below fix the reported formatting nits in the corresponding commits.

I will have now a look at ec_mul_consttime to raise ECerr in the appropriate cases.

@mattcaswell can you check 6f9fd585222 ?

(I couldn't find more specific reasons than ERR_R_EC_LIB/ERR_R_BN_LIB). Suggestions?

Indentation seems strange here.

This is side note, which means that no action is actually expected, just think about it. While p != q works, I find it a bit less readable. Because you have to unravel it to scalar being NULL. At the same time replacing it with scalar == NULL doesn't make it perfect either. Maybe [again, this is just for consideration] one can arrange code so that condition would be q != NULL? So that pattern would be

if (q != NULL) EC_POINT_add(group, q, q, p, ctx);
else           q = p;

On additional thought, it's probably names being so generic that tips me off. For example if q was called say acc[umulator], it wouldn't be a problem. Point is that generic names make you wonder if there is any meaning behind letter. For example you would think of p as "product", and what would be q? And what is connection? With name like acc[umulator] you see directly that it's just a placeholder...

bbd0e2b is my attempt at maximizing readability: I clearly and explicitly separated the 3 cases: this comes at the "cost" of duplicating one line of code (ec_mul_consttime(group, r, scalars[0], points[0], ctx)) but I think it is a good tradeoff which does not uglify the code and solves way more problems in readability and upkeep than it creates.

Cool!

I for one sense inconsistency in this function naming. Now when it's shared, a better name might be more appropriate. Maybe ec_mul_by_scalar_consttime? Just me? Obviously side note...

What about ec_scalarmul_ladder?

I feel a bit of resistance in naming anything "consttime" given that we cannot promise anything about the underlying layer.

Once we agree on the renaming, given that it would require fixing themake update step and rewrite some commit messages, I would do a proper rebase, also squashing the stack of small fixup commits that piled up till now.

But I will have to wait until tomorrow to act upon this.

I totally favor a renaming, for the reason above, let's just agree on a proper name.

@bbbrumley /@mattcaswell would you care to chime in with your opinion on proper names?

What about ec_scalarmul_ladder?

Great! Except maybe ec_scalar_mul_ladder? Just pick either :-)

maybe ec_scalar_mul_ladder

+1

ec_scalar_mul_ladder works for me.

On side note one can find it confusing that ec_wNAF_mul calls ec_scalar_mul_ladder with inverse conditions. In other words except for scalar != NULL && num==1 case one can simply call ec_wNAF_mul. This makes you wonder if it would be appropriate to implement the additional scalar != NULL && num==1 in ec_wNAF_mul. In which case this method can be simply omitted, right?

If I understand correctly, you are suggesting to handle the (scalar != NULL && num==1) in ec_wNAF_mul() calling ec_scalar_mul_ladder: the problem with that is that for other implementations ec_scalar_mul_ladder can be slower than the regular "non-const-time" path.

Double multiplication is basically only used by ECDSA verification right now, where all the inputs are public, so a fast and insecure implementation could be used, rather than ec_scalar_mul_ladder.

What we really would need is refactor the EC_METHOD API to ditch/deprecate the unified EC_POINTs_mul (which makes a sense from a mathematical point of view, but is not really the way the 3 different scalar multiplication cases are used by cryptosystems) in favor of 3 different function pointers for fixed_point_mul, variable_point_mul, double_point_mul.
Right now we have wrappers that are forcing the 3 different operations into the unified EC_POINTs_mul path, and then in ec_scalar_mul_ladder we split them again.
In some cases, a specific double_point_mul could almost half the current execution cost of the verification step, as we could use formulas (e.g., derived from Shamir's trick) that avoid doing 2 distinct scalar multiplications and then 1 addition, and share as much computation as possible.
But this must wait for the next major release development cycle!

Does this answer your comment, or did I completely misunderstand what you meant?

Edit: grammar, formatting

the problem with that is that for other implementations ec_scalar_mul_ladder can be slower

Fair enough. Then how about

if (scalar == NULL || num != 1 || order == 0 || cofactor == 0)
    return ec_wNAF_mul()
/* scalar != NULL && num == 1 is faster with ec_scalar_mul_ladder here [and now] */
...

Just keep in mind that this is side note, so "how about" is really just a question, it's not a suggestion. Point is not larger than order != 0 && cofactor !=0 case in ec_wNAF_mul being partial duplicate of the code below this line. [Off topic, this kind of asks for question if ec_wNAF_mul is actually suitable name.]

I think this change would hinder readability and mask intent: what we want to convey is that binary non-degenerate curves (i.e., have a order and cardinality that makes sense, given that mathematically speaking 0 is not really an option for either value) use ec_scalar_mul_ladder for the 3 cases, any other crazyness that can be fed through the EC_POINTs_mul API is basically uncharted territory and not cryptographically relevant, so we will let the default wNAF implementation handle it.

It's true that, currently, we could let ec_wNAF_mul handle every case but the double scalar multiplication, and it would work, depending on the fact that that function will itself call the ec_scalar_mul_ladder for the fixed and variable point.
But I think that relying on the fact that currently something called ec_wNAF_mul is hijacked to perform a completely different code path in specific cases is even worse than duplicating the checks!

Re: renaming ec_wNAF_mul

I would argue that it would make sense to have in ec_mult.c something like ec_points_mul_default working as a wrapper distinguishing the two single point multiplication cases (for which ec_scalar_mul_ladder is then called) from the truly multi-point multiplication case (for which ec_wNAF_mul is called).
But I would leave this to a separate PR.

Even better, in a separate PR I would rather start a transition to 3 separate function pointers in EC_METHOD for the fixed/variable/double point multiplication, and let the EC_POINTs_mul wrapper do something like:

/* All kinds of checks on not NULL args, every point_is_compat, etc. */

if (group->meth->points_mul)
    /* you're on your own, probably will have to replicate most of the checks below */ 
    return group->meth->points_mul(...);

degenerate = BN_is_zero(group->order) || BN_is_zero(group->cofactor);

if (scalar != NULL && num == 0)
    if (group->meth->fixed_scalar_mul)
        /* you still have to handle degenerate cases */
        return group->meth->fixed_scalar_mul(...);
    else if (!degenerate)
        return ec_scalar_mul_ladder(...);

if (scalar == NULL && num == 1)
    if (group->meth->variable_scalar_mul)
        /* you still have to handle degenerate cases */
        return group->meth->variable_scalar_mul(...);
    else if (!degenerate)
        return ec_scalar_mul_ladder(...);

if (scalar != NULL && num == 1 && group->meth->double_scalar_mul)
    /* you still have to handle degenerate cases */
    return group->meth->double_scalar_mul(...);

/* HIC SUNT DRACONES */
return ec_wNAF_mul(...);

Until something similar exists, I don't think we can avoid specific EC_POINTs_mul implementations to repeat the checks for the 3 cases.
And the existence of the degenerate case will also probably mean that most non-default implementations of the 3 scalar_mul variants will need to repeat the check for degenerate cases.

Edit: Reword a bit and fixed a lot of typos....