Received-SPF: softfail (google.com: domain of transitioning linux-crypto-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <a22345e8-92a5-687e-4940-ac7b9dcc7c2f@linux.ibm.com>
Date:   Tue, 26 Apr 2022 13:53:16 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] ecdsa: Fix incorrect usage of vli_cmp
Content-Language: en-US
To:     herbert@gondor.apana.org.au, davem@davemloft.net,
        linux-crypto@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, stable@vger.kernel.org
References: <20220421185740.799271-1-stefanb@linux.ibm.com>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20220421185740.799271-1-stefanb@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 4/21/22 14:57, Stefan Berger wrote:
> Fix incorrect usage of vli_cmp when calculating the value of res.x. For
> signature verification to succeed, res.x must be the same as the r
> component of the signature which is in the range of [1..n-1] with 'n'
> being the order of the curve. Therefore, when res.x equals n calculate
> res.x = res.x - n as well. Signature verification could have previously
> unnecessarily failed in extremely rare cases.

Actually, I am withdrawing this patch. Before this patch res.x could 
equal 'n' and then wouldn't match r due to the range of r being 
[1..n-1]. Now if res.x equals 'n' then res.x - n will be 0 and again 
will not match 'r' due to the range of r being [1..n-1]. So it makes no 
difference whether vli_cmp() == 1 or vli_cmp() >= 1 and the concern 
above about rare cases not verifying the signature is wrong.

    Stefan

> 
> Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   crypto/ecdsa.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
> index b3a8a6b572ba..674ab9275366 100644
> --- a/crypto/ecdsa.c
> +++ b/crypto/ecdsa.c
> @@ -120,8 +120,8 @@ static int _ecdsa_verify(struct ecc_ctx *ctx, const u64 *hash, const u64 *r, con
>   	/* res = u1*G + u2 * pub_key */
>   	ecc_point_mult_shamir(&res, u1, &curve->g, u2, &ctx->pub_key, curve);
>   
> -	/* res.x = res.x mod n (if res.x > order) */
> -	if (unlikely(vli_cmp(res.x, curve->n, ndigits) == 1))
> +	/* res.x = res.x mod n (if res.x >= order) */
> +	if (unlikely(vli_cmp(res.x, curve->n, ndigits) >= 0))
>   		/* faster alternative for NIST p384, p256 & p192 */
>   		vli_sub(res.x, res.x, curve->n, ndigits);
>