linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] DH: SP800-56A rev 3 compliant shared secret
@ 2020-07-10 10:09 Stephan Müller
  2020-07-10 10:10 ` [PATCH 1/3] crypto: ECDH - check validity of Z before export Stephan Müller
                   ` (3 more replies)
  0 siblings, 4 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-10 10:09 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang

Hi,

The patch set adds the shared secret validation as defined by
SP800-56A rev 3. For ECDH this only implies that the validation
of the shared secret is moved before the shared secret is
returned to the caller.

For DH, the validation is required to be performed against the prime
of the domain parameter set.

This patch adds the MPI library file mpi_sub_ui that is required
to calculate P - 1 for the DH check. It would be possible, though
to simply set the LSB of the prime to 0 to obtain P - 1 (since
P is odd per definition) which implies that mpi_sub_ui would not
be needed. However, this would require a copy operation from
the existing prime MPI value into a temporary MPI where the
modification can be performed. Such copy operation is not available.
Therefore, the solution with the addition of mpi_sub_ui was chose.

NOTE: The function mpi_sub_ui is also added with the patch set
"[PATCH v5 2/8] lib/mpi: Extend the MPI library" currently sent
to the linux-crypto mailing list.

Marcelo Henrique Cerri (1):
  lib/mpi: Add mpi_sub_ui()

Stephan Mueller (2):
  crypto: ECDH - check validity of Z before export
  crypto: DH - check validity of Z before export

 crypto/dh.c          | 29 +++++++++++++++++++++
 crypto/ecc.c         | 11 +++++---
 include/linux/mpi.h  |  3 +++
 lib/mpi/Makefile     |  1 +
 lib/mpi/mpi-sub-ui.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 101 insertions(+), 3 deletions(-)
 create mode 100644 lib/mpi/mpi-sub-ui.c

-- 
2.26.2





^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH 1/3] crypto: ECDH - check validity of Z before export
  2020-07-10 10:09 [PATCH 0/3] DH: SP800-56A rev 3 compliant shared secret Stephan Müller
@ 2020-07-10 10:10 ` Stephan Müller
  2020-07-10 10:10 ` [PATCH 2/3] lib/mpi: Add mpi_sub_ui() Stephan Müller
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-10 10:10 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang

From 5385865b3f44d331f91c6786a2e7f4e2fb4d8cb2 Mon Sep 17 00:00:00 2001
From: Stephan Mueller <stephan.mueller@atsec.com>
Date: Thu, 11 Jun 2020 08:12:54 +0200
Subject: 

SP800-56A rev3 section 5.7.1.2 step 2 mandates that the validity of the
calculated shared secret is verified before the data is returned to the
caller. Thus, the export function and the validity check functions are
reversed. In addition, the sensitive variables of priv and rand_z are
zeroized.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/ecc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 02d35be7702b..52e2d49262f2 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1495,11 +1495,16 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 
 	ecc_point_mult(product, pk, priv, rand_z, curve, ndigits);
 
-	ecc_swap_digits(product->x, secret, ndigits);
-
-	if (ecc_point_is_zero(product))
+	if (ecc_point_is_zero(product)) {
 		ret = -EFAULT;
+		goto err_validity;
+	}
+
+	ecc_swap_digits(product->x, secret, ndigits);
 
+err_validity:
+	memzero_explicit(priv, sizeof(priv));
+	memzero_explicit(rand_z, sizeof(rand_z));
 	ecc_free_point(product);
 err_alloc_product:
 	ecc_free_point(pk);
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 2/3] lib/mpi: Add mpi_sub_ui()
  2020-07-10 10:09 [PATCH 0/3] DH: SP800-56A rev 3 compliant shared secret Stephan Müller
  2020-07-10 10:10 ` [PATCH 1/3] crypto: ECDH - check validity of Z before export Stephan Müller
@ 2020-07-10 10:10 ` Stephan Müller
  2020-07-10 14:42   ` Ard Biesheuvel
  2020-07-10 10:15 ` [PATCH 3/3] crypto: DH - check validity of Z before export Stephan Müller
  2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
  3 siblings, 1 reply; 42+ messages in thread
From: Stephan Müller @ 2020-07-10 10:10 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang

Add mpi_sub_ui() based on Gnu PG mpz_sub_ui() from mpz/aors_ui.h
adapting the code to the kernel's structures and coding style and also
removing the defines used to produce mpz_sub_ui() and mpz_add_ui()
from the same code.

Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 include/linux/mpi.h  |  3 +++
 lib/mpi/Makefile     |  1 +
 lib/mpi/mpi-sub-ui.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)
 create mode 100644 lib/mpi/mpi-sub-ui.c

diff --git a/include/linux/mpi.h b/include/linux/mpi.h
index 7bd6d8af0004..5d906dfbf3ed 100644
--- a/include/linux/mpi.h
+++ b/include/linux/mpi.h
@@ -63,6 +63,9 @@ int mpi_powm(MPI res, MPI base, MPI exp, MPI mod);
 int mpi_cmp_ui(MPI u, ulong v);
 int mpi_cmp(MPI u, MPI v);
 
+/*-- mpi-sub-ui.c --*/
+int mpi_sub_ui(MPI w, MPI u, unsigned long vval);
+
 /*-- mpi-bit.c --*/
 void mpi_normalize(MPI a);
 unsigned mpi_get_nbits(MPI a);
diff --git a/lib/mpi/Makefile b/lib/mpi/Makefile
index d5874a7f5ff9..43b8fce14079 100644
--- a/lib/mpi/Makefile
+++ b/lib/mpi/Makefile
@@ -16,6 +16,7 @@ mpi-y = \
 	mpicoder.o			\
 	mpi-bit.o			\
 	mpi-cmp.o			\
+	mpi-sub-ui.o			\
 	mpih-cmp.o			\
 	mpih-div.o			\
 	mpih-mul.o			\
diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
new file mode 100644
index 000000000000..fa6b085bac36
--- /dev/null
+++ b/lib/mpi/mpi-sub-ui.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* mpi-sub-ui.c  -  MPI functions
+ *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
+ *      Free Software Foundation, Inc.
+ *
+ * This file is part of GnuPG.
+ *
+ * Note: This code is heavily based on the GNU MP Library.
+ *	 Actually it's the same code with only minor changes in the
+ *	 way the data is stored; this is to support the abstraction
+ *	 of an optional secure memory allocation which may be used
+ *	 to avoid revealing of sensitive data due to paging etc.
+ *	 The GNU MP Library itself is published under the LGPL;
+ *	 however I decided to publish this code under the plain GPL.
+ */
+
+#include "mpi-internal.h"
+
+int mpi_sub_ui(MPI w, MPI u, unsigned long vval)
+{
+	if (u->nlimbs == 0) {
+		if (mpi_resize(w, 1) < 0)
+			return -ENOMEM;
+		w->d[0] = vval;
+		w->nlimbs = (vval != 0);
+		w->sign = (vval != 0);
+		return 0;
+	}
+
+	/* If not space for W (and possible carry), increase space. */
+	if (mpi_resize(w, u->nlimbs + 1))
+		return -ENOMEM;
+
+	if (u->sign) {
+		mpi_limb_t cy;
+
+		cy = mpihelp_add_1(w->d, u->d, u->nlimbs, (mpi_limb_t) vval);
+		w->d[u->nlimbs] = cy;
+		w->nlimbs = u->nlimbs + cy;
+		w->sign = 1;
+	} else {
+		/* The signs are different.  Need exact comparison to determine
+		 * which operand to subtract from which.
+		 */
+		if (u->nlimbs == 1 && u->d[0] < vval) {
+			w->d[0] = vval - u->d[0];
+			w->nlimbs = 1;
+			w->sign = 1;
+		} else {
+			mpihelp_sub_1(w->d, u->d, u->nlimbs, (mpi_limb_t) vval);
+			/* Size can decrease with at most one limb. */
+			w->nlimbs = (u->nlimbs - (w->d[u->nlimbs - 1] == 0));
+			w->sign = 0;
+		}
+	}
+
+	mpi_normalize(w);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mpi_sub_ui);
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 3/3] crypto: DH - check validity of Z before export
  2020-07-10 10:09 [PATCH 0/3] DH: SP800-56A rev 3 compliant shared secret Stephan Müller
  2020-07-10 10:10 ` [PATCH 1/3] crypto: ECDH - check validity of Z before export Stephan Müller
  2020-07-10 10:10 ` [PATCH 2/3] lib/mpi: Add mpi_sub_ui() Stephan Müller
@ 2020-07-10 10:15 ` Stephan Müller
  2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
  3 siblings, 0 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-10 10:15 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang

SP800-56A rev3 section 5.7.1.1 step 2 mandates that the validity of the
calculated shared secret is verified before the data is returned to the
caller. This patch adds the validation check.

Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/dh.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/crypto/dh.c b/crypto/dh.c
index 566f624a2de2..f84fd50ec79b 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -9,6 +9,7 @@
 #include <crypto/internal/kpp.h>
 #include <crypto/kpp.h>
 #include <crypto/dh.h>
+#include <linux/fips.h>
 #include <linux/mpi.h>
 
 struct dh_ctx {
@@ -179,6 +180,34 @@ static int dh_compute_value(struct kpp_request *req)
 	if (ret)
 		goto err_free_base;
 
+	/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
+	if (fips_enabled && req->src) {
+		MPI pone;
+
+		/* z <= 1 */
+		if (mpi_cmp_ui(val, 1) < 1) {
+			ret = -EBADMSG;
+			goto err_free_base;
+		}
+
+		/* z == p - 1 */
+		pone = mpi_alloc(0);
+
+		if (!pone) {
+			ret = -ENOMEM;
+			goto err_free_base;
+		}
+
+		ret = mpi_sub_ui(pone, ctx->p, 1);
+		if (!ret && !mpi_cmp(pone, val))
+			ret = -EBADMSG;
+
+		mpi_free(pone);
+
+		if (ret)
+			goto err_free_base;
+	}
+
 	ret = mpi_write_to_sgl(val, req->dst, req->dst_len, &sign);
 	if (ret)
 		goto err_free_base;
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/3] lib/mpi: Add mpi_sub_ui()
  2020-07-10 10:10 ` [PATCH 2/3] lib/mpi: Add mpi_sub_ui() Stephan Müller
@ 2020-07-10 14:42   ` Ard Biesheuvel
  2020-07-10 15:10     ` Stephan Mueller
  0 siblings, 1 reply; 42+ messages in thread
From: Ard Biesheuvel @ 2020-07-10 14:42 UTC (permalink / raw)
  To: Stephan Müller
  Cc: Herbert Xu, Linux Crypto Mailing List, Marcelo Cerri, Tianjia Zhang

On Fri, 10 Jul 2020 at 13:16, Stephan Müller <smueller@chronox.de> wrote:
>
> Add mpi_sub_ui() based on Gnu PG mpz_sub_ui() from mpz/aors_ui.h
> adapting the code to the kernel's structures and coding style and also
> removing the defines used to produce mpz_sub_ui() and mpz_add_ui()
> from the same code.
>

Isn't GnuPG GPLv3 ?


> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  include/linux/mpi.h  |  3 +++
>  lib/mpi/Makefile     |  1 +
>  lib/mpi/mpi-sub-ui.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+)
>  create mode 100644 lib/mpi/mpi-sub-ui.c
>
> diff --git a/include/linux/mpi.h b/include/linux/mpi.h
> index 7bd6d8af0004..5d906dfbf3ed 100644
> --- a/include/linux/mpi.h
> +++ b/include/linux/mpi.h
> @@ -63,6 +63,9 @@ int mpi_powm(MPI res, MPI base, MPI exp, MPI mod);
>  int mpi_cmp_ui(MPI u, ulong v);
>  int mpi_cmp(MPI u, MPI v);
>
> +/*-- mpi-sub-ui.c --*/
> +int mpi_sub_ui(MPI w, MPI u, unsigned long vval);
> +
>  /*-- mpi-bit.c --*/
>  void mpi_normalize(MPI a);
>  unsigned mpi_get_nbits(MPI a);
> diff --git a/lib/mpi/Makefile b/lib/mpi/Makefile
> index d5874a7f5ff9..43b8fce14079 100644
> --- a/lib/mpi/Makefile
> +++ b/lib/mpi/Makefile
> @@ -16,6 +16,7 @@ mpi-y = \
>         mpicoder.o                      \
>         mpi-bit.o                       \
>         mpi-cmp.o                       \
> +       mpi-sub-ui.o                    \
>         mpih-cmp.o                      \
>         mpih-div.o                      \
>         mpih-mul.o                      \
> diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> new file mode 100644
> index 000000000000..fa6b085bac36
> --- /dev/null
> +++ b/lib/mpi/mpi-sub-ui.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* mpi-sub-ui.c  -  MPI functions
> + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> + *      Free Software Foundation, Inc.
> + *
> + * This file is part of GnuPG.
> + *
> + * Note: This code is heavily based on the GNU MP Library.
> + *      Actually it's the same code with only minor changes in the
> + *      way the data is stored; this is to support the abstraction
> + *      of an optional secure memory allocation which may be used
> + *      to avoid revealing of sensitive data due to paging etc.
> + *      The GNU MP Library itself is published under the LGPL;
> + *      however I decided to publish this code under the plain GPL.
> + */
> +
> +#include "mpi-internal.h"
> +
> +int mpi_sub_ui(MPI w, MPI u, unsigned long vval)
> +{
> +       if (u->nlimbs == 0) {
> +               if (mpi_resize(w, 1) < 0)
> +                       return -ENOMEM;
> +               w->d[0] = vval;
> +               w->nlimbs = (vval != 0);
> +               w->sign = (vval != 0);
> +               return 0;
> +       }
> +
> +       /* If not space for W (and possible carry), increase space. */
> +       if (mpi_resize(w, u->nlimbs + 1))
> +               return -ENOMEM;
> +
> +       if (u->sign) {
> +               mpi_limb_t cy;
> +
> +               cy = mpihelp_add_1(w->d, u->d, u->nlimbs, (mpi_limb_t) vval);
> +               w->d[u->nlimbs] = cy;
> +               w->nlimbs = u->nlimbs + cy;
> +               w->sign = 1;
> +       } else {
> +               /* The signs are different.  Need exact comparison to determine
> +                * which operand to subtract from which.
> +                */
> +               if (u->nlimbs == 1 && u->d[0] < vval) {
> +                       w->d[0] = vval - u->d[0];
> +                       w->nlimbs = 1;
> +                       w->sign = 1;
> +               } else {
> +                       mpihelp_sub_1(w->d, u->d, u->nlimbs, (mpi_limb_t) vval);
> +                       /* Size can decrease with at most one limb. */
> +                       w->nlimbs = (u->nlimbs - (w->d[u->nlimbs - 1] == 0));
> +                       w->sign = 0;
> +               }
> +       }
> +
> +       mpi_normalize(w);
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(mpi_sub_ui);
> --
> 2.26.2
>
>
>
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/3] lib/mpi: Add mpi_sub_ui()
  2020-07-10 14:42   ` Ard Biesheuvel
@ 2020-07-10 15:10     ` Stephan Mueller
  0 siblings, 0 replies; 42+ messages in thread
From: Stephan Mueller @ 2020-07-10 15:10 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Herbert Xu, Linux Crypto Mailing List, Marcelo Cerri, Tianjia Zhang

Am Freitag, 10. Juli 2020, 16:42:39 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Fri, 10 Jul 2020 at 13:16, Stephan Müller <smueller@chronox.de> wrote:
> > Add mpi_sub_ui() based on Gnu PG mpz_sub_ui() from mpz/aors_ui.h
> > adapting the code to the kernel's structures and coding style and also
> > removing the defines used to produce mpz_sub_ui() and mpz_add_ui()
> > from the same code.
> 
> Isn't GnuPG GPLv3 ?

Thanks for pointing that out. It is actually a mix-up - the function is from 
Gnu MP and not GnuPG.

Gnu MP is GPLv2 and GPLv3.

I will have that corrected with a 2nd patch.

Thanks

Ciao
Stephan



^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks
  2020-07-10 10:09 [PATCH 0/3] DH: SP800-56A rev 3 compliant shared secret Stephan Müller
                   ` (2 preceding siblings ...)
  2020-07-10 10:15 ` [PATCH 3/3] crypto: DH - check validity of Z before export Stephan Müller
@ 2020-07-12 16:38 ` Stephan Müller
  2020-07-12 16:39   ` [PATCH v2 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
                     ` (5 more replies)
  3 siblings, 6 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-12 16:38 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

Hi,

This patch set adds the required checks to make all aspects of
(EC)DH compliant with SP800-56A rev 3 assuming that all keys
are ephemeral. The use of static keys adds yet additional
validations which are hard to achieve in the kernel.

SP800-56A rev 3 mandates various checks:

- validation of remote public key defined in section 5.6.2.2.2
  is already implemented in:

  * ECC: crypto_ecdh_shared_secret with the call of
    ecc_is_pubkey_valid_partial

  * FFC: dh_compute_val when the req->src is read and validated with
    dh_is_pubkey_valid

- validation of generated shared secret: The patch set adds the
  shared secret validation as defined by SP800-56A rev 3. For
  ECDH this only implies that the validation of the shared secret
  is moved before the shared secret is returned to the caller.

  For DH, the validation is required to be performed against the prime
  of the domain parameter set.

  This patch adds the MPI library file mpi_sub_ui that is required
  to calculate P - 1 for the DH check. It would be possible, though
  to simply set the LSB of the prime to 0 to obtain P - 1 (since
  P is odd per definition) which implies that mpi_sub_ui would not
  be needed. However, this would require a copy operation from
  the existing prime MPI value into a temporary MPI where the
  modification can be performed. Such copy operation is not available.
  Therefore, the solution with the addition of mpi_sub_ui was chosen.

  NOTE: The function mpi_sub_ui is also added with the patch set
  "[PATCH v5 2/8] lib/mpi: Extend the MPI library" currently sent
  to the linux-crypto mailing list.

- validation of the generated local public key: Patches 4 and 5 of
  this patch set adds the required checks.

Changes to v1:

- fix reference to Gnu MP as outlined by Ard Biesheuvel
- addition of patches 4 and 5

Marcelo Henrique Cerri (1):
  lib/mpi: Add mpi_sub_ui()

Stephan Mueller (4):
  crypto: ECDH - check validity of Z before export
  crypto: DH - check validity of Z before export
  crypto: DH SP800-56A rev 3 local public key validation
  crypto: ECDH SP800-56A rev 3 local public key validation

 crypto/dh.c          | 38 ++++++++++++++++++++++++++++
 crypto/ecc.c         | 42 ++++++++++++++++++++++++++++---
 crypto/ecc.h         | 14 +++++++++++
 include/linux/mpi.h  |  3 +++
 lib/mpi/Makefile     |  1 +
 lib/mpi/mpi-sub-ui.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 154 insertions(+), 4 deletions(-)
 create mode 100644 lib/mpi/mpi-sub-ui.c

-- 
2.26.2




^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 1/5] crypto: ECDH - check validity of Z before export
  2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
@ 2020-07-12 16:39   ` Stephan Müller
  2020-07-12 18:02     ` Vitaly Chikunov
  2020-07-15 13:17     ` Marcelo Henrique Cerri
  2020-07-12 16:39   ` [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui() Stephan Müller
                     ` (4 subsequent siblings)
  5 siblings, 2 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-12 16:39 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

SP800-56A rev3 section 5.7.1.2 step 2 mandates that the validity of the
calculated shared secret is verified before the data is returned to the
caller. Thus, the export function and the validity check functions are
reversed. In addition, the sensitive variables of priv and rand_z are
zeroized.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/ecc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 02d35be7702b..52e2d49262f2 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1495,11 +1495,16 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 
 	ecc_point_mult(product, pk, priv, rand_z, curve, ndigits);
 
-	ecc_swap_digits(product->x, secret, ndigits);
-
-	if (ecc_point_is_zero(product))
+	if (ecc_point_is_zero(product)) {
 		ret = -EFAULT;
+		goto err_validity;
+	}
+
+	ecc_swap_digits(product->x, secret, ndigits);
 
+err_validity:
+	memzero_explicit(priv, sizeof(priv));
+	memzero_explicit(rand_z, sizeof(rand_z));
 	ecc_free_point(product);
 err_alloc_product:
 	ecc_free_point(pk);
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
  2020-07-12 16:39   ` [PATCH v2 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
@ 2020-07-12 16:39   ` Stephan Müller
  2020-07-16  7:30     ` Herbert Xu
  2020-07-12 16:40   ` [PATCH v2 3/5] crypto: DH - check validity of Z before export Stephan Müller
                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 42+ messages in thread
From: Stephan Müller @ 2020-07-12 16:39 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

Add mpi_sub_ui() based on Gnu MP mpz_sub_ui() from mpz/aors_ui.h
adapting the code to the kernel's structures and coding style and also
removing the defines used to produce mpz_sub_ui() and mpz_add_ui()
from the same code.

Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 include/linux/mpi.h  |  3 +++
 lib/mpi/Makefile     |  1 +
 lib/mpi/mpi-sub-ui.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)
 create mode 100644 lib/mpi/mpi-sub-ui.c

diff --git a/include/linux/mpi.h b/include/linux/mpi.h
index 7bd6d8af0004..5d906dfbf3ed 100644
--- a/include/linux/mpi.h
+++ b/include/linux/mpi.h
@@ -63,6 +63,9 @@ int mpi_powm(MPI res, MPI base, MPI exp, MPI mod);
 int mpi_cmp_ui(MPI u, ulong v);
 int mpi_cmp(MPI u, MPI v);
 
+/*-- mpi-sub-ui.c --*/
+int mpi_sub_ui(MPI w, MPI u, unsigned long vval);
+
 /*-- mpi-bit.c --*/
 void mpi_normalize(MPI a);
 unsigned mpi_get_nbits(MPI a);
diff --git a/lib/mpi/Makefile b/lib/mpi/Makefile
index d5874a7f5ff9..43b8fce14079 100644
--- a/lib/mpi/Makefile
+++ b/lib/mpi/Makefile
@@ -16,6 +16,7 @@ mpi-y = \
 	mpicoder.o			\
 	mpi-bit.o			\
 	mpi-cmp.o			\
+	mpi-sub-ui.o			\
 	mpih-cmp.o			\
 	mpih-div.o			\
 	mpih-mul.o			\
diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
new file mode 100644
index 000000000000..fa6b085bac36
--- /dev/null
+++ b/lib/mpi/mpi-sub-ui.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* mpi-sub-ui.c  -  MPI functions
+ *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
+ *      Free Software Foundation, Inc.
+ *
+ * This file is part of GnuPG.
+ *
+ * Note: This code is heavily based on the GNU MP Library.
+ *	 Actually it's the same code with only minor changes in the
+ *	 way the data is stored; this is to support the abstraction
+ *	 of an optional secure memory allocation which may be used
+ *	 to avoid revealing of sensitive data due to paging etc.
+ *	 The GNU MP Library itself is published under the LGPL;
+ *	 however I decided to publish this code under the plain GPL.
+ */
+
+#include "mpi-internal.h"
+
+int mpi_sub_ui(MPI w, MPI u, unsigned long vval)
+{
+	if (u->nlimbs == 0) {
+		if (mpi_resize(w, 1) < 0)
+			return -ENOMEM;
+		w->d[0] = vval;
+		w->nlimbs = (vval != 0);
+		w->sign = (vval != 0);
+		return 0;
+	}
+
+	/* If not space for W (and possible carry), increase space. */
+	if (mpi_resize(w, u->nlimbs + 1))
+		return -ENOMEM;
+
+	if (u->sign) {
+		mpi_limb_t cy;
+
+		cy = mpihelp_add_1(w->d, u->d, u->nlimbs, (mpi_limb_t) vval);
+		w->d[u->nlimbs] = cy;
+		w->nlimbs = u->nlimbs + cy;
+		w->sign = 1;
+	} else {
+		/* The signs are different.  Need exact comparison to determine
+		 * which operand to subtract from which.
+		 */
+		if (u->nlimbs == 1 && u->d[0] < vval) {
+			w->d[0] = vval - u->d[0];
+			w->nlimbs = 1;
+			w->sign = 1;
+		} else {
+			mpihelp_sub_1(w->d, u->d, u->nlimbs, (mpi_limb_t) vval);
+			/* Size can decrease with at most one limb. */
+			w->nlimbs = (u->nlimbs - (w->d[u->nlimbs - 1] == 0));
+			w->sign = 0;
+		}
+	}
+
+	mpi_normalize(w);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mpi_sub_ui);
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v2 3/5] crypto: DH - check validity of Z before export
  2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
  2020-07-12 16:39   ` [PATCH v2 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
  2020-07-12 16:39   ` [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui() Stephan Müller
@ 2020-07-12 16:40   ` Stephan Müller
  2020-07-15 13:17     ` Marcelo Henrique Cerri
  2020-07-12 16:40   ` [PATCH v2 4/5] crypto: DH SP800-56A rev 3 local public key validation Stephan Müller
                     ` (2 subsequent siblings)
  5 siblings, 1 reply; 42+ messages in thread
From: Stephan Müller @ 2020-07-12 16:40 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

SP800-56A rev3 section 5.7.1.1 step 2 mandates that the validity of the
calculated shared secret is verified before the data is returned to the
caller. This patch adds the validation check.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/dh.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/crypto/dh.c b/crypto/dh.c
index 566f624a2de2..f84fd50ec79b 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -9,6 +9,7 @@
 #include <crypto/internal/kpp.h>
 #include <crypto/kpp.h>
 #include <crypto/dh.h>
+#include <linux/fips.h>
 #include <linux/mpi.h>
 
 struct dh_ctx {
@@ -179,6 +180,34 @@ static int dh_compute_value(struct kpp_request *req)
 	if (ret)
 		goto err_free_base;
 
+	/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
+	if (fips_enabled && req->src) {
+		MPI pone;
+
+		/* z <= 1 */
+		if (mpi_cmp_ui(val, 1) < 1) {
+			ret = -EBADMSG;
+			goto err_free_base;
+		}
+
+		/* z == p - 1 */
+		pone = mpi_alloc(0);
+
+		if (!pone) {
+			ret = -ENOMEM;
+			goto err_free_base;
+		}
+
+		ret = mpi_sub_ui(pone, ctx->p, 1);
+		if (!ret && !mpi_cmp(pone, val))
+			ret = -EBADMSG;
+
+		mpi_free(pone);
+
+		if (ret)
+			goto err_free_base;
+	}
+
 	ret = mpi_write_to_sgl(val, req->dst, req->dst_len, &sign);
 	if (ret)
 		goto err_free_base;
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v2 4/5] crypto: DH SP800-56A rev 3 local public key validation
  2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
                     ` (2 preceding siblings ...)
  2020-07-12 16:40   ` [PATCH v2 3/5] crypto: DH - check validity of Z before export Stephan Müller
@ 2020-07-12 16:40   ` Stephan Müller
  2020-07-15 13:18     ` Marcelo Henrique Cerri
  2020-07-12 16:42   ` [PATCH v2 5/5] crypto: ECDH " Stephan Müller
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
  5 siblings, 1 reply; 42+ messages in thread
From: Stephan Müller @ 2020-07-12 16:40 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

After the generation of a local public key, SP800-56A rev 3 section
5.6.2.1.3 mandates a validation of that key with a full validation
compliant to section 5.6.2.3.1.

Only if the full validation passes, the key is allowed to be used.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/dh.c | 59 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/crypto/dh.c b/crypto/dh.c
index f84fd50ec79b..cd4f32092e5c 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -180,32 +180,41 @@ static int dh_compute_value(struct kpp_request *req)
 	if (ret)
 		goto err_free_base;
 
-	/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
-	if (fips_enabled && req->src) {
-		MPI pone;
-
-		/* z <= 1 */
-		if (mpi_cmp_ui(val, 1) < 1) {
-			ret = -EBADMSG;
-			goto err_free_base;
-		}
-
-		/* z == p - 1 */
-		pone = mpi_alloc(0);
-
-		if (!pone) {
-			ret = -ENOMEM;
-			goto err_free_base;
+	if (fips_enabled) {
+		/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
+		if (req->src) {
+			MPI pone;
+
+			/* z <= 1 */
+			if (mpi_cmp_ui(val, 1) < 1) {
+				ret = -EBADMSG;
+				goto err_free_base;
+			}
+
+			/* z == p - 1 */
+			pone = mpi_alloc(0);
+
+			if (!pone) {
+				ret = -ENOMEM;
+				goto err_free_base;
+			}
+
+			ret = mpi_sub_ui(pone, ctx->p, 1);
+			if (!ret && !mpi_cmp(pone, val))
+				ret = -EBADMSG;
+
+			mpi_free(pone);
+
+			if (ret)
+				goto err_free_base;
+
+		/* SP800-56A rev 3 5.6.2.1.3 key check */
+		} else {
+			if (dh_is_pubkey_valid(ctx, val)) {
+				ret = -EAGAIN;
+				goto err_free_val;
+			}
 		}
-
-		ret = mpi_sub_ui(pone, ctx->p, 1);
-		if (!ret && !mpi_cmp(pone, val))
-			ret = -EBADMSG;
-
-		mpi_free(pone);
-
-		if (ret)
-			goto err_free_base;
 	}
 
 	ret = mpi_write_to_sgl(val, req->dst, req->dst_len, &sign);
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v2 5/5] crypto: ECDH SP800-56A rev 3 local public key validation
  2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
                     ` (3 preceding siblings ...)
  2020-07-12 16:40   ` [PATCH v2 4/5] crypto: DH SP800-56A rev 3 local public key validation Stephan Müller
@ 2020-07-12 16:42   ` Stephan Müller
  2020-07-12 18:06     ` Vitaly Chikunov
  2020-07-15 13:19     ` Marcelo Henrique Cerri
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
  5 siblings, 2 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-12 16:42 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

After the generation of a local public key, SP800-56A rev 3 section
5.6.2.1.3 mandates a validation of that key with a full validation
compliant to section 5.6.2.3.3.

Only if the full validation passes, the key is allowed to be used.

The patch adds the full key validation compliant to 5.6.2.3.3 and
performs the required check on the generated public key.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/ecc.c | 31 ++++++++++++++++++++++++++++++-
 crypto/ecc.h | 14 ++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 52e2d49262f2..7308487e7c55 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1404,7 +1404,9 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
 	}
 
 	ecc_point_mult(pk, &curve->g, priv, NULL, curve, ndigits);
-	if (ecc_point_is_zero(pk)) {
+
+	/* SP800-56A rev 3 5.6.2.1.3 key check */
+	if (ecc_is_pubkey_valid_full(curve, pk)) {
 		ret = -EAGAIN;
 		goto err_free_point;
 	}
@@ -1452,6 +1454,33 @@ int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
 }
 EXPORT_SYMBOL(ecc_is_pubkey_valid_partial);
 
+/* SP800-56A section 5.6.2.3.3 full verification */
+int ecc_is_pubkey_valid_full(const struct ecc_curve *curve,
+			     struct ecc_point *pk)
+{
+	struct ecc_point *nQ;
+
+	/* Checks 1 through 3 */
+	int ret = ecc_is_pubkey_valid_partial(curve, pk);
+
+	if (ret)
+		return ret;
+
+	/* Check 4: Verify that nQ is the zero point. */
+	nQ = ecc_alloc_point(pk->ndigits);
+	if (!nQ)
+		return -ENOMEM;
+
+	ecc_point_mult(nQ, pk, curve->n, NULL, curve, pk->ndigits);
+	if (!ecc_point_is_zero(nQ))
+		ret = -EINVAL;
+
+	ecc_free_point(nQ);
+
+	return ret;
+}
+EXPORT_SYMBOL(ecc_is_pubkey_valid_full);
+
 int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 			      const u64 *private_key, const u64 *public_key,
 			      u64 *secret)
diff --git a/crypto/ecc.h b/crypto/ecc.h
index ab0eb70b9c09..d4e546b9ad79 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -147,6 +147,20 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
 				struct ecc_point *pk);
 
+/**
+ * ecc_is_pubkey_valid_full() - Full public key validation
+ *
+ * @curve:		elliptic curve domain parameters
+ * @pk:			public key as a point
+ *
+ * Valdiate public key according to SP800-56A section 5.6.2.3.3 ECC Full
+ * Public-Key Validation Routine.
+ *
+ * Return: 0 if validation is successful, -EINVAL if validation is failed.
+ */
+int ecc_is_pubkey_valid_full(const struct ecc_curve *curve,
+			     struct ecc_point *pk);
+
 /**
  * vli_is_zero() - Determine is vli is zero
  *
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 1/5] crypto: ECDH - check validity of Z before export
  2020-07-12 16:39   ` [PATCH v2 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
@ 2020-07-12 18:02     ` Vitaly Chikunov
  2020-07-15 13:17     ` Marcelo Henrique Cerri
  1 sibling, 0 replies; 42+ messages in thread
From: Vitaly Chikunov @ 2020-07-12 18:02 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Marcelo Cerri, Tianjia Zhang,
	ard.biesheuvel, nhorman, simo

On Sun, Jul 12, 2020 at 06:39:26PM +0200, Stephan Müller wrote:
> SP800-56A rev3 section 5.7.1.2 step 2 mandates that the validity of the
> calculated shared secret is verified before the data is returned to the
> caller. Thus, the export function and the validity check functions are
> reversed. In addition, the sensitive variables of priv and rand_z are

Reviewed-by: Vitaly Chikunov <vt@altlinux.org>

> zeroized.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/ecc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 02d35be7702b..52e2d49262f2 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -1495,11 +1495,16 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  
>  	ecc_point_mult(product, pk, priv, rand_z, curve, ndigits);
>  
> -	ecc_swap_digits(product->x, secret, ndigits);
> -
> -	if (ecc_point_is_zero(product))
> +	if (ecc_point_is_zero(product)) {
>  		ret = -EFAULT;
> +		goto err_validity;
> +	}
> +
> +	ecc_swap_digits(product->x, secret, ndigits);
>  
> +err_validity:
> +	memzero_explicit(priv, sizeof(priv));
> +	memzero_explicit(rand_z, sizeof(rand_z));
>  	ecc_free_point(product);
>  err_alloc_product:
>  	ecc_free_point(pk);
> -- 
> 2.26.2
> 
> 
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 5/5] crypto: ECDH SP800-56A rev 3 local public key validation
  2020-07-12 16:42   ` [PATCH v2 5/5] crypto: ECDH " Stephan Müller
@ 2020-07-12 18:06     ` Vitaly Chikunov
  2020-07-13  5:04       ` Stephan Mueller
  2020-07-15 13:19     ` Marcelo Henrique Cerri
  1 sibling, 1 reply; 42+ messages in thread
From: Vitaly Chikunov @ 2020-07-12 18:06 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Marcelo Cerri, Tianjia Zhang,
	ard.biesheuvel, nhorman, simo

Stephan,

On Sun, Jul 12, 2020 at 06:42:14PM +0200, Stephan Müller wrote:
> After the generation of a local public key, SP800-56A rev 3 section
> 5.6.2.1.3 mandates a validation of that key with a full validation
> compliant to section 5.6.2.3.3.
> 
> Only if the full validation passes, the key is allowed to be used.
> 
> The patch adds the full key validation compliant to 5.6.2.3.3 and
> performs the required check on the generated public key.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/ecc.c | 31 ++++++++++++++++++++++++++++++-
>  crypto/ecc.h | 14 ++++++++++++++
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 52e2d49262f2..7308487e7c55 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -1404,7 +1404,9 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
>  	}
>  
>  	ecc_point_mult(pk, &curve->g, priv, NULL, curve, ndigits);
> -	if (ecc_point_is_zero(pk)) {
> +
> +	/* SP800-56A rev 3 5.6.2.1.3 key check */
> +	if (ecc_is_pubkey_valid_full(curve, pk)) {
>  		ret = -EAGAIN;
>  		goto err_free_point;
>  	}
> @@ -1452,6 +1454,33 @@ int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
>  }
>  EXPORT_SYMBOL(ecc_is_pubkey_valid_partial);
>  
> +/* SP800-56A section 5.6.2.3.3 full verification */

Btw, 5.6.2.3.3 is partial validation, 5.6.2.3.2 is full validation
routine.

Thanks,

> +int ecc_is_pubkey_valid_full(const struct ecc_curve *curve,
> +			     struct ecc_point *pk)
> +{
> +	struct ecc_point *nQ;
> +
> +	/* Checks 1 through 3 */
> +	int ret = ecc_is_pubkey_valid_partial(curve, pk);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Check 4: Verify that nQ is the zero point. */
> +	nQ = ecc_alloc_point(pk->ndigits);
> +	if (!nQ)
> +		return -ENOMEM;
> +
> +	ecc_point_mult(nQ, pk, curve->n, NULL, curve, pk->ndigits);
> +	if (!ecc_point_is_zero(nQ))
> +		ret = -EINVAL;
> +
> +	ecc_free_point(nQ);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ecc_is_pubkey_valid_full);
> +
>  int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  			      const u64 *private_key, const u64 *public_key,
>  			      u64 *secret)
> diff --git a/crypto/ecc.h b/crypto/ecc.h
> index ab0eb70b9c09..d4e546b9ad79 100644
> --- a/crypto/ecc.h
> +++ b/crypto/ecc.h
> @@ -147,6 +147,20 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
>  				struct ecc_point *pk);
>  
> +/**
> + * ecc_is_pubkey_valid_full() - Full public key validation
> + *
> + * @curve:		elliptic curve domain parameters
> + * @pk:			public key as a point
> + *
> + * Valdiate public key according to SP800-56A section 5.6.2.3.3 ECC Full
> + * Public-Key Validation Routine.
> + *
> + * Return: 0 if validation is successful, -EINVAL if validation is failed.
> + */
> +int ecc_is_pubkey_valid_full(const struct ecc_curve *curve,
> +			     struct ecc_point *pk);
> +
>  /**
>   * vli_is_zero() - Determine is vli is zero
>   *
> -- 
> 2.26.2
> 
> 
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 5/5] crypto: ECDH SP800-56A rev 3 local public key validation
  2020-07-12 18:06     ` Vitaly Chikunov
@ 2020-07-13  5:04       ` Stephan Mueller
  2020-07-13  5:59         ` Vitaly Chikunov
  0 siblings, 1 reply; 42+ messages in thread
From: Stephan Mueller @ 2020-07-13  5:04 UTC (permalink / raw)
  To: Stephan Müller, herbert, linux-crypto, Marcelo Cerri,
	Tianjia Zhang, ard.biesheuvel, nhorman, simo

Am Sonntag, 12. Juli 2020, 20:06:13 CEST schrieb Vitaly Chikunov:

Hi Vitaly,

> Stephan,
> 
> On Sun, Jul 12, 2020 at 06:42:14PM +0200, Stephan Müller wrote:
> > After the generation of a local public key, SP800-56A rev 3 section
> > 5.6.2.1.3 mandates a validation of that key with a full validation
> > compliant to section 5.6.2.3.3.
> > 
> > Only if the full validation passes, the key is allowed to be used.
> > 
> > The patch adds the full key validation compliant to 5.6.2.3.3 and
> > performs the required check on the generated public key.
> > 
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> > ---
> > 
> >  crypto/ecc.c | 31 ++++++++++++++++++++++++++++++-
> >  crypto/ecc.h | 14 ++++++++++++++
> >  2 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/crypto/ecc.c b/crypto/ecc.c
> > index 52e2d49262f2..7308487e7c55 100644
> > --- a/crypto/ecc.c
> > +++ b/crypto/ecc.c
> > @@ -1404,7 +1404,9 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned
> > int ndigits,> 
> >  	}
> >  	
> >  	ecc_point_mult(pk, &curve->g, priv, NULL, curve, ndigits);
> > 
> > -	if (ecc_point_is_zero(pk)) {
> > +
> > +	/* SP800-56A rev 3 5.6.2.1.3 key check */
> > +	if (ecc_is_pubkey_valid_full(curve, pk)) {
> > 
> >  		ret = -EAGAIN;
> >  		goto err_free_point;
> >  	
> >  	}
> > 
> > @@ -1452,6 +1454,33 @@ int ecc_is_pubkey_valid_partial(const struct
> > ecc_curve *curve,> 
> >  }
> >  EXPORT_SYMBOL(ecc_is_pubkey_valid_partial);
> > 
> > +/* SP800-56A section 5.6.2.3.3 full verification */
> 
> Btw, 5.6.2.3.3 is partial validation, 5.6.2.3.2 is full validation
> routine.

Looking at SP800-56A revision 3 from April 2018 I see:

"5.6.2.3.3 ECC Full Public-Key Validation Routine"

Thanks for the review.

Ciao
Stephan



^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 5/5] crypto: ECDH SP800-56A rev 3 local public key validation
  2020-07-13  5:04       ` Stephan Mueller
@ 2020-07-13  5:59         ` Vitaly Chikunov
  2020-07-13  6:02           ` Stephan Müller
  0 siblings, 1 reply; 42+ messages in thread
From: Vitaly Chikunov @ 2020-07-13  5:59 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: herbert, linux-crypto, Marcelo Cerri, Tianjia Zhang,
	ard.biesheuvel, nhorman, simo

On Mon, Jul 13, 2020 at 07:04:39AM +0200, Stephan Mueller wrote:
> Am Sonntag, 12. Juli 2020, 20:06:13 CEST schrieb Vitaly Chikunov:
> 
> Hi Vitaly,
> 
> > Stephan,
> > 
> > On Sun, Jul 12, 2020 at 06:42:14PM +0200, Stephan Müller wrote:
> > > After the generation of a local public key, SP800-56A rev 3 section
> > > 5.6.2.1.3 mandates a validation of that key with a full validation
> > > compliant to section 5.6.2.3.3.
> > > 
> > > Only if the full validation passes, the key is allowed to be used.
> > > 
> > > The patch adds the full key validation compliant to 5.6.2.3.3 and
> > > performs the required check on the generated public key.
> > > 
> > > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> > > ---
> > > 
> > >  crypto/ecc.c | 31 ++++++++++++++++++++++++++++++-
> > >  crypto/ecc.h | 14 ++++++++++++++
> > >  2 files changed, 44 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/crypto/ecc.c b/crypto/ecc.c
> > > index 52e2d49262f2..7308487e7c55 100644
> > > --- a/crypto/ecc.c
> > > +++ b/crypto/ecc.c
> > > @@ -1404,7 +1404,9 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned
> > > int ndigits,> 
> > >  	}
> > >  	
> > >  	ecc_point_mult(pk, &curve->g, priv, NULL, curve, ndigits);
> > > 
> > > -	if (ecc_point_is_zero(pk)) {
> > > +
> > > +	/* SP800-56A rev 3 5.6.2.1.3 key check */
> > > +	if (ecc_is_pubkey_valid_full(curve, pk)) {
> > > 
> > >  		ret = -EAGAIN;
> > >  		goto err_free_point;
> > >  	
> > >  	}
> > > 
> > > @@ -1452,6 +1454,33 @@ int ecc_is_pubkey_valid_partial(const struct
> > > ecc_curve *curve,> 
> > >  }
> > >  EXPORT_SYMBOL(ecc_is_pubkey_valid_partial);
> > > 
> > > +/* SP800-56A section 5.6.2.3.3 full verification */
> > 
> > Btw, 5.6.2.3.3 is partial validation, 5.6.2.3.2 is full validation
> > routine.
> 
> Looking at SP800-56A revision 3 from April 2018 I see:
> 
> "5.6.2.3.3 ECC Full Public-Key Validation Routine"

You are right. I looked at

  https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Ar2.pdf

which is Rev 2. And in Rev 3 they inserted `5.6.2.3.2 FFC Partial Public-Key
Validation Routine', so ECC paragraph numbers are shifted up.

Thanks,


> 
> Thanks for the review.
> 
> Ciao
> Stephan
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 5/5] crypto: ECDH SP800-56A rev 3 local public key validation
  2020-07-13  5:59         ` Vitaly Chikunov
@ 2020-07-13  6:02           ` Stephan Müller
  0 siblings, 0 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-13  6:02 UTC (permalink / raw)
  To: Stephan Mueller, herbert, linux-crypto, Marcelo Cerri,
	Tianjia Zhang, ard.biesheuvel, nhorman, simo

Am Montag, 13. Juli 2020, 07:59:50 CEST schrieb Vitaly Chikunov:

Hi Vitaly,

> > > > +/* SP800-56A section 5.6.2.3.3 full verification */
> > > 
> > > Btw, 5.6.2.3.3 is partial validation, 5.6.2.3.2 is full validation
> > > routine.
> > 
> > Looking at SP800-56A revision 3 from April 2018 I see:
> > 
> > "5.6.2.3.3 ECC Full Public-Key Validation Routine"
> 
> You are right. I looked at
> 
>  
> https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Ar2.pdf
> 
> which is Rev 2. And in Rev 3 they inserted `5.6.2.3.2 FFC Partial Public-Key
> Validation Routine', so ECC paragraph numbers are shifted up.

Thank you for the confirmation.

Ciao
Stephan



^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 1/5] crypto: ECDH - check validity of Z before export
  2020-07-12 16:39   ` [PATCH v2 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
  2020-07-12 18:02     ` Vitaly Chikunov
@ 2020-07-15 13:17     ` Marcelo Henrique Cerri
  1 sibling, 0 replies; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-15 13:17 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Tianjia Zhang, ard.biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

Reviewed-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Tested-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

On Sun, Jul 12, 2020 at 06:39:26PM +0200, Stephan Müller wrote:
> SP800-56A rev3 section 5.7.1.2 step 2 mandates that the validity of the
> calculated shared secret is verified before the data is returned to the
> caller. Thus, the export function and the validity check functions are
> reversed. In addition, the sensitive variables of priv and rand_z are
> zeroized.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/ecc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 02d35be7702b..52e2d49262f2 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -1495,11 +1495,16 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  
>  	ecc_point_mult(product, pk, priv, rand_z, curve, ndigits);
>  
> -	ecc_swap_digits(product->x, secret, ndigits);
> -
> -	if (ecc_point_is_zero(product))
> +	if (ecc_point_is_zero(product)) {
>  		ret = -EFAULT;
> +		goto err_validity;
> +	}
> +
> +	ecc_swap_digits(product->x, secret, ndigits);
>  
> +err_validity:
> +	memzero_explicit(priv, sizeof(priv));
> +	memzero_explicit(rand_z, sizeof(rand_z));
>  	ecc_free_point(product);
>  err_alloc_product:
>  	ecc_free_point(pk);
> -- 
> 2.26.2
> 
> 
> 
> 

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 3/5] crypto: DH - check validity of Z before export
  2020-07-12 16:40   ` [PATCH v2 3/5] crypto: DH - check validity of Z before export Stephan Müller
@ 2020-07-15 13:17     ` Marcelo Henrique Cerri
  0 siblings, 0 replies; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-15 13:17 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Tianjia Zhang, ard.biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

Reviewed-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Tested-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

On Sun, Jul 12, 2020 at 06:40:20PM +0200, Stephan Müller wrote:
> SP800-56A rev3 section 5.7.1.1 step 2 mandates that the validity of the
> calculated shared secret is verified before the data is returned to the
> caller. This patch adds the validation check.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/dh.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/crypto/dh.c b/crypto/dh.c
> index 566f624a2de2..f84fd50ec79b 100644
> --- a/crypto/dh.c
> +++ b/crypto/dh.c
> @@ -9,6 +9,7 @@
>  #include <crypto/internal/kpp.h>
>  #include <crypto/kpp.h>
>  #include <crypto/dh.h>
> +#include <linux/fips.h>
>  #include <linux/mpi.h>
>  
>  struct dh_ctx {
> @@ -179,6 +180,34 @@ static int dh_compute_value(struct kpp_request *req)
>  	if (ret)
>  		goto err_free_base;
>  
> +	/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
> +	if (fips_enabled && req->src) {
> +		MPI pone;
> +
> +		/* z <= 1 */
> +		if (mpi_cmp_ui(val, 1) < 1) {
> +			ret = -EBADMSG;
> +			goto err_free_base;
> +		}
> +
> +		/* z == p - 1 */
> +		pone = mpi_alloc(0);
> +
> +		if (!pone) {
> +			ret = -ENOMEM;
> +			goto err_free_base;
> +		}
> +
> +		ret = mpi_sub_ui(pone, ctx->p, 1);
> +		if (!ret && !mpi_cmp(pone, val))
> +			ret = -EBADMSG;
> +
> +		mpi_free(pone);
> +
> +		if (ret)
> +			goto err_free_base;
> +	}
> +
>  	ret = mpi_write_to_sgl(val, req->dst, req->dst_len, &sign);
>  	if (ret)
>  		goto err_free_base;
> -- 
> 2.26.2
> 
> 
> 
> 

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 4/5] crypto: DH SP800-56A rev 3 local public key validation
  2020-07-12 16:40   ` [PATCH v2 4/5] crypto: DH SP800-56A rev 3 local public key validation Stephan Müller
@ 2020-07-15 13:18     ` Marcelo Henrique Cerri
  0 siblings, 0 replies; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-15 13:18 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Tianjia Zhang, ard.biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

Reviewed-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Tested-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

On Sun, Jul 12, 2020 at 06:40:57PM +0200, Stephan Müller wrote:
> After the generation of a local public key, SP800-56A rev 3 section
> 5.6.2.1.3 mandates a validation of that key with a full validation
> compliant to section 5.6.2.3.1.
> 
> Only if the full validation passes, the key is allowed to be used.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/dh.c | 59 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/crypto/dh.c b/crypto/dh.c
> index f84fd50ec79b..cd4f32092e5c 100644
> --- a/crypto/dh.c
> +++ b/crypto/dh.c
> @@ -180,32 +180,41 @@ static int dh_compute_value(struct kpp_request *req)
>  	if (ret)
>  		goto err_free_base;
>  
> -	/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
> -	if (fips_enabled && req->src) {
> -		MPI pone;
> -
> -		/* z <= 1 */
> -		if (mpi_cmp_ui(val, 1) < 1) {
> -			ret = -EBADMSG;
> -			goto err_free_base;
> -		}
> -
> -		/* z == p - 1 */
> -		pone = mpi_alloc(0);
> -
> -		if (!pone) {
> -			ret = -ENOMEM;
> -			goto err_free_base;
> +	if (fips_enabled) {
> +		/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
> +		if (req->src) {
> +			MPI pone;
> +
> +			/* z <= 1 */
> +			if (mpi_cmp_ui(val, 1) < 1) {
> +				ret = -EBADMSG;
> +				goto err_free_base;
> +			}
> +
> +			/* z == p - 1 */
> +			pone = mpi_alloc(0);
> +
> +			if (!pone) {
> +				ret = -ENOMEM;
> +				goto err_free_base;
> +			}
> +
> +			ret = mpi_sub_ui(pone, ctx->p, 1);
> +			if (!ret && !mpi_cmp(pone, val))
> +				ret = -EBADMSG;
> +
> +			mpi_free(pone);
> +
> +			if (ret)
> +				goto err_free_base;
> +
> +		/* SP800-56A rev 3 5.6.2.1.3 key check */
> +		} else {
> +			if (dh_is_pubkey_valid(ctx, val)) {
> +				ret = -EAGAIN;
> +				goto err_free_val;
> +			}
>  		}
> -
> -		ret = mpi_sub_ui(pone, ctx->p, 1);
> -		if (!ret && !mpi_cmp(pone, val))
> -			ret = -EBADMSG;
> -
> -		mpi_free(pone);
> -
> -		if (ret)
> -			goto err_free_base;
>  	}
>  
>  	ret = mpi_write_to_sgl(val, req->dst, req->dst_len, &sign);
> -- 
> 2.26.2
> 
> 
> 
> 

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 5/5] crypto: ECDH SP800-56A rev 3 local public key validation
  2020-07-12 16:42   ` [PATCH v2 5/5] crypto: ECDH " Stephan Müller
  2020-07-12 18:06     ` Vitaly Chikunov
@ 2020-07-15 13:19     ` Marcelo Henrique Cerri
  1 sibling, 0 replies; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-15 13:19 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Tianjia Zhang, ard.biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]

Reviewed-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Tested-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

On Sun, Jul 12, 2020 at 06:42:14PM +0200, Stephan Müller wrote:
> After the generation of a local public key, SP800-56A rev 3 section
> 5.6.2.1.3 mandates a validation of that key with a full validation
> compliant to section 5.6.2.3.3.
> 
> Only if the full validation passes, the key is allowed to be used.
> 
> The patch adds the full key validation compliant to 5.6.2.3.3 and
> performs the required check on the generated public key.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/ecc.c | 31 ++++++++++++++++++++++++++++++-
>  crypto/ecc.h | 14 ++++++++++++++
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 52e2d49262f2..7308487e7c55 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -1404,7 +1404,9 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
>  	}
>  
>  	ecc_point_mult(pk, &curve->g, priv, NULL, curve, ndigits);
> -	if (ecc_point_is_zero(pk)) {
> +
> +	/* SP800-56A rev 3 5.6.2.1.3 key check */
> +	if (ecc_is_pubkey_valid_full(curve, pk)) {
>  		ret = -EAGAIN;
>  		goto err_free_point;
>  	}
> @@ -1452,6 +1454,33 @@ int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
>  }
>  EXPORT_SYMBOL(ecc_is_pubkey_valid_partial);
>  
> +/* SP800-56A section 5.6.2.3.3 full verification */
> +int ecc_is_pubkey_valid_full(const struct ecc_curve *curve,
> +			     struct ecc_point *pk)
> +{
> +	struct ecc_point *nQ;
> +
> +	/* Checks 1 through 3 */
> +	int ret = ecc_is_pubkey_valid_partial(curve, pk);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Check 4: Verify that nQ is the zero point. */
> +	nQ = ecc_alloc_point(pk->ndigits);
> +	if (!nQ)
> +		return -ENOMEM;
> +
> +	ecc_point_mult(nQ, pk, curve->n, NULL, curve, pk->ndigits);
> +	if (!ecc_point_is_zero(nQ))
> +		ret = -EINVAL;
> +
> +	ecc_free_point(nQ);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ecc_is_pubkey_valid_full);
> +
>  int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  			      const u64 *private_key, const u64 *public_key,
>  			      u64 *secret)
> diff --git a/crypto/ecc.h b/crypto/ecc.h
> index ab0eb70b9c09..d4e546b9ad79 100644
> --- a/crypto/ecc.h
> +++ b/crypto/ecc.h
> @@ -147,6 +147,20 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
>  				struct ecc_point *pk);
>  
> +/**
> + * ecc_is_pubkey_valid_full() - Full public key validation
> + *
> + * @curve:		elliptic curve domain parameters
> + * @pk:			public key as a point
> + *
> + * Valdiate public key according to SP800-56A section 5.6.2.3.3 ECC Full
> + * Public-Key Validation Routine.
> + *
> + * Return: 0 if validation is successful, -EINVAL if validation is failed.
> + */
> +int ecc_is_pubkey_valid_full(const struct ecc_curve *curve,
> +			     struct ecc_point *pk);
> +
>  /**
>   * vli_is_zero() - Determine is vli is zero
>   *
> -- 
> 2.26.2
> 
> 
> 
> 

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-12 16:39   ` [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui() Stephan Müller
@ 2020-07-16  7:30     ` Herbert Xu
  2020-07-16  8:41       ` Ard Biesheuvel
  0 siblings, 1 reply; 42+ messages in thread
From: Herbert Xu @ 2020-07-16  7:30 UTC (permalink / raw)
  To: Stephan Müller
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
>
> diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> new file mode 100644
> index 000000000000..fa6b085bac36
> --- /dev/null
> +++ b/lib/mpi/mpi-sub-ui.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* mpi-sub-ui.c  -  MPI functions
> + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> + *      Free Software Foundation, Inc.
> + *
> + * This file is part of GnuPG.
> + *
> + * Note: This code is heavily based on the GNU MP Library.
> + *	 Actually it's the same code with only minor changes in the
> + *	 way the data is stored; this is to support the abstraction
> + *	 of an optional secure memory allocation which may be used
> + *	 to avoid revealing of sensitive data due to paging etc.
> + *	 The GNU MP Library itself is published under the LGPL;
> + *	 however I decided to publish this code under the plain GPL.
> + */

Hmm, you said that this code is from GNU MP.  But this notice clearly
says that it's part of GnuPG and is under GPL.  Though it doesn't
clarify what version of GPL it is.  Can you please clarify this with
the author?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16  7:30     ` Herbert Xu
@ 2020-07-16  8:41       ` Ard Biesheuvel
  2020-07-16 12:50         ` Marcelo Henrique Cerri
  0 siblings, 1 reply; 42+ messages in thread
From: Ard Biesheuvel @ 2020-07-16  8:41 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Stephan Müller, Linux Crypto Mailing List, Marcelo Cerri,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> >
> > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > new file mode 100644
> > index 000000000000..fa6b085bac36
> > --- /dev/null
> > +++ b/lib/mpi/mpi-sub-ui.c
> > @@ -0,0 +1,60 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/* mpi-sub-ui.c  -  MPI functions
> > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > + *      Free Software Foundation, Inc.
> > + *
> > + * This file is part of GnuPG.
> > + *
> > + * Note: This code is heavily based on the GNU MP Library.
> > + *    Actually it's the same code with only minor changes in the
> > + *    way the data is stored; this is to support the abstraction
> > + *    of an optional secure memory allocation which may be used
> > + *    to avoid revealing of sensitive data due to paging etc.
> > + *    The GNU MP Library itself is published under the LGPL;
> > + *    however I decided to publish this code under the plain GPL.
> > + */
>
> Hmm, you said that this code is from GNU MP.  But this notice clearly
> says that it's part of GnuPG and is under GPL.  Though it doesn't
> clarify what version of GPL it is.  Can you please clarify this with
> the author?
>

GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
copyright years and the explicit statements that the file is part of
GnuPG and not under the original LGPL license, there is no way we can
take this code under the kernel's GPLv2 license.

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16  8:41       ` Ard Biesheuvel
@ 2020-07-16 12:50         ` Marcelo Henrique Cerri
  2020-07-16 13:09           ` Ard Biesheuvel
  0 siblings, 1 reply; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-16 12:50 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Herbert Xu, Stephan Müller, Linux Crypto Mailing List,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 5729 bytes --]

No. The code is really based on Gnu MP. I used the header from
lib/mpi/mpi-pow.c as reference and that's source of the mention to
GnuPG that went unnoticed by me.

You can find the original Gnu MP source that I used as reference in
the file gmp-6.2.0/mpz/aors_ui.h from:

https://gmplib.org/download/gmp/gmp-6.2.0.tar.lz

I'm pasting the contents of gmp-6.2.0/mpz/aors_ui.h below for
reference. Do you think we should use or adapt the original header
instead?

That said, assuming the patch set submitted by Tianjia is updated to
ensure that mpi_sub_ui() and other functions are returning allocation
errors, we could drop this patch in favor of that patch set that is
more extensive and also provides an implementation to mpi_sub_ui().


--->8---
/* mpz_add_ui, mpz_sub_ui -- Add or subtract an mpz_t and an unsigned
   one-word integer.

Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
Free Software Foundation, Inc.

This file is part of the GNU MP Library.

The GNU MP Library is free software; you can redistribute it and/or modify
it under the terms of either:

  * the GNU Lesser General Public License as published by the Free
    Software Foundation; either version 3 of the License, or (at your
    option) any later version.

or

  * the GNU General Public License as published by the Free Software
    Foundation; either version 2 of the License, or (at your option) any
    later version.

or both in parallel, as here.

The GNU MP Library is distributed in the hope that it will be useful, but
WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
for more details.

You should have received copies of the GNU General Public License and the
GNU Lesser General Public License along with the GNU MP Library.  If not,
see https://www.gnu.org/licenses/.  */

#include "gmp-impl.h"


#ifdef OPERATION_add_ui
#define FUNCTION          mpz_add_ui
#define FUNCTION2         mpz_add
#define VARIATION_CMP     >=
#define VARIATION_NEG
#define VARIATION_UNNEG   -
#endif

#ifdef OPERATION_sub_ui
#define FUNCTION          mpz_sub_ui
#define FUNCTION2         mpz_sub
#define VARIATION_CMP     <
#define VARIATION_NEG     -
#define VARIATION_UNNEG
#endif

#ifndef FUNCTION
Error, need OPERATION_add_ui or OPERATION_sub_ui
#endif


void
FUNCTION (mpz_ptr w, mpz_srcptr u, unsigned long int vval)
{
  mp_srcptr up;
  mp_ptr wp;
  mp_size_t usize, wsize;
  mp_size_t abs_usize;

#if BITS_PER_ULONG > GMP_NUMB_BITS  /* avoid warnings about shift amount */
  if (vval > GMP_NUMB_MAX)
    {
      mpz_t v;
      mp_limb_t vl[2];
      PTR(v) = vl;
      vl[0] = vval & GMP_NUMB_MASK;
      vl[1] = vval >> GMP_NUMB_BITS;
      SIZ(v) = 2;
      FUNCTION2 (w, u, v);
      return;
    }
#endif

  usize = SIZ (u);
  if (usize == 0)
    {
      MPZ_NEWALLOC (w, 1)[0] = vval;
      SIZ (w) = VARIATION_NEG (vval != 0);
      return;
    }

  abs_usize = ABS (usize);

  /* If not space for W (and possible carry), increase space.  */
  wp = MPZ_REALLOC (w, abs_usize + 1);

  /* These must be after realloc (U may be the same as W).  */
  up = PTR (u);

  if (usize VARIATION_CMP 0)
    {
      mp_limb_t cy;
      cy = mpn_add_1 (wp, up, abs_usize, (mp_limb_t) vval);
      wp[abs_usize] = cy;
      wsize = VARIATION_NEG (abs_usize + cy);
    }
  else
    {
      /* The signs are different.  Need exact comparison to determine
	 which operand to subtract from which.  */
      if (abs_usize == 1 && up[0] < vval)
	{
	  wp[0] = vval - up[0];
	  wsize = VARIATION_NEG 1;
	}
      else
	{
	  mpn_sub_1 (wp, up, abs_usize, (mp_limb_t) vval);
	  /* Size can decrease with at most one limb.  */
	  wsize = VARIATION_UNNEG (abs_usize - (wp[abs_usize - 1] == 0));
	}
    }

  SIZ (w) = wsize;
}
--->*---



On Thu, Jul 16, 2020 at 11:41:17AM +0300, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> > >
> > > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > > new file mode 100644
> > > index 000000000000..fa6b085bac36
> > > --- /dev/null
> > > +++ b/lib/mpi/mpi-sub-ui.c
> > > @@ -0,0 +1,60 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/* mpi-sub-ui.c  -  MPI functions
> > > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > + *      Free Software Foundation, Inc.
> > > + *
> > > + * This file is part of GnuPG.
> > > + *
> > > + * Note: This code is heavily based on the GNU MP Library.
> > > + *    Actually it's the same code with only minor changes in the
> > > + *    way the data is stored; this is to support the abstraction
> > > + *    of an optional secure memory allocation which may be used
> > > + *    to avoid revealing of sensitive data due to paging etc.
> > > + *    The GNU MP Library itself is published under the LGPL;
> > > + *    however I decided to publish this code under the plain GPL.
> > > + */
> >
> > Hmm, you said that this code is from GNU MP.  But this notice clearly
> > says that it's part of GnuPG and is under GPL.  Though it doesn't
> > clarify what version of GPL it is.  Can you please clarify this with
> > the author?
> >
> 
> GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
> copyright years and the explicit statements that the file is part of
> GnuPG and not under the original LGPL license, there is no way we can
> take this code under the kernel's GPLv2 license.

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16 12:50         ` Marcelo Henrique Cerri
@ 2020-07-16 13:09           ` Ard Biesheuvel
  2020-07-16 13:41             ` Marcelo Henrique Cerri
  0 siblings, 1 reply; 42+ messages in thread
From: Ard Biesheuvel @ 2020-07-16 13:09 UTC (permalink / raw)
  To: Marcelo Henrique Cerri
  Cc: Herbert Xu, Stephan Müller, Linux Crypto Mailing List,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
<marcelo.cerri@canonical.com> wrote:
>
> No. The code is really based on Gnu MP. I used the header from
> lib/mpi/mpi-pow.c as reference and that's source of the mention to
> GnuPG that went unnoticed by me.
>

So where did the file lib/mpi/mpi-sub-ui.c come from? From GnuPG or
from GnuMP? Did you modify the license statement? Because as proposed,
this patch clearly is not acceptable from GPL compliance  point of
view.



> You can find the original Gnu MP source that I used as reference in
> the file gmp-6.2.0/mpz/aors_ui.h from:
>
> https://gmplib.org/download/gmp/gmp-6.2.0.tar.lz
>
> I'm pasting the contents of gmp-6.2.0/mpz/aors_ui.h below for
> reference. Do you think we should use or adapt the original header
> instead?
>
> That said, assuming the patch set submitted by Tianjia is updated to
> ensure that mpi_sub_ui() and other functions are returning allocation
> errors, we could drop this patch in favor of that patch set that is
> more extensive and also provides an implementation to mpi_sub_ui().
>
>
> --->8---
> /* mpz_add_ui, mpz_sub_ui -- Add or subtract an mpz_t and an unsigned
>    one-word integer.
>
> Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> Free Software Foundation, Inc.
>
> This file is part of the GNU MP Library.
>
> The GNU MP Library is free software; you can redistribute it and/or modify
> it under the terms of either:
>
>   * the GNU Lesser General Public License as published by the Free
>     Software Foundation; either version 3 of the License, or (at your
>     option) any later version.
>
> or
>
>   * the GNU General Public License as published by the Free Software
>     Foundation; either version 2 of the License, or (at your option) any
>     later version.
>
> or both in parallel, as here.
>
> The GNU MP Library is distributed in the hope that it will be useful, but
> WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> for more details.
>
> You should have received copies of the GNU General Public License and the
> GNU Lesser General Public License along with the GNU MP Library.  If not,
> see https://www.gnu.org/licenses/.  */
>
> #include "gmp-impl.h"
>
>
> #ifdef OPERATION_add_ui
> #define FUNCTION          mpz_add_ui
> #define FUNCTION2         mpz_add
> #define VARIATION_CMP     >=
> #define VARIATION_NEG
> #define VARIATION_UNNEG   -
> #endif
>
> #ifdef OPERATION_sub_ui
> #define FUNCTION          mpz_sub_ui
> #define FUNCTION2         mpz_sub
> #define VARIATION_CMP     <
> #define VARIATION_NEG     -
> #define VARIATION_UNNEG
> #endif
>
> #ifndef FUNCTION
> Error, need OPERATION_add_ui or OPERATION_sub_ui
> #endif
>
>
> void
> FUNCTION (mpz_ptr w, mpz_srcptr u, unsigned long int vval)
> {
>   mp_srcptr up;
>   mp_ptr wp;
>   mp_size_t usize, wsize;
>   mp_size_t abs_usize;
>
> #if BITS_PER_ULONG > GMP_NUMB_BITS  /* avoid warnings about shift amount */
>   if (vval > GMP_NUMB_MAX)
>     {
>       mpz_t v;
>       mp_limb_t vl[2];
>       PTR(v) = vl;
>       vl[0] = vval & GMP_NUMB_MASK;
>       vl[1] = vval >> GMP_NUMB_BITS;
>       SIZ(v) = 2;
>       FUNCTION2 (w, u, v);
>       return;
>     }
> #endif
>
>   usize = SIZ (u);
>   if (usize == 0)
>     {
>       MPZ_NEWALLOC (w, 1)[0] = vval;
>       SIZ (w) = VARIATION_NEG (vval != 0);
>       return;
>     }
>
>   abs_usize = ABS (usize);
>
>   /* If not space for W (and possible carry), increase space.  */
>   wp = MPZ_REALLOC (w, abs_usize + 1);
>
>   /* These must be after realloc (U may be the same as W).  */
>   up = PTR (u);
>
>   if (usize VARIATION_CMP 0)
>     {
>       mp_limb_t cy;
>       cy = mpn_add_1 (wp, up, abs_usize, (mp_limb_t) vval);
>       wp[abs_usize] = cy;
>       wsize = VARIATION_NEG (abs_usize + cy);
>     }
>   else
>     {
>       /* The signs are different.  Need exact comparison to determine
>          which operand to subtract from which.  */
>       if (abs_usize == 1 && up[0] < vval)
>         {
>           wp[0] = vval - up[0];
>           wsize = VARIATION_NEG 1;
>         }
>       else
>         {
>           mpn_sub_1 (wp, up, abs_usize, (mp_limb_t) vval);
>           /* Size can decrease with at most one limb.  */
>           wsize = VARIATION_UNNEG (abs_usize - (wp[abs_usize - 1] == 0));
>         }
>     }
>
>   SIZ (w) = wsize;
> }
> --->*---
>
>
>
> On Thu, Jul 16, 2020 at 11:41:17AM +0300, Ard Biesheuvel wrote:
> > On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >
> > > On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> > > >
> > > > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > > > new file mode 100644
> > > > index 000000000000..fa6b085bac36
> > > > --- /dev/null
> > > > +++ b/lib/mpi/mpi-sub-ui.c
> > > > @@ -0,0 +1,60 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/* mpi-sub-ui.c  -  MPI functions
> > > > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > + *      Free Software Foundation, Inc.
> > > > + *
> > > > + * This file is part of GnuPG.
> > > > + *
> > > > + * Note: This code is heavily based on the GNU MP Library.
> > > > + *    Actually it's the same code with only minor changes in the
> > > > + *    way the data is stored; this is to support the abstraction
> > > > + *    of an optional secure memory allocation which may be used
> > > > + *    to avoid revealing of sensitive data due to paging etc.
> > > > + *    The GNU MP Library itself is published under the LGPL;
> > > > + *    however I decided to publish this code under the plain GPL.
> > > > + */
> > >
> > > Hmm, you said that this code is from GNU MP.  But this notice clearly
> > > says that it's part of GnuPG and is under GPL.  Though it doesn't
> > > clarify what version of GPL it is.  Can you please clarify this with
> > > the author?
> > >
> >
> > GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
> > copyright years and the explicit statements that the file is part of
> > GnuPG and not under the original LGPL license, there is no way we can
> > take this code under the kernel's GPLv2 license.
>
> --
> Regards,
> Marcelo
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16 13:09           ` Ard Biesheuvel
@ 2020-07-16 13:41             ` Marcelo Henrique Cerri
  2020-07-16 13:53               ` Ard Biesheuvel
  0 siblings, 1 reply; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-16 13:41 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Herbert Xu, Stephan Müller, Linux Crypto Mailing List,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 8513 bytes --]

On Thu, Jul 16, 2020 at 04:09:39PM +0300, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
> <marcelo.cerri@canonical.com> wrote:
> >
> > No. The code is really based on Gnu MP. I used the header from
> > lib/mpi/mpi-pow.c as reference and that's source of the mention to
> > GnuPG that went unnoticed by me.
> >
> 
> So where did the file lib/mpi/mpi-sub-ui.c come from? From GnuPG or
> from GnuMP? Did you modify the license statement? Because as proposed,
> this patch clearly is not acceptable from GPL compliance  point of
> view.

Sorry for the confusion. The code is from Gnu MP (not GnuPG).

Gnu MP is distributed under either LGPLv3 or later or GPLv2 or later
(check their license statement on the aors_ui.h file below).

For mpi-sub-ui.h I added a SPDX identifier for GPLv2 or later and I
kept the FSF copyright line.

I also used the header from mpi-powm.c as a reference basically to
inform the code was changed from its original form.

Here lies my mistake, I didn't notice that part was referring to GnuPG
instead of Gnu MP.

So mpi-sub-ui.h header was actually intended to be:

    // SPDX-License-Identifier: GPL-2.0-or-later
    /* mpi-sub-ui.c  -  MPI functions
     *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
     *      Free Software Foundation, Inc.
     *
     * This file is part of Gnu MP.
     *
     * Note: This code is heavily based on the GNU MP Library.
     *      Actually it's the same code with only minor changes in the
     *      way the data is stored; this is to support the abstraction
     *      of an optional secure memory allocation which may be used
     *      to avoid revealing of sensitive data due to paging etc.
     *      The GNU MP Library itself is published under the LGPL;
     *      however I decided to publish this code under the plain GPL.
     */

Or maybe instead of "This file is part of Gnu MP.", "This file is
based on Gnu MP" might be more appropriate.

Do you have any license concerns considering this updated header?


> 
> 
> 
> > You can find the original Gnu MP source that I used as reference in
> > the file gmp-6.2.0/mpz/aors_ui.h from:
> >
> > https://gmplib.org/download/gmp/gmp-6.2.0.tar.lz
> >
> > I'm pasting the contents of gmp-6.2.0/mpz/aors_ui.h below for
> > reference. Do you think we should use or adapt the original header
> > instead?
> >
> > That said, assuming the patch set submitted by Tianjia is updated to
> > ensure that mpi_sub_ui() and other functions are returning allocation
> > errors, we could drop this patch in favor of that patch set that is
> > more extensive and also provides an implementation to mpi_sub_ui().
> >
> >
> > --->8---
> > /* mpz_add_ui, mpz_sub_ui -- Add or subtract an mpz_t and an unsigned
> >    one-word integer.
> >
> > Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > Free Software Foundation, Inc.
> >


Gnu MP license -.
                V


> > This file is part of the GNU MP Library.
> >
> > The GNU MP Library is free software; you can redistribute it and/or modify
> > it under the terms of either:
> >
> >   * the GNU Lesser General Public License as published by the Free
> >     Software Foundation; either version 3 of the License, or (at your
> >     option) any later version.
> >
> > or
> >
> >   * the GNU General Public License as published by the Free Software
> >     Foundation; either version 2 of the License, or (at your option) any
> >     later version.
> >
> > or both in parallel, as here.
> >
> > The GNU MP Library is distributed in the hope that it will be useful, but
> > WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> > or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > for more details.
> >
> > You should have received copies of the GNU General Public License and the
> > GNU Lesser General Public License along with the GNU MP Library.  If not,
> > see https://www.gnu.org/licenses/.  */
> >
> > #include "gmp-impl.h"
> >
> >
> > #ifdef OPERATION_add_ui
> > #define FUNCTION          mpz_add_ui
> > #define FUNCTION2         mpz_add
> > #define VARIATION_CMP     >=
> > #define VARIATION_NEG
> > #define VARIATION_UNNEG   -
> > #endif
> >
> > #ifdef OPERATION_sub_ui
> > #define FUNCTION          mpz_sub_ui
> > #define FUNCTION2         mpz_sub
> > #define VARIATION_CMP     <
> > #define VARIATION_NEG     -
> > #define VARIATION_UNNEG
> > #endif
> >
> > #ifndef FUNCTION
> > Error, need OPERATION_add_ui or OPERATION_sub_ui
> > #endif
> >
> >
> > void
> > FUNCTION (mpz_ptr w, mpz_srcptr u, unsigned long int vval)
> > {
> >   mp_srcptr up;
> >   mp_ptr wp;
> >   mp_size_t usize, wsize;
> >   mp_size_t abs_usize;
> >
> > #if BITS_PER_ULONG > GMP_NUMB_BITS  /* avoid warnings about shift amount */
> >   if (vval > GMP_NUMB_MAX)
> >     {
> >       mpz_t v;
> >       mp_limb_t vl[2];
> >       PTR(v) = vl;
> >       vl[0] = vval & GMP_NUMB_MASK;
> >       vl[1] = vval >> GMP_NUMB_BITS;
> >       SIZ(v) = 2;
> >       FUNCTION2 (w, u, v);
> >       return;
> >     }
> > #endif
> >
> >   usize = SIZ (u);
> >   if (usize == 0)
> >     {
> >       MPZ_NEWALLOC (w, 1)[0] = vval;
> >       SIZ (w) = VARIATION_NEG (vval != 0);
> >       return;
> >     }
> >
> >   abs_usize = ABS (usize);
> >
> >   /* If not space for W (and possible carry), increase space.  */
> >   wp = MPZ_REALLOC (w, abs_usize + 1);
> >
> >   /* These must be after realloc (U may be the same as W).  */
> >   up = PTR (u);
> >
> >   if (usize VARIATION_CMP 0)
> >     {
> >       mp_limb_t cy;
> >       cy = mpn_add_1 (wp, up, abs_usize, (mp_limb_t) vval);
> >       wp[abs_usize] = cy;
> >       wsize = VARIATION_NEG (abs_usize + cy);
> >     }
> >   else
> >     {
> >       /* The signs are different.  Need exact comparison to determine
> >          which operand to subtract from which.  */
> >       if (abs_usize == 1 && up[0] < vval)
> >         {
> >           wp[0] = vval - up[0];
> >           wsize = VARIATION_NEG 1;
> >         }
> >       else
> >         {
> >           mpn_sub_1 (wp, up, abs_usize, (mp_limb_t) vval);
> >           /* Size can decrease with at most one limb.  */
> >           wsize = VARIATION_UNNEG (abs_usize - (wp[abs_usize - 1] == 0));
> >         }
> >     }
> >
> >   SIZ (w) = wsize;
> > }
> > --->*---
> >
> >
> >
> > On Thu, Jul 16, 2020 at 11:41:17AM +0300, Ard Biesheuvel wrote:
> > > On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > >
> > > > On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> > > > >
> > > > > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > > > > new file mode 100644
> > > > > index 000000000000..fa6b085bac36
> > > > > --- /dev/null
> > > > > +++ b/lib/mpi/mpi-sub-ui.c
> > > > > @@ -0,0 +1,60 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +/* mpi-sub-ui.c  -  MPI functions
> > > > > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > + *      Free Software Foundation, Inc.
> > > > > + *
> > > > > + * This file is part of GnuPG.
> > > > > + *
> > > > > + * Note: This code is heavily based on the GNU MP Library.
> > > > > + *    Actually it's the same code with only minor changes in the
> > > > > + *    way the data is stored; this is to support the abstraction
> > > > > + *    of an optional secure memory allocation which may be used
> > > > > + *    to avoid revealing of sensitive data due to paging etc.
> > > > > + *    The GNU MP Library itself is published under the LGPL;
> > > > > + *    however I decided to publish this code under the plain GPL.
> > > > > + */
> > > >
> > > > Hmm, you said that this code is from GNU MP.  But this notice clearly
> > > > says that it's part of GnuPG and is under GPL.  Though it doesn't
> > > > clarify what version of GPL it is.  Can you please clarify this with
> > > > the author?
> > > >
> > >
> > > GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
> > > copyright years and the explicit statements that the file is part of
> > > GnuPG and not under the original LGPL license, there is no way we can
> > > take this code under the kernel's GPLv2 license.
> >
> > --
> > Regards,
> > Marcelo
> >

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16 13:41             ` Marcelo Henrique Cerri
@ 2020-07-16 13:53               ` Ard Biesheuvel
  2020-07-16 14:23                 ` Marcelo Henrique Cerri
  0 siblings, 1 reply; 42+ messages in thread
From: Ard Biesheuvel @ 2020-07-16 13:53 UTC (permalink / raw)
  To: Marcelo Henrique Cerri
  Cc: Herbert Xu, Stephan Müller, Linux Crypto Mailing List,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

On Thu, 16 Jul 2020 at 16:41, Marcelo Henrique Cerri
<marcelo.cerri@canonical.com> wrote:
>
> On Thu, Jul 16, 2020 at 04:09:39PM +0300, Ard Biesheuvel wrote:
> > On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
> > <marcelo.cerri@canonical.com> wrote:
> > >
> > > No. The code is really based on Gnu MP. I used the header from
> > > lib/mpi/mpi-pow.c as reference and that's source of the mention to
> > > GnuPG that went unnoticed by me.
> > >
> >
> > So where did the file lib/mpi/mpi-sub-ui.c come from? From GnuPG or
> > from GnuMP? Did you modify the license statement? Because as proposed,
> > this patch clearly is not acceptable from GPL compliance  point of
> > view.
>
> Sorry for the confusion. The code is from Gnu MP (not GnuPG).
>
> Gnu MP is distributed under either LGPLv3 or later or GPLv2 or later
> (check their license statement on the aors_ui.h file below).
>
> For mpi-sub-ui.h I added a SPDX identifier for GPLv2 or later and I
> kept the FSF copyright line.
>
> I also used the header from mpi-powm.c as a reference basically to
> inform the code was changed from its original form.
>
> Here lies my mistake, I didn't notice that part was referring to GnuPG
> instead of Gnu MP.
>
> So mpi-sub-ui.h header was actually intended to be:
>
>     // SPDX-License-Identifier: GPL-2.0-or-later
>     /* mpi-sub-ui.c  -  MPI functions
>      *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
>      *      Free Software Foundation, Inc.
>      *
>      * This file is part of Gnu MP.
>      *
>      * Note: This code is heavily based on the GNU MP Library.
>      *      Actually it's the same code with only minor changes in the
>      *      way the data is stored; this is to support the abstraction
>      *      of an optional secure memory allocation which may be used
>      *      to avoid revealing of sensitive data due to paging etc.
>      *      The GNU MP Library itself is published under the LGPL;
>      *      however I decided to publish this code under the plain GPL.
>      */
>
> Or maybe instead of "This file is part of Gnu MP.", "This file is
> based on Gnu MP" might be more appropriate.
>
> Do you have any license concerns considering this updated header?
>

Yes. How can this code be both part of GnuMP *and* be heavily based on
it, but with changes?

Please avoid making changes to the original header, just add the SPDX
header in front, and add a clear justification in the commit log where
the file came from (preferably including git url and commit ID), and
what you based your assertion on that its license is compatible with
GPLv2.

Ideally, you would import the file *exactly* as it appears in the
upstream in one patch (with the above justification), and apply any
necessary changes in a subsequent patch, so it's  crystal clear that
we are complying with the original license.



>
> >
> >
> >
> > > You can find the original Gnu MP source that I used as reference in
> > > the file gmp-6.2.0/mpz/aors_ui.h from:
> > >
> > > https://gmplib.org/download/gmp/gmp-6.2.0.tar.lz
> > >
> > > I'm pasting the contents of gmp-6.2.0/mpz/aors_ui.h below for
> > > reference. Do you think we should use or adapt the original header
> > > instead?
> > >
> > > That said, assuming the patch set submitted by Tianjia is updated to
> > > ensure that mpi_sub_ui() and other functions are returning allocation
> > > errors, we could drop this patch in favor of that patch set that is
> > > more extensive and also provides an implementation to mpi_sub_ui().
> > >
> > >
> > > --->8---
> > > /* mpz_add_ui, mpz_sub_ui -- Add or subtract an mpz_t and an unsigned
> > >    one-word integer.
> > >
> > > Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > Free Software Foundation, Inc.
> > >
>
>
> Gnu MP license -.
>                 V
>
>
> > > This file is part of the GNU MP Library.
> > >
> > > The GNU MP Library is free software; you can redistribute it and/or modify
> > > it under the terms of either:
> > >
> > >   * the GNU Lesser General Public License as published by the Free
> > >     Software Foundation; either version 3 of the License, or (at your
> > >     option) any later version.
> > >
> > > or
> > >
> > >   * the GNU General Public License as published by the Free Software
> > >     Foundation; either version 2 of the License, or (at your option) any
> > >     later version.
> > >
> > > or both in parallel, as here.
> > >
> > > The GNU MP Library is distributed in the hope that it will be useful, but
> > > WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> > > or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > > for more details.
> > >
> > > You should have received copies of the GNU General Public License and the
> > > GNU Lesser General Public License along with the GNU MP Library.  If not,
> > > see https://www.gnu.org/licenses/.  */
> > >
> > > #include "gmp-impl.h"
> > >
> > >
> > > #ifdef OPERATION_add_ui
> > > #define FUNCTION          mpz_add_ui
> > > #define FUNCTION2         mpz_add
> > > #define VARIATION_CMP     >=
> > > #define VARIATION_NEG
> > > #define VARIATION_UNNEG   -
> > > #endif
> > >
> > > #ifdef OPERATION_sub_ui
> > > #define FUNCTION          mpz_sub_ui
> > > #define FUNCTION2         mpz_sub
> > > #define VARIATION_CMP     <
> > > #define VARIATION_NEG     -
> > > #define VARIATION_UNNEG
> > > #endif
> > >
> > > #ifndef FUNCTION
> > > Error, need OPERATION_add_ui or OPERATION_sub_ui
> > > #endif
> > >
> > >
> > > void
> > > FUNCTION (mpz_ptr w, mpz_srcptr u, unsigned long int vval)
> > > {
> > >   mp_srcptr up;
> > >   mp_ptr wp;
> > >   mp_size_t usize, wsize;
> > >   mp_size_t abs_usize;
> > >
> > > #if BITS_PER_ULONG > GMP_NUMB_BITS  /* avoid warnings about shift amount */
> > >   if (vval > GMP_NUMB_MAX)
> > >     {
> > >       mpz_t v;
> > >       mp_limb_t vl[2];
> > >       PTR(v) = vl;
> > >       vl[0] = vval & GMP_NUMB_MASK;
> > >       vl[1] = vval >> GMP_NUMB_BITS;
> > >       SIZ(v) = 2;
> > >       FUNCTION2 (w, u, v);
> > >       return;
> > >     }
> > > #endif
> > >
> > >   usize = SIZ (u);
> > >   if (usize == 0)
> > >     {
> > >       MPZ_NEWALLOC (w, 1)[0] = vval;
> > >       SIZ (w) = VARIATION_NEG (vval != 0);
> > >       return;
> > >     }
> > >
> > >   abs_usize = ABS (usize);
> > >
> > >   /* If not space for W (and possible carry), increase space.  */
> > >   wp = MPZ_REALLOC (w, abs_usize + 1);
> > >
> > >   /* These must be after realloc (U may be the same as W).  */
> > >   up = PTR (u);
> > >
> > >   if (usize VARIATION_CMP 0)
> > >     {
> > >       mp_limb_t cy;
> > >       cy = mpn_add_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > >       wp[abs_usize] = cy;
> > >       wsize = VARIATION_NEG (abs_usize + cy);
> > >     }
> > >   else
> > >     {
> > >       /* The signs are different.  Need exact comparison to determine
> > >          which operand to subtract from which.  */
> > >       if (abs_usize == 1 && up[0] < vval)
> > >         {
> > >           wp[0] = vval - up[0];
> > >           wsize = VARIATION_NEG 1;
> > >         }
> > >       else
> > >         {
> > >           mpn_sub_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > >           /* Size can decrease with at most one limb.  */
> > >           wsize = VARIATION_UNNEG (abs_usize - (wp[abs_usize - 1] == 0));
> > >         }
> > >     }
> > >
> > >   SIZ (w) = wsize;
> > > }
> > > --->*---
> > >
> > >
> > >
> > > On Thu, Jul 16, 2020 at 11:41:17AM +0300, Ard Biesheuvel wrote:
> > > > On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > > >
> > > > > On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> > > > > >
> > > > > > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..fa6b085bac36
> > > > > > --- /dev/null
> > > > > > +++ b/lib/mpi/mpi-sub-ui.c
> > > > > > @@ -0,0 +1,60 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > +/* mpi-sub-ui.c  -  MPI functions
> > > > > > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > > + *      Free Software Foundation, Inc.
> > > > > > + *
> > > > > > + * This file is part of GnuPG.
> > > > > > + *
> > > > > > + * Note: This code is heavily based on the GNU MP Library.
> > > > > > + *    Actually it's the same code with only minor changes in the
> > > > > > + *    way the data is stored; this is to support the abstraction
> > > > > > + *    of an optional secure memory allocation which may be used
> > > > > > + *    to avoid revealing of sensitive data due to paging etc.
> > > > > > + *    The GNU MP Library itself is published under the LGPL;
> > > > > > + *    however I decided to publish this code under the plain GPL.
> > > > > > + */
> > > > >
> > > > > Hmm, you said that this code is from GNU MP.  But this notice clearly
> > > > > says that it's part of GnuPG and is under GPL.  Though it doesn't
> > > > > clarify what version of GPL it is.  Can you please clarify this with
> > > > > the author?
> > > > >
> > > >
> > > > GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
> > > > copyright years and the explicit statements that the file is part of
> > > > GnuPG and not under the original LGPL license, there is no way we can
> > > > take this code under the kernel's GPLv2 license.
> > >
> > > --
> > > Regards,
> > > Marcelo
> > >
>
> --
> Regards,
> Marcelo
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16 13:53               ` Ard Biesheuvel
@ 2020-07-16 14:23                 ` Marcelo Henrique Cerri
  2020-07-16 14:37                   ` Ard Biesheuvel
  0 siblings, 1 reply; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-16 14:23 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Herbert Xu, Stephan Müller, Linux Crypto Mailing List,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 12414 bytes --]

On Thu, Jul 16, 2020 at 04:53:24PM +0300, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 16:41, Marcelo Henrique Cerri
> <marcelo.cerri@canonical.com> wrote:
> >
> > On Thu, Jul 16, 2020 at 04:09:39PM +0300, Ard Biesheuvel wrote:
> > > On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
> > > <marcelo.cerri@canonical.com> wrote:
> > > >
> > > > No. The code is really based on Gnu MP. I used the header from
> > > > lib/mpi/mpi-pow.c as reference and that's source of the mention to
> > > > GnuPG that went unnoticed by me.
> > > >
> > >
> > > So where did the file lib/mpi/mpi-sub-ui.c come from? From GnuPG or
> > > from GnuMP? Did you modify the license statement? Because as proposed,
> > > this patch clearly is not acceptable from GPL compliance  point of
> > > view.
> >
> > Sorry for the confusion. The code is from Gnu MP (not GnuPG).
> >
> > Gnu MP is distributed under either LGPLv3 or later or GPLv2 or later
> > (check their license statement on the aors_ui.h file below).
> >
> > For mpi-sub-ui.h I added a SPDX identifier for GPLv2 or later and I
> > kept the FSF copyright line.
> >
> > I also used the header from mpi-powm.c as a reference basically to
> > inform the code was changed from its original form.
> >
> > Here lies my mistake, I didn't notice that part was referring to GnuPG
> > instead of Gnu MP.
> >
> > So mpi-sub-ui.h header was actually intended to be:
> >
> >     // SPDX-License-Identifier: GPL-2.0-or-later
> >     /* mpi-sub-ui.c  -  MPI functions
> >      *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> >      *      Free Software Foundation, Inc.
> >      *
> >      * This file is part of Gnu MP.
> >      *
> >      * Note: This code is heavily based on the GNU MP Library.
> >      *      Actually it's the same code with only minor changes in the
> >      *      way the data is stored; this is to support the abstraction
> >      *      of an optional secure memory allocation which may be used
> >      *      to avoid revealing of sensitive data due to paging etc.
> >      *      The GNU MP Library itself is published under the LGPL;
> >      *      however I decided to publish this code under the plain GPL.
> >      */
> >
> > Or maybe instead of "This file is part of Gnu MP.", "This file is
> > based on Gnu MP" might be more appropriate.
> >
> > Do you have any license concerns considering this updated header?
> >
> 
> Yes. How can this code be both part of GnuMP *and* be heavily based on
> it, but with changes?
>
> Please avoid making changes to the original header, just add the SPDX
> header in front, and add a clear justification in the commit log where
> the file came from (preferably including git url and commit ID), and
> what you based your assertion on that its license is compatible with
> GPLv2.

The commit message is stating the origin, but I can add a reference to
the mercurial repo with its corresponding id.

> 
> Ideally, you would import the file *exactly* as it appears in the
> upstream in one patch (with the above justification), and apply any
> necessary changes in a subsequent patch, so it's  crystal clear that
> we are complying with the original license.

I'm not sure that's the ideal approach for this case. The logic is the
same but since naming convention, macros, data types and etc are
pretty different everything was basically re-written to fit the
kernel. Adding the original file and then massively changing will just
add unnecessary noise.

If you agree I will update the commit message with more details about
the original source and then just update the comment header in
mpi-sub-ui.c following closely the original header with minor
adjustments to explain its origin and to fix some checkpatch warnings.

Something like that:

// SPDX-License-Identifier: GPL-2.0-or-later
/* mpi-sub-ui.c - Subtract an unsigned integer from an MPI.
 *
 * Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
 * Free Software Foundation, Inc.
 *
 * This file was based on the GNU MP Library source file:
 * https://gmplib.org/repo/gmp-6.2/file/tip/mpz/aors_ui.h
 *
 * The GNU MP Library is free software; you can redistribute it and/or modify
 * it under the terms of either:
 *
 *   * the GNU Lesser General Public License as published by the Free
 *     Software Foundation; either version 3 of the License, or (at your
 *     option) any later version.
 *
 * or
 *
 *   * the GNU General Public License as published by the Free Software
 *     Foundation; either version 2 of the License, or (at your option) any
 *     later version.
 *
 * or both in parallel, as here.
 *
 * The GNU MP Library is distributed in the hope that it will be useful, but
 * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
 * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * for more details.
 *
 * You should have received copies of the GNU General Public License and the
 * GNU Lesser General Public License along with the GNU MP Library.  If not,
 * see https://www.gnu.org/licenses/.
 */

> 
> 
> 
> >
> > >
> > >
> > >
> > > > You can find the original Gnu MP source that I used as reference in
> > > > the file gmp-6.2.0/mpz/aors_ui.h from:
> > > >
> > > > https://gmplib.org/download/gmp/gmp-6.2.0.tar.lz
> > > >
> > > > I'm pasting the contents of gmp-6.2.0/mpz/aors_ui.h below for
> > > > reference. Do you think we should use or adapt the original header
> > > > instead?
> > > >
> > > > That said, assuming the patch set submitted by Tianjia is updated to
> > > > ensure that mpi_sub_ui() and other functions are returning allocation
> > > > errors, we could drop this patch in favor of that patch set that is
> > > > more extensive and also provides an implementation to mpi_sub_ui().
> > > >
> > > >
> > > > --->8---
> > > > /* mpz_add_ui, mpz_sub_ui -- Add or subtract an mpz_t and an unsigned
> > > >    one-word integer.
> > > >
> > > > Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > Free Software Foundation, Inc.
> > > >
> >
> >
> > Gnu MP license -.
> >                 V
> >
> >
> > > > This file is part of the GNU MP Library.
> > > >
> > > > The GNU MP Library is free software; you can redistribute it and/or modify
> > > > it under the terms of either:
> > > >
> > > >   * the GNU Lesser General Public License as published by the Free
> > > >     Software Foundation; either version 3 of the License, or (at your
> > > >     option) any later version.
> > > >
> > > > or
> > > >
> > > >   * the GNU General Public License as published by the Free Software
> > > >     Foundation; either version 2 of the License, or (at your option) any
> > > >     later version.
> > > >
> > > > or both in parallel, as here.
> > > >
> > > > The GNU MP Library is distributed in the hope that it will be useful, but
> > > > WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> > > > or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > > > for more details.
> > > >
> > > > You should have received copies of the GNU General Public License and the
> > > > GNU Lesser General Public License along with the GNU MP Library.  If not,
> > > > see https://www.gnu.org/licenses/.  */
> > > >
> > > > #include "gmp-impl.h"
> > > >
> > > >
> > > > #ifdef OPERATION_add_ui
> > > > #define FUNCTION          mpz_add_ui
> > > > #define FUNCTION2         mpz_add
> > > > #define VARIATION_CMP     >=
> > > > #define VARIATION_NEG
> > > > #define VARIATION_UNNEG   -
> > > > #endif
> > > >
> > > > #ifdef OPERATION_sub_ui
> > > > #define FUNCTION          mpz_sub_ui
> > > > #define FUNCTION2         mpz_sub
> > > > #define VARIATION_CMP     <
> > > > #define VARIATION_NEG     -
> > > > #define VARIATION_UNNEG
> > > > #endif
> > > >
> > > > #ifndef FUNCTION
> > > > Error, need OPERATION_add_ui or OPERATION_sub_ui
> > > > #endif
> > > >
> > > >
> > > > void
> > > > FUNCTION (mpz_ptr w, mpz_srcptr u, unsigned long int vval)
> > > > {
> > > >   mp_srcptr up;
> > > >   mp_ptr wp;
> > > >   mp_size_t usize, wsize;
> > > >   mp_size_t abs_usize;
> > > >
> > > > #if BITS_PER_ULONG > GMP_NUMB_BITS  /* avoid warnings about shift amount */
> > > >   if (vval > GMP_NUMB_MAX)
> > > >     {
> > > >       mpz_t v;
> > > >       mp_limb_t vl[2];
> > > >       PTR(v) = vl;
> > > >       vl[0] = vval & GMP_NUMB_MASK;
> > > >       vl[1] = vval >> GMP_NUMB_BITS;
> > > >       SIZ(v) = 2;
> > > >       FUNCTION2 (w, u, v);
> > > >       return;
> > > >     }
> > > > #endif
> > > >
> > > >   usize = SIZ (u);
> > > >   if (usize == 0)
> > > >     {
> > > >       MPZ_NEWALLOC (w, 1)[0] = vval;
> > > >       SIZ (w) = VARIATION_NEG (vval != 0);
> > > >       return;
> > > >     }
> > > >
> > > >   abs_usize = ABS (usize);
> > > >
> > > >   /* If not space for W (and possible carry), increase space.  */
> > > >   wp = MPZ_REALLOC (w, abs_usize + 1);
> > > >
> > > >   /* These must be after realloc (U may be the same as W).  */
> > > >   up = PTR (u);
> > > >
> > > >   if (usize VARIATION_CMP 0)
> > > >     {
> > > >       mp_limb_t cy;
> > > >       cy = mpn_add_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > > >       wp[abs_usize] = cy;
> > > >       wsize = VARIATION_NEG (abs_usize + cy);
> > > >     }
> > > >   else
> > > >     {
> > > >       /* The signs are different.  Need exact comparison to determine
> > > >          which operand to subtract from which.  */
> > > >       if (abs_usize == 1 && up[0] < vval)
> > > >         {
> > > >           wp[0] = vval - up[0];
> > > >           wsize = VARIATION_NEG 1;
> > > >         }
> > > >       else
> > > >         {
> > > >           mpn_sub_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > > >           /* Size can decrease with at most one limb.  */
> > > >           wsize = VARIATION_UNNEG (abs_usize - (wp[abs_usize - 1] == 0));
> > > >         }
> > > >     }
> > > >
> > > >   SIZ (w) = wsize;
> > > > }
> > > > --->*---
> > > >
> > > >
> > > >
> > > > On Thu, Jul 16, 2020 at 11:41:17AM +0300, Ard Biesheuvel wrote:
> > > > > On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > > > >
> > > > > > On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> > > > > > >
> > > > > > > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..fa6b085bac36
> > > > > > > --- /dev/null
> > > > > > > +++ b/lib/mpi/mpi-sub-ui.c
> > > > > > > @@ -0,0 +1,60 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > +/* mpi-sub-ui.c  -  MPI functions
> > > > > > > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > > > + *      Free Software Foundation, Inc.
> > > > > > > + *
> > > > > > > + * This file is part of GnuPG.
> > > > > > > + *
> > > > > > > + * Note: This code is heavily based on the GNU MP Library.
> > > > > > > + *    Actually it's the same code with only minor changes in the
> > > > > > > + *    way the data is stored; this is to support the abstraction
> > > > > > > + *    of an optional secure memory allocation which may be used
> > > > > > > + *    to avoid revealing of sensitive data due to paging etc.
> > > > > > > + *    The GNU MP Library itself is published under the LGPL;
> > > > > > > + *    however I decided to publish this code under the plain GPL.
> > > > > > > + */
> > > > > >
> > > > > > Hmm, you said that this code is from GNU MP.  But this notice clearly
> > > > > > says that it's part of GnuPG and is under GPL.  Though it doesn't
> > > > > > clarify what version of GPL it is.  Can you please clarify this with
> > > > > > the author?
> > > > > >
> > > > >
> > > > > GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
> > > > > copyright years and the explicit statements that the file is part of
> > > > > GnuPG and not under the original LGPL license, there is no way we can
> > > > > take this code under the kernel's GPLv2 license.
> > > >
> > > > --
> > > > Regards,
> > > > Marcelo
> > > >
> >
> > --
> > Regards,
> > Marcelo
> >

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16 14:23                 ` Marcelo Henrique Cerri
@ 2020-07-16 14:37                   ` Ard Biesheuvel
  2020-07-16 14:56                     ` Marcelo Henrique Cerri
  0 siblings, 1 reply; 42+ messages in thread
From: Ard Biesheuvel @ 2020-07-16 14:37 UTC (permalink / raw)
  To: Marcelo Henrique Cerri
  Cc: Herbert Xu, Stephan Müller, Linux Crypto Mailing List,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

On Thu, 16 Jul 2020 at 17:23, Marcelo Henrique Cerri
<marcelo.cerri@canonical.com> wrote:
>
> On Thu, Jul 16, 2020 at 04:53:24PM +0300, Ard Biesheuvel wrote:
> > On Thu, 16 Jul 2020 at 16:41, Marcelo Henrique Cerri
> > <marcelo.cerri@canonical.com> wrote:
> > >
> > > On Thu, Jul 16, 2020 at 04:09:39PM +0300, Ard Biesheuvel wrote:
> > > > On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
> > > > <marcelo.cerri@canonical.com> wrote:
> > > > >
> > > > > No. The code is really based on Gnu MP. I used the header from
> > > > > lib/mpi/mpi-pow.c as reference and that's source of the mention to
> > > > > GnuPG that went unnoticed by me.
> > > > >
> > > >
> > > > So where did the file lib/mpi/mpi-sub-ui.c come from? From GnuPG or
> > > > from GnuMP? Did you modify the license statement? Because as proposed,
> > > > this patch clearly is not acceptable from GPL compliance  point of
> > > > view.
> > >
> > > Sorry for the confusion. The code is from Gnu MP (not GnuPG).
> > >
> > > Gnu MP is distributed under either LGPLv3 or later or GPLv2 or later
> > > (check their license statement on the aors_ui.h file below).
> > >
> > > For mpi-sub-ui.h I added a SPDX identifier for GPLv2 or later and I
> > > kept the FSF copyright line.
> > >
> > > I also used the header from mpi-powm.c as a reference basically to
> > > inform the code was changed from its original form.
> > >
> > > Here lies my mistake, I didn't notice that part was referring to GnuPG
> > > instead of Gnu MP.
> > >
> > > So mpi-sub-ui.h header was actually intended to be:
> > >
> > >     // SPDX-License-Identifier: GPL-2.0-or-later
> > >     /* mpi-sub-ui.c  -  MPI functions
> > >      *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > >      *      Free Software Foundation, Inc.
> > >      *
> > >      * This file is part of Gnu MP.
> > >      *
> > >      * Note: This code is heavily based on the GNU MP Library.
> > >      *      Actually it's the same code with only minor changes in the
> > >      *      way the data is stored; this is to support the abstraction
> > >      *      of an optional secure memory allocation which may be used
> > >      *      to avoid revealing of sensitive data due to paging etc.
> > >      *      The GNU MP Library itself is published under the LGPL;
> > >      *      however I decided to publish this code under the plain GPL.
> > >      */
> > >
> > > Or maybe instead of "This file is part of Gnu MP.", "This file is
> > > based on Gnu MP" might be more appropriate.
> > >
> > > Do you have any license concerns considering this updated header?
> > >
> >
> > Yes. How can this code be both part of GnuMP *and* be heavily based on
> > it, but with changes?
> >

You haven't answered this question yet. I suppose you just slapped a
different license text on this file, one that already existed in
lib/mpi?

> > Please avoid making changes to the original header, just add the SPDX
> > header in front, and add a clear justification in the commit log where
> > the file came from (preferably including git url and commit ID), and
> > what you based your assertion on that its license is compatible with
> > GPLv2.
>
> The commit message is stating the origin, but I can add a reference to
> the mercurial repo with its corresponding id.
>

Yes, please.

> >
> > Ideally, you would import the file *exactly* as it appears in the
> > upstream in one patch (with the above justification), and apply any
> > necessary changes in a subsequent patch, so it's  crystal clear that
> > we are complying with the original license.
>
> I'm not sure that's the ideal approach for this case. The logic is the
> same but since naming convention, macros, data types and etc are
> pretty different everything was basically re-written to fit the
> kernel. Adding the original file and then massively changing will just
> add unnecessary noise.
>

Do any of these modifications resemble the changes made to the GnuPG
versions of these routines?

> If you agree I will update the commit message with more details about
> the original source and then just update the comment header in
> mpi-sub-ui.c following closely the original header with minor
> adjustments to explain its origin and to fix some checkpatch warnings.
>

That is fine, provided that none of our modifications were taken from
the GnuPG version of this file without giving credit.


> Something like that:
>
> // SPDX-License-Identifier: GPL-2.0-or-later
> /* mpi-sub-ui.c - Subtract an unsigned integer from an MPI.
>  *
>  * Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
>  * Free Software Foundation, Inc.
>  *
>  * This file was based on the GNU MP Library source file:
>  * https://gmplib.org/repo/gmp-6.2/file/tip/mpz/aors_ui.h
>  *
>  * The GNU MP Library is free software; you can redistribute it and/or modify
>  * it under the terms of either:
>  *
>  *   * the GNU Lesser General Public License as published by the Free
>  *     Software Foundation; either version 3 of the License, or (at your
>  *     option) any later version.
>  *
>  * or
>  *
>  *   * the GNU General Public License as published by the Free Software
>  *     Foundation; either version 2 of the License, or (at your option) any
>  *     later version.
>  *
>  * or both in parallel, as here.
>  *
>  * The GNU MP Library is distributed in the hope that it will be useful, but
>  * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>  * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>  * for more details.
>  *
>  * You should have received copies of the GNU General Public License and the
>  * GNU Lesser General Public License along with the GNU MP Library.  If not,
>  * see https://www.gnu.org/licenses/.
>  */
>
> >
> >
> >
> > >
> > > >
> > > >
> > > >
> > > > > You can find the original Gnu MP source that I used as reference in
> > > > > the file gmp-6.2.0/mpz/aors_ui.h from:
> > > > >
> > > > > https://gmplib.org/download/gmp/gmp-6.2.0.tar.lz
> > > > >
> > > > > I'm pasting the contents of gmp-6.2.0/mpz/aors_ui.h below for
> > > > > reference. Do you think we should use or adapt the original header
> > > > > instead?
> > > > >
> > > > > That said, assuming the patch set submitted by Tianjia is updated to
> > > > > ensure that mpi_sub_ui() and other functions are returning allocation
> > > > > errors, we could drop this patch in favor of that patch set that is
> > > > > more extensive and also provides an implementation to mpi_sub_ui().
> > > > >
> > > > >
> > > > > --->8---
> > > > > /* mpz_add_ui, mpz_sub_ui -- Add or subtract an mpz_t and an unsigned
> > > > >    one-word integer.
> > > > >
> > > > > Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > Free Software Foundation, Inc.
> > > > >
> > >
> > >
> > > Gnu MP license -.
> > >                 V
> > >
> > >
> > > > > This file is part of the GNU MP Library.
> > > > >
> > > > > The GNU MP Library is free software; you can redistribute it and/or modify
> > > > > it under the terms of either:
> > > > >
> > > > >   * the GNU Lesser General Public License as published by the Free
> > > > >     Software Foundation; either version 3 of the License, or (at your
> > > > >     option) any later version.
> > > > >
> > > > > or
> > > > >
> > > > >   * the GNU General Public License as published by the Free Software
> > > > >     Foundation; either version 2 of the License, or (at your option) any
> > > > >     later version.
> > > > >
> > > > > or both in parallel, as here.
> > > > >
> > > > > The GNU MP Library is distributed in the hope that it will be useful, but
> > > > > WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> > > > > or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > > > > for more details.
> > > > >
> > > > > You should have received copies of the GNU General Public License and the
> > > > > GNU Lesser General Public License along with the GNU MP Library.  If not,
> > > > > see https://www.gnu.org/licenses/.  */
> > > > >
> > > > > #include "gmp-impl.h"
> > > > >
> > > > >
> > > > > #ifdef OPERATION_add_ui
> > > > > #define FUNCTION          mpz_add_ui
> > > > > #define FUNCTION2         mpz_add
> > > > > #define VARIATION_CMP     >=
> > > > > #define VARIATION_NEG
> > > > > #define VARIATION_UNNEG   -
> > > > > #endif
> > > > >
> > > > > #ifdef OPERATION_sub_ui
> > > > > #define FUNCTION          mpz_sub_ui
> > > > > #define FUNCTION2         mpz_sub
> > > > > #define VARIATION_CMP     <
> > > > > #define VARIATION_NEG     -
> > > > > #define VARIATION_UNNEG
> > > > > #endif
> > > > >
> > > > > #ifndef FUNCTION
> > > > > Error, need OPERATION_add_ui or OPERATION_sub_ui
> > > > > #endif
> > > > >
> > > > >
> > > > > void
> > > > > FUNCTION (mpz_ptr w, mpz_srcptr u, unsigned long int vval)
> > > > > {
> > > > >   mp_srcptr up;
> > > > >   mp_ptr wp;
> > > > >   mp_size_t usize, wsize;
> > > > >   mp_size_t abs_usize;
> > > > >
> > > > > #if BITS_PER_ULONG > GMP_NUMB_BITS  /* avoid warnings about shift amount */
> > > > >   if (vval > GMP_NUMB_MAX)
> > > > >     {
> > > > >       mpz_t v;
> > > > >       mp_limb_t vl[2];
> > > > >       PTR(v) = vl;
> > > > >       vl[0] = vval & GMP_NUMB_MASK;
> > > > >       vl[1] = vval >> GMP_NUMB_BITS;
> > > > >       SIZ(v) = 2;
> > > > >       FUNCTION2 (w, u, v);
> > > > >       return;
> > > > >     }
> > > > > #endif
> > > > >
> > > > >   usize = SIZ (u);
> > > > >   if (usize == 0)
> > > > >     {
> > > > >       MPZ_NEWALLOC (w, 1)[0] = vval;
> > > > >       SIZ (w) = VARIATION_NEG (vval != 0);
> > > > >       return;
> > > > >     }
> > > > >
> > > > >   abs_usize = ABS (usize);
> > > > >
> > > > >   /* If not space for W (and possible carry), increase space.  */
> > > > >   wp = MPZ_REALLOC (w, abs_usize + 1);
> > > > >
> > > > >   /* These must be after realloc (U may be the same as W).  */
> > > > >   up = PTR (u);
> > > > >
> > > > >   if (usize VARIATION_CMP 0)
> > > > >     {
> > > > >       mp_limb_t cy;
> > > > >       cy = mpn_add_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > > > >       wp[abs_usize] = cy;
> > > > >       wsize = VARIATION_NEG (abs_usize + cy);
> > > > >     }
> > > > >   else
> > > > >     {
> > > > >       /* The signs are different.  Need exact comparison to determine
> > > > >          which operand to subtract from which.  */
> > > > >       if (abs_usize == 1 && up[0] < vval)
> > > > >         {
> > > > >           wp[0] = vval - up[0];
> > > > >           wsize = VARIATION_NEG 1;
> > > > >         }
> > > > >       else
> > > > >         {
> > > > >           mpn_sub_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > > > >           /* Size can decrease with at most one limb.  */
> > > > >           wsize = VARIATION_UNNEG (abs_usize - (wp[abs_usize - 1] == 0));
> > > > >         }
> > > > >     }
> > > > >
> > > > >   SIZ (w) = wsize;
> > > > > }
> > > > > --->*---
> > > > >
> > > > >
> > > > >
> > > > > On Thu, Jul 16, 2020 at 11:41:17AM +0300, Ard Biesheuvel wrote:
> > > > > > On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > > > > >
> > > > > > > On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> > > > > > > >
> > > > > > > > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..fa6b085bac36
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/lib/mpi/mpi-sub-ui.c
> > > > > > > > @@ -0,0 +1,60 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > > +/* mpi-sub-ui.c  -  MPI functions
> > > > > > > > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > > > > + *      Free Software Foundation, Inc.
> > > > > > > > + *
> > > > > > > > + * This file is part of GnuPG.
> > > > > > > > + *
> > > > > > > > + * Note: This code is heavily based on the GNU MP Library.
> > > > > > > > + *    Actually it's the same code with only minor changes in the
> > > > > > > > + *    way the data is stored; this is to support the abstraction
> > > > > > > > + *    of an optional secure memory allocation which may be used
> > > > > > > > + *    to avoid revealing of sensitive data due to paging etc.
> > > > > > > > + *    The GNU MP Library itself is published under the LGPL;
> > > > > > > > + *    however I decided to publish this code under the plain GPL.
> > > > > > > > + */
> > > > > > >
> > > > > > > Hmm, you said that this code is from GNU MP.  But this notice clearly
> > > > > > > says that it's part of GnuPG and is under GPL.  Though it doesn't
> > > > > > > clarify what version of GPL it is.  Can you please clarify this with
> > > > > > > the author?
> > > > > > >
> > > > > >
> > > > > > GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
> > > > > > copyright years and the explicit statements that the file is part of
> > > > > > GnuPG and not under the original LGPL license, there is no way we can
> > > > > > take this code under the kernel's GPLv2 license.
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Marcelo
> > > > >
> > >
> > > --
> > > Regards,
> > > Marcelo
> > >
>
> --
> Regards,
> Marcelo
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16 14:37                   ` Ard Biesheuvel
@ 2020-07-16 14:56                     ` Marcelo Henrique Cerri
  2020-07-16 15:20                       ` Ard Biesheuvel
  0 siblings, 1 reply; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-16 14:56 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Herbert Xu, Stephan Müller, Linux Crypto Mailing List,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 15151 bytes --]

On Thu, Jul 16, 2020 at 05:37:32PM +0300, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 17:23, Marcelo Henrique Cerri
> <marcelo.cerri@canonical.com> wrote:
> >
> > On Thu, Jul 16, 2020 at 04:53:24PM +0300, Ard Biesheuvel wrote:
> > > On Thu, 16 Jul 2020 at 16:41, Marcelo Henrique Cerri
> > > <marcelo.cerri@canonical.com> wrote:
> > > >
> > > > On Thu, Jul 16, 2020 at 04:09:39PM +0300, Ard Biesheuvel wrote:
> > > > > On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
> > > > > <marcelo.cerri@canonical.com> wrote:
> > > > > >
> > > > > > No. The code is really based on Gnu MP. I used the header from
> > > > > > lib/mpi/mpi-pow.c as reference and that's source of the mention to
> > > > > > GnuPG that went unnoticed by me.
> > > > > >
> > > > >
> > > > > So where did the file lib/mpi/mpi-sub-ui.c come from? From GnuPG or
> > > > > from GnuMP? Did you modify the license statement? Because as proposed,
> > > > > this patch clearly is not acceptable from GPL compliance  point of
> > > > > view.
> > > >
> > > > Sorry for the confusion. The code is from Gnu MP (not GnuPG).
> > > >
> > > > Gnu MP is distributed under either LGPLv3 or later or GPLv2 or later
> > > > (check their license statement on the aors_ui.h file below).
> > > >
> > > > For mpi-sub-ui.h I added a SPDX identifier for GPLv2 or later and I
> > > > kept the FSF copyright line.
> > > >
> > > > I also used the header from mpi-powm.c as a reference basically to
> > > > inform the code was changed from its original form.
> > > >
> > > > Here lies my mistake, I didn't notice that part was referring to GnuPG
> > > > instead of Gnu MP.
> > > >
> > > > So mpi-sub-ui.h header was actually intended to be:
> > > >
> > > >     // SPDX-License-Identifier: GPL-2.0-or-later
> > > >     /* mpi-sub-ui.c  -  MPI functions
> > > >      *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > >      *      Free Software Foundation, Inc.
> > > >      *
> > > >      * This file is part of Gnu MP.
> > > >      *
> > > >      * Note: This code is heavily based on the GNU MP Library.
> > > >      *      Actually it's the same code with only minor changes in the
> > > >      *      way the data is stored; this is to support the abstraction
> > > >      *      of an optional secure memory allocation which may be used
> > > >      *      to avoid revealing of sensitive data due to paging etc.
> > > >      *      The GNU MP Library itself is published under the LGPL;
> > > >      *      however I decided to publish this code under the plain GPL.
> > > >      */
> > > >
> > > > Or maybe instead of "This file is part of Gnu MP.", "This file is
> > > > based on Gnu MP" might be more appropriate.
> > > >
> > > > Do you have any license concerns considering this updated header?
> > > >
> > >
> > > Yes. How can this code be both part of GnuMP *and* be heavily based on
> > > it, but with changes?
> > >

The final mpi-sub-ui.c is not part of Gnu MP, but heavily based on it
sounds very accurate to me.

>
> You haven't answered this question yet. I suppose you just slapped a
> different license text on this file, one that already existed in
> lib/mpi?

Correct. The copyright line I used matches the original copyright from
Gnu MP.

The "This file is part..." line and the Note is the same from
mpi/lib/mpi-powm.c.

However I'm proposing the removal of that part in favor of the new
header I listed below based on Gnu MP header.

>
> > > Please avoid making changes to the original header, just add the SPDX
> > > header in front, and add a clear justification in the commit log where
> > > the file came from (preferably including git url and commit ID), and
> > > what you based your assertion on that its license is compatible with
> > > GPLv2.
> >
> > The commit message is stating the origin, but I can add a reference to
> > the mercurial repo with its corresponding id.
> >
> 
> Yes, please.
> 
> > >
> > > Ideally, you would import the file *exactly* as it appears in the
> > > upstream in one patch (with the above justification), and apply any
> > > necessary changes in a subsequent patch, so it's  crystal clear that
> > > we are complying with the original license.
> >
> > I'm not sure that's the ideal approach for this case. The logic is the
> > same but since naming convention, macros, data types and etc are
> > pretty different everything was basically re-written to fit the
> > kernel. Adding the original file and then massively changing will just
> > add unnecessary noise.
> >
> 
> Do any of these modifications resemble the changes made to the GnuPG
> versions of these routines?

I haven't looked at the GnuPG code at all. Neither while I was
preparing this patch, neither after or before. So I can affirm this
patch has no influence of GnuPG code at all.

But if you want to be sure I can check that. Please let me know.

As I said, the GnuPG reference came from me trying to re-use the
header from kernel's lib/mpi/mpi-powm.c file.

> 
> > If you agree I will update the commit message with more details about
> > the original source and then just update the comment header in
> > mpi-sub-ui.c following closely the original header with minor
> > adjustments to explain its origin and to fix some checkpatch warnings.
> >
> 
> That is fine, provided that none of our modifications were taken from
> the GnuPG version of this file without giving credit.

I can assure I haven't used any code from GnuPG at all.

> 
> 
> > Something like that:
> >
> > // SPDX-License-Identifier: GPL-2.0-or-later
> > /* mpi-sub-ui.c - Subtract an unsigned integer from an MPI.
> >  *
> >  * Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> >  * Free Software Foundation, Inc.
> >  *
> >  * This file was based on the GNU MP Library source file:
> >  * https://gmplib.org/repo/gmp-6.2/file/tip/mpz/aors_ui.h
> >  *
> >  * The GNU MP Library is free software; you can redistribute it and/or modify
> >  * it under the terms of either:
> >  *
> >  *   * the GNU Lesser General Public License as published by the Free
> >  *     Software Foundation; either version 3 of the License, or (at your
> >  *     option) any later version.
> >  *
> >  * or
> >  *
> >  *   * the GNU General Public License as published by the Free Software
> >  *     Foundation; either version 2 of the License, or (at your option) any
> >  *     later version.
> >  *
> >  * or both in parallel, as here.
> >  *
> >  * The GNU MP Library is distributed in the hope that it will be useful, but
> >  * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> >  * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> >  * for more details.
> >  *
> >  * You should have received copies of the GNU General Public License and the
> >  * GNU Lesser General Public License along with the GNU MP Library.  If not,
> >  * see https://www.gnu.org/licenses/.
> >  */
> >
> > >
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > > > You can find the original Gnu MP source that I used as reference in
> > > > > > the file gmp-6.2.0/mpz/aors_ui.h from:
> > > > > >
> > > > > > https://gmplib.org/download/gmp/gmp-6.2.0.tar.lz
> > > > > >
> > > > > > I'm pasting the contents of gmp-6.2.0/mpz/aors_ui.h below for
> > > > > > reference. Do you think we should use or adapt the original header
> > > > > > instead?
> > > > > >
> > > > > > That said, assuming the patch set submitted by Tianjia is updated to
> > > > > > ensure that mpi_sub_ui() and other functions are returning allocation
> > > > > > errors, we could drop this patch in favor of that patch set that is
> > > > > > more extensive and also provides an implementation to mpi_sub_ui().
> > > > > >
> > > > > >
> > > > > > --->8---
> > > > > > /* mpz_add_ui, mpz_sub_ui -- Add or subtract an mpz_t and an unsigned
> > > > > >    one-word integer.
> > > > > >
> > > > > > Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > > Free Software Foundation, Inc.
> > > > > >
> > > >
> > > >
> > > > Gnu MP license -.
> > > >                 V
> > > >
> > > >
> > > > > > This file is part of the GNU MP Library.
> > > > > >
> > > > > > The GNU MP Library is free software; you can redistribute it and/or modify
> > > > > > it under the terms of either:
> > > > > >
> > > > > >   * the GNU Lesser General Public License as published by the Free
> > > > > >     Software Foundation; either version 3 of the License, or (at your
> > > > > >     option) any later version.
> > > > > >
> > > > > > or
> > > > > >
> > > > > >   * the GNU General Public License as published by the Free Software
> > > > > >     Foundation; either version 2 of the License, or (at your option) any
> > > > > >     later version.
> > > > > >
> > > > > > or both in parallel, as here.
> > > > > >
> > > > > > The GNU MP Library is distributed in the hope that it will be useful, but
> > > > > > WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> > > > > > or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > > > > > for more details.
> > > > > >
> > > > > > You should have received copies of the GNU General Public License and the
> > > > > > GNU Lesser General Public License along with the GNU MP Library.  If not,
> > > > > > see https://www.gnu.org/licenses/.  */
> > > > > >
> > > > > > #include "gmp-impl.h"
> > > > > >
> > > > > >
> > > > > > #ifdef OPERATION_add_ui
> > > > > > #define FUNCTION          mpz_add_ui
> > > > > > #define FUNCTION2         mpz_add
> > > > > > #define VARIATION_CMP     >=
> > > > > > #define VARIATION_NEG
> > > > > > #define VARIATION_UNNEG   -
> > > > > > #endif
> > > > > >
> > > > > > #ifdef OPERATION_sub_ui
> > > > > > #define FUNCTION          mpz_sub_ui
> > > > > > #define FUNCTION2         mpz_sub
> > > > > > #define VARIATION_CMP     <
> > > > > > #define VARIATION_NEG     -
> > > > > > #define VARIATION_UNNEG
> > > > > > #endif
> > > > > >
> > > > > > #ifndef FUNCTION
> > > > > > Error, need OPERATION_add_ui or OPERATION_sub_ui
> > > > > > #endif
> > > > > >
> > > > > >
> > > > > > void
> > > > > > FUNCTION (mpz_ptr w, mpz_srcptr u, unsigned long int vval)
> > > > > > {
> > > > > >   mp_srcptr up;
> > > > > >   mp_ptr wp;
> > > > > >   mp_size_t usize, wsize;
> > > > > >   mp_size_t abs_usize;
> > > > > >
> > > > > > #if BITS_PER_ULONG > GMP_NUMB_BITS  /* avoid warnings about shift amount */
> > > > > >   if (vval > GMP_NUMB_MAX)
> > > > > >     {
> > > > > >       mpz_t v;
> > > > > >       mp_limb_t vl[2];
> > > > > >       PTR(v) = vl;
> > > > > >       vl[0] = vval & GMP_NUMB_MASK;
> > > > > >       vl[1] = vval >> GMP_NUMB_BITS;
> > > > > >       SIZ(v) = 2;
> > > > > >       FUNCTION2 (w, u, v);
> > > > > >       return;
> > > > > >     }
> > > > > > #endif
> > > > > >
> > > > > >   usize = SIZ (u);
> > > > > >   if (usize == 0)
> > > > > >     {
> > > > > >       MPZ_NEWALLOC (w, 1)[0] = vval;
> > > > > >       SIZ (w) = VARIATION_NEG (vval != 0);
> > > > > >       return;
> > > > > >     }
> > > > > >
> > > > > >   abs_usize = ABS (usize);
> > > > > >
> > > > > >   /* If not space for W (and possible carry), increase space.  */
> > > > > >   wp = MPZ_REALLOC (w, abs_usize + 1);
> > > > > >
> > > > > >   /* These must be after realloc (U may be the same as W).  */
> > > > > >   up = PTR (u);
> > > > > >
> > > > > >   if (usize VARIATION_CMP 0)
> > > > > >     {
> > > > > >       mp_limb_t cy;
> > > > > >       cy = mpn_add_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > > > > >       wp[abs_usize] = cy;
> > > > > >       wsize = VARIATION_NEG (abs_usize + cy);
> > > > > >     }
> > > > > >   else
> > > > > >     {
> > > > > >       /* The signs are different.  Need exact comparison to determine
> > > > > >          which operand to subtract from which.  */
> > > > > >       if (abs_usize == 1 && up[0] < vval)
> > > > > >         {
> > > > > >           wp[0] = vval - up[0];
> > > > > >           wsize = VARIATION_NEG 1;
> > > > > >         }
> > > > > >       else
> > > > > >         {
> > > > > >           mpn_sub_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > > > > >           /* Size can decrease with at most one limb.  */
> > > > > >           wsize = VARIATION_UNNEG (abs_usize - (wp[abs_usize - 1] == 0));
> > > > > >         }
> > > > > >     }
> > > > > >
> > > > > >   SIZ (w) = wsize;
> > > > > > }
> > > > > > --->*---
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Thu, Jul 16, 2020 at 11:41:17AM +0300, Ard Biesheuvel wrote:
> > > > > > > On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > > > > > >
> > > > > > > > On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> > > > > > > > >
> > > > > > > > > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..fa6b085bac36
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/lib/mpi/mpi-sub-ui.c
> > > > > > > > > @@ -0,0 +1,60 @@
> > > > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > > > +/* mpi-sub-ui.c  -  MPI functions
> > > > > > > > > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > > > > > + *      Free Software Foundation, Inc.
> > > > > > > > > + *
> > > > > > > > > + * This file is part of GnuPG.
> > > > > > > > > + *
> > > > > > > > > + * Note: This code is heavily based on the GNU MP Library.
> > > > > > > > > + *    Actually it's the same code with only minor changes in the
> > > > > > > > > + *    way the data is stored; this is to support the abstraction
> > > > > > > > > + *    of an optional secure memory allocation which may be used
> > > > > > > > > + *    to avoid revealing of sensitive data due to paging etc.
> > > > > > > > > + *    The GNU MP Library itself is published under the LGPL;
> > > > > > > > > + *    however I decided to publish this code under the plain GPL.
> > > > > > > > > + */
> > > > > > > >
> > > > > > > > Hmm, you said that this code is from GNU MP.  But this notice clearly
> > > > > > > > says that it's part of GnuPG and is under GPL.  Though it doesn't
> > > > > > > > clarify what version of GPL it is.  Can you please clarify this with
> > > > > > > > the author?
> > > > > > > >
> > > > > > >
> > > > > > > GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
> > > > > > > copyright years and the explicit statements that the file is part of
> > > > > > > GnuPG and not under the original LGPL license, there is no way we can
> > > > > > > take this code under the kernel's GPLv2 license.
> > > > > >
> > > > > > --
> > > > > > Regards,
> > > > > > Marcelo
> > > > > >
> > > >
> > > > --
> > > > Regards,
> > > > Marcelo
> > > >
> >
> > --
> > Regards,
> > Marcelo
> >

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-16 14:56                     ` Marcelo Henrique Cerri
@ 2020-07-16 15:20                       ` Ard Biesheuvel
  0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2020-07-16 15:20 UTC (permalink / raw)
  To: Marcelo Henrique Cerri
  Cc: Herbert Xu, Stephan Müller, Linux Crypto Mailing List,
	Tianjia Zhang, Ard Biesheuvel, nhorman, simo

On Thu, 16 Jul 2020 at 17:56, Marcelo Henrique Cerri
<marcelo.cerri@canonical.com> wrote:
>
> On Thu, Jul 16, 2020 at 05:37:32PM +0300, Ard Biesheuvel wrote:
> > On Thu, 16 Jul 2020 at 17:23, Marcelo Henrique Cerri
> > <marcelo.cerri@canonical.com> wrote:
> > >
> > > On Thu, Jul 16, 2020 at 04:53:24PM +0300, Ard Biesheuvel wrote:
> > > > On Thu, 16 Jul 2020 at 16:41, Marcelo Henrique Cerri
> > > > <marcelo.cerri@canonical.com> wrote:
> > > > >
> > > > > On Thu, Jul 16, 2020 at 04:09:39PM +0300, Ard Biesheuvel wrote:
> > > > > > On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
> > > > > > <marcelo.cerri@canonical.com> wrote:
> > > > > > >
> > > > > > > No. The code is really based on Gnu MP. I used the header from
> > > > > > > lib/mpi/mpi-pow.c as reference and that's source of the mention to
> > > > > > > GnuPG that went unnoticed by me.
> > > > > > >
> > > > > >
> > > > > > So where did the file lib/mpi/mpi-sub-ui.c come from? From GnuPG or
> > > > > > from GnuMP? Did you modify the license statement? Because as proposed,
> > > > > > this patch clearly is not acceptable from GPL compliance  point of
> > > > > > view.
> > > > >
> > > > > Sorry for the confusion. The code is from Gnu MP (not GnuPG).
> > > > >
> > > > > Gnu MP is distributed under either LGPLv3 or later or GPLv2 or later
> > > > > (check their license statement on the aors_ui.h file below).
> > > > >
> > > > > For mpi-sub-ui.h I added a SPDX identifier for GPLv2 or later and I
> > > > > kept the FSF copyright line.
> > > > >
> > > > > I also used the header from mpi-powm.c as a reference basically to
> > > > > inform the code was changed from its original form.
> > > > >
> > > > > Here lies my mistake, I didn't notice that part was referring to GnuPG
> > > > > instead of Gnu MP.
> > > > >
> > > > > So mpi-sub-ui.h header was actually intended to be:
> > > > >
> > > > >     // SPDX-License-Identifier: GPL-2.0-or-later
> > > > >     /* mpi-sub-ui.c  -  MPI functions
> > > > >      *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > >      *      Free Software Foundation, Inc.
> > > > >      *
> > > > >      * This file is part of Gnu MP.
> > > > >      *
> > > > >      * Note: This code is heavily based on the GNU MP Library.
> > > > >      *      Actually it's the same code with only minor changes in the
> > > > >      *      way the data is stored; this is to support the abstraction
> > > > >      *      of an optional secure memory allocation which may be used
> > > > >      *      to avoid revealing of sensitive data due to paging etc.
> > > > >      *      The GNU MP Library itself is published under the LGPL;
> > > > >      *      however I decided to publish this code under the plain GPL.
> > > > >      */
> > > > >
> > > > > Or maybe instead of "This file is part of Gnu MP.", "This file is
> > > > > based on Gnu MP" might be more appropriate.
> > > > >
> > > > > Do you have any license concerns considering this updated header?
> > > > >
> > > >
> > > > Yes. How can this code be both part of GnuMP *and* be heavily based on
> > > > it, but with changes?
> > > >
>
> The final mpi-sub-ui.c is not part of Gnu MP, but heavily based on it
> sounds very accurate to me.
>
> >
> > You haven't answered this question yet. I suppose you just slapped a
> > different license text on this file, one that already existed in
> > lib/mpi?
>
> Correct. The copyright line I used matches the original copyright from
> Gnu MP.
>
> The "This file is part..." line and the Note is the same from
> mpi/lib/mpi-powm.c.
>
> However I'm proposing the removal of that part in favor of the new
> header I listed below based on Gnu MP header.
>
> >
> > > > Please avoid making changes to the original header, just add the SPDX
> > > > header in front, and add a clear justification in the commit log where
> > > > the file came from (preferably including git url and commit ID), and
> > > > what you based your assertion on that its license is compatible with
> > > > GPLv2.
> > >
> > > The commit message is stating the origin, but I can add a reference to
> > > the mercurial repo with its corresponding id.
> > >
> >
> > Yes, please.
> >
> > > >
> > > > Ideally, you would import the file *exactly* as it appears in the
> > > > upstream in one patch (with the above justification), and apply any
> > > > necessary changes in a subsequent patch, so it's  crystal clear that
> > > > we are complying with the original license.
> > >
> > > I'm not sure that's the ideal approach for this case. The logic is the
> > > same but since naming convention, macros, data types and etc are
> > > pretty different everything was basically re-written to fit the
> > > kernel. Adding the original file and then massively changing will just
> > > add unnecessary noise.
> > >
> >
> > Do any of these modifications resemble the changes made to the GnuPG
> > versions of these routines?
>
> I haven't looked at the GnuPG code at all. Neither while I was
> preparing this patch, neither after or before. So I can affirm this
> patch has no influence of GnuPG code at all.
>
> But if you want to be sure I can check that. Please let me know.
>

No please don't. If you never looked at it, no reason to do it now.

> As I said, the GnuPG reference came from me trying to re-use the
> header from kernel's lib/mpi/mpi-powm.c file.
>

Fair enough. In that case, i have no objections.


> >
> > > If you agree I will update the commit message with more details about
> > > the original source and then just update the comment header in
> > > mpi-sub-ui.c following closely the original header with minor
> > > adjustments to explain its origin and to fix some checkpatch warnings.
> > >
> >
> > That is fine, provided that none of our modifications were taken from
> > the GnuPG version of this file without giving credit.
>
> I can assure I haven't used any code from GnuPG at all.
>
> >
> >
> > > Something like that:
> > >
> > > // SPDX-License-Identifier: GPL-2.0-or-later
> > > /* mpi-sub-ui.c - Subtract an unsigned integer from an MPI.
> > >  *
> > >  * Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > >  * Free Software Foundation, Inc.
> > >  *
> > >  * This file was based on the GNU MP Library source file:
> > >  * https://gmplib.org/repo/gmp-6.2/file/tip/mpz/aors_ui.h
> > >  *
> > >  * The GNU MP Library is free software; you can redistribute it and/or modify
> > >  * it under the terms of either:
> > >  *
> > >  *   * the GNU Lesser General Public License as published by the Free
> > >  *     Software Foundation; either version 3 of the License, or (at your
> > >  *     option) any later version.
> > >  *
> > >  * or
> > >  *
> > >  *   * the GNU General Public License as published by the Free Software
> > >  *     Foundation; either version 2 of the License, or (at your option) any
> > >  *     later version.
> > >  *
> > >  * or both in parallel, as here.
> > >  *
> > >  * The GNU MP Library is distributed in the hope that it will be useful, but
> > >  * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> > >  * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > >  * for more details.
> > >  *
> > >  * You should have received copies of the GNU General Public License and the
> > >  * GNU Lesser General Public License along with the GNU MP Library.  If not,
> > >  * see https://www.gnu.org/licenses/.
> > >  */
> > >
> > > >
> > > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > You can find the original Gnu MP source that I used as reference in
> > > > > > > the file gmp-6.2.0/mpz/aors_ui.h from:
> > > > > > >
> > > > > > > https://gmplib.org/download/gmp/gmp-6.2.0.tar.lz
> > > > > > >
> > > > > > > I'm pasting the contents of gmp-6.2.0/mpz/aors_ui.h below for
> > > > > > > reference. Do you think we should use or adapt the original header
> > > > > > > instead?
> > > > > > >
> > > > > > > That said, assuming the patch set submitted by Tianjia is updated to
> > > > > > > ensure that mpi_sub_ui() and other functions are returning allocation
> > > > > > > errors, we could drop this patch in favor of that patch set that is
> > > > > > > more extensive and also provides an implementation to mpi_sub_ui().
> > > > > > >
> > > > > > >
> > > > > > > --->8---
> > > > > > > /* mpz_add_ui, mpz_sub_ui -- Add or subtract an mpz_t and an unsigned
> > > > > > >    one-word integer.
> > > > > > >
> > > > > > > Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > > > Free Software Foundation, Inc.
> > > > > > >
> > > > >
> > > > >
> > > > > Gnu MP license -.
> > > > >                 V
> > > > >
> > > > >
> > > > > > > This file is part of the GNU MP Library.
> > > > > > >
> > > > > > > The GNU MP Library is free software; you can redistribute it and/or modify
> > > > > > > it under the terms of either:
> > > > > > >
> > > > > > >   * the GNU Lesser General Public License as published by the Free
> > > > > > >     Software Foundation; either version 3 of the License, or (at your
> > > > > > >     option) any later version.
> > > > > > >
> > > > > > > or
> > > > > > >
> > > > > > >   * the GNU General Public License as published by the Free Software
> > > > > > >     Foundation; either version 2 of the License, or (at your option) any
> > > > > > >     later version.
> > > > > > >
> > > > > > > or both in parallel, as here.
> > > > > > >
> > > > > > > The GNU MP Library is distributed in the hope that it will be useful, but
> > > > > > > WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> > > > > > > or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > > > > > > for more details.
> > > > > > >
> > > > > > > You should have received copies of the GNU General Public License and the
> > > > > > > GNU Lesser General Public License along with the GNU MP Library.  If not,
> > > > > > > see https://www.gnu.org/licenses/.  */
> > > > > > >
> > > > > > > #include "gmp-impl.h"
> > > > > > >
> > > > > > >
> > > > > > > #ifdef OPERATION_add_ui
> > > > > > > #define FUNCTION          mpz_add_ui
> > > > > > > #define FUNCTION2         mpz_add
> > > > > > > #define VARIATION_CMP     >=
> > > > > > > #define VARIATION_NEG
> > > > > > > #define VARIATION_UNNEG   -
> > > > > > > #endif
> > > > > > >
> > > > > > > #ifdef OPERATION_sub_ui
> > > > > > > #define FUNCTION          mpz_sub_ui
> > > > > > > #define FUNCTION2         mpz_sub
> > > > > > > #define VARIATION_CMP     <
> > > > > > > #define VARIATION_NEG     -
> > > > > > > #define VARIATION_UNNEG
> > > > > > > #endif
> > > > > > >
> > > > > > > #ifndef FUNCTION
> > > > > > > Error, need OPERATION_add_ui or OPERATION_sub_ui
> > > > > > > #endif
> > > > > > >
> > > > > > >
> > > > > > > void
> > > > > > > FUNCTION (mpz_ptr w, mpz_srcptr u, unsigned long int vval)
> > > > > > > {
> > > > > > >   mp_srcptr up;
> > > > > > >   mp_ptr wp;
> > > > > > >   mp_size_t usize, wsize;
> > > > > > >   mp_size_t abs_usize;
> > > > > > >
> > > > > > > #if BITS_PER_ULONG > GMP_NUMB_BITS  /* avoid warnings about shift amount */
> > > > > > >   if (vval > GMP_NUMB_MAX)
> > > > > > >     {
> > > > > > >       mpz_t v;
> > > > > > >       mp_limb_t vl[2];
> > > > > > >       PTR(v) = vl;
> > > > > > >       vl[0] = vval & GMP_NUMB_MASK;
> > > > > > >       vl[1] = vval >> GMP_NUMB_BITS;
> > > > > > >       SIZ(v) = 2;
> > > > > > >       FUNCTION2 (w, u, v);
> > > > > > >       return;
> > > > > > >     }
> > > > > > > #endif
> > > > > > >
> > > > > > >   usize = SIZ (u);
> > > > > > >   if (usize == 0)
> > > > > > >     {
> > > > > > >       MPZ_NEWALLOC (w, 1)[0] = vval;
> > > > > > >       SIZ (w) = VARIATION_NEG (vval != 0);
> > > > > > >       return;
> > > > > > >     }
> > > > > > >
> > > > > > >   abs_usize = ABS (usize);
> > > > > > >
> > > > > > >   /* If not space for W (and possible carry), increase space.  */
> > > > > > >   wp = MPZ_REALLOC (w, abs_usize + 1);
> > > > > > >
> > > > > > >   /* These must be after realloc (U may be the same as W).  */
> > > > > > >   up = PTR (u);
> > > > > > >
> > > > > > >   if (usize VARIATION_CMP 0)
> > > > > > >     {
> > > > > > >       mp_limb_t cy;
> > > > > > >       cy = mpn_add_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > > > > > >       wp[abs_usize] = cy;
> > > > > > >       wsize = VARIATION_NEG (abs_usize + cy);
> > > > > > >     }
> > > > > > >   else
> > > > > > >     {
> > > > > > >       /* The signs are different.  Need exact comparison to determine
> > > > > > >          which operand to subtract from which.  */
> > > > > > >       if (abs_usize == 1 && up[0] < vval)
> > > > > > >         {
> > > > > > >           wp[0] = vval - up[0];
> > > > > > >           wsize = VARIATION_NEG 1;
> > > > > > >         }
> > > > > > >       else
> > > > > > >         {
> > > > > > >           mpn_sub_1 (wp, up, abs_usize, (mp_limb_t) vval);
> > > > > > >           /* Size can decrease with at most one limb.  */
> > > > > > >           wsize = VARIATION_UNNEG (abs_usize - (wp[abs_usize - 1] == 0));
> > > > > > >         }
> > > > > > >     }
> > > > > > >
> > > > > > >   SIZ (w) = wsize;
> > > > > > > }
> > > > > > > --->*---
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Thu, Jul 16, 2020 at 11:41:17AM +0300, Ard Biesheuvel wrote:
> > > > > > > > On Thu, 16 Jul 2020 at 10:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Jul 12, 2020 at 06:39:54PM +0200, Stephan Müller wrote:
> > > > > > > > > >
> > > > > > > > > > diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 000000000000..fa6b085bac36
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/lib/mpi/mpi-sub-ui.c
> > > > > > > > > > @@ -0,0 +1,60 @@
> > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > > > > +/* mpi-sub-ui.c  -  MPI functions
> > > > > > > > > > + *      Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
> > > > > > > > > > + *      Free Software Foundation, Inc.
> > > > > > > > > > + *
> > > > > > > > > > + * This file is part of GnuPG.
> > > > > > > > > > + *
> > > > > > > > > > + * Note: This code is heavily based on the GNU MP Library.
> > > > > > > > > > + *    Actually it's the same code with only minor changes in the
> > > > > > > > > > + *    way the data is stored; this is to support the abstraction
> > > > > > > > > > + *    of an optional secure memory allocation which may be used
> > > > > > > > > > + *    to avoid revealing of sensitive data due to paging etc.
> > > > > > > > > > + *    The GNU MP Library itself is published under the LGPL;
> > > > > > > > > > + *    however I decided to publish this code under the plain GPL.
> > > > > > > > > > + */
> > > > > > > > >
> > > > > > > > > Hmm, you said that this code is from GNU MP.  But this notice clearly
> > > > > > > > > says that it's part of GnuPG and is under GPL.  Though it doesn't
> > > > > > > > > clarify what version of GPL it is.  Can you please clarify this with
> > > > > > > > > the author?
> > > > > > > > >
> > > > > > > >
> > > > > > > > GnuPG was relicensed under GPLv3 in ~2007, IIRC, so given the
> > > > > > > > copyright years and the explicit statements that the file is part of
> > > > > > > > GnuPG and not under the original LGPL license, there is no way we can
> > > > > > > > take this code under the kernel's GPLv2 license.
> > > > > > >
> > > > > > > --
> > > > > > > Regards,
> > > > > > > Marcelo
> > > > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Marcelo
> > > > >
> > >
> > > --
> > > Regards,
> > > Marcelo
> > >
>
> --
> Regards,
> Marcelo
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks
  2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
                     ` (4 preceding siblings ...)
  2020-07-12 16:42   ` [PATCH v2 5/5] crypto: ECDH " Stephan Müller
@ 2020-07-20 17:05   ` Stephan Müller
  2020-07-20 17:07     ` [PATCH v3 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
                       ` (6 more replies)
  5 siblings, 7 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-20 17:05 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

Hi,

This patch set adds the required checks to make all aspects of
(EC)DH compliant with SP800-56A rev 3 assuming that all keys
are ephemeral. The use of static keys adds yet additional
validations which are hard to achieve in the kernel.

SP800-56A rev 3 mandates various checks:

- validation of remote public key defined in section 5.6.2.2.2
  is already implemented in:

  * ECC: crypto_ecdh_shared_secret with the call of
    ecc_is_pubkey_valid_partial

  * FFC: dh_compute_val when the req->src is read and validated with
    dh_is_pubkey_valid

- validation of generated shared secret: The patch set adds the
  shared secret validation as defined by SP800-56A rev 3. For
  ECDH this only implies that the validation of the shared secret
  is moved before the shared secret is returned to the caller.

  For DH, the validation is required to be performed against the prime
  of the domain parameter set.

  This patch adds the MPI library file mpi_sub_ui that is required
  to calculate P - 1 for the DH check. It would be possible, though
  to simply set the LSB of the prime to 0 to obtain P - 1 (since
  P is odd per definition) which implies that mpi_sub_ui would not
  be needed. However, this would require a copy operation from
  the existing prime MPI value into a temporary MPI where the
  modification can be performed. Such copy operation is not available.
  Therefore, the solution with the addition of mpi_sub_ui was chosen.

  NOTE: The function mpi_sub_ui is also added with the patch set
  "[PATCH v5 2/8] lib/mpi: Extend the MPI library" currently sent
  to the linux-crypto mailing list.

- validation of the generated local public key: Patches 4 and 5 of
  this patch set adds the required checks.

Changes to v2:

- add reference to GnuMP providing the basis for patch 2 and updating
  the copyright note in patch 2

Changes to v1:

- fix reference to Gnu MP as outlined by Ard Biesheuvel
- addition of patches 4 and 5

Marcelo Henrique Cerri (1):
  lib/mpi: Add mpi_sub_ui()

Stephan Mueller (4):
  crypto: ECDH - check validity of Z before export
  crypto: DH - check validity of Z before export
  crypto: DH SP800-56A rev 3 local public key validation
  crypto: ECDH SP800-56A rev 3 local public key validation

 crypto/dh.c          | 38 +++++++++++++++++++++
 crypto/ecc.c         | 42 +++++++++++++++++++++---
 crypto/ecc.h         | 14 ++++++++
 include/linux/mpi.h  |  3 ++
 lib/mpi/Makefile     |  1 +
 lib/mpi/mpi-sub-ui.c | 78 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 172 insertions(+), 4 deletions(-)
 create mode 100644 lib/mpi/mpi-sub-ui.c

-- 
2.26.2





^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v3 1/5] crypto: ECDH - check validity of Z before export
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
@ 2020-07-20 17:07     ` Stephan Müller
  2020-07-22 13:11       ` Vitaly Chikunov
  2020-07-24 17:47       ` Neil Horman
  2020-07-20 17:08     ` [PATCH v3 2/5] lib/mpi: Add mpi_sub_ui() Stephan Müller
                       ` (5 subsequent siblings)
  6 siblings, 2 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-20 17:07 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

SP800-56A rev3 section 5.7.1.2 step 2 mandates that the validity of the
calculated shared secret is verified before the data is returned to the
caller. Thus, the export function and the validity check functions are
reversed. In addition, the sensitive variables of priv and rand_z are
zeroized.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/ecc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 02d35be7702b..52e2d49262f2 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1495,11 +1495,16 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 
 	ecc_point_mult(product, pk, priv, rand_z, curve, ndigits);
 
-	ecc_swap_digits(product->x, secret, ndigits);
-
-	if (ecc_point_is_zero(product))
+	if (ecc_point_is_zero(product)) {
 		ret = -EFAULT;
+		goto err_validity;
+	}
+
+	ecc_swap_digits(product->x, secret, ndigits);
 
+err_validity:
+	memzero_explicit(priv, sizeof(priv));
+	memzero_explicit(rand_z, sizeof(rand_z));
 	ecc_free_point(product);
 err_alloc_product:
 	ecc_free_point(pk);
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v3 2/5] lib/mpi: Add mpi_sub_ui()
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
  2020-07-20 17:07     ` [PATCH v3 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
@ 2020-07-20 17:08     ` Stephan Müller
  2020-07-20 17:08     ` [PATCH v3 3/5] crypto: DH - check validity of Z before export Stephan Müller
                       ` (4 subsequent siblings)
  6 siblings, 0 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-20 17:08 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

From: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

Add mpi_sub_ui() based on Gnu MP mpz_sub_ui() function from file
mpz/aors_ui.h[1] from change id 510b83519d1c adapting the code to the
kernel's data structures, helper functions and coding style and also
removing the defines used to produce mpz_sub_ui() and mpz_add_ui()
from the same code.

[1] https://gmplib.org/repo/gmp-6.2/file/510b83519d1c/mpz/aors.h

Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 include/linux/mpi.h  |  3 ++
 lib/mpi/Makefile     |  1 +
 lib/mpi/mpi-sub-ui.c | 78 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)
 create mode 100644 lib/mpi/mpi-sub-ui.c

diff --git a/include/linux/mpi.h b/include/linux/mpi.h
index 7bd6d8af0004..5d906dfbf3ed 100644
--- a/include/linux/mpi.h
+++ b/include/linux/mpi.h
@@ -63,6 +63,9 @@ int mpi_powm(MPI res, MPI base, MPI exp, MPI mod);
 int mpi_cmp_ui(MPI u, ulong v);
 int mpi_cmp(MPI u, MPI v);
 
+/*-- mpi-sub-ui.c --*/
+int mpi_sub_ui(MPI w, MPI u, unsigned long vval);
+
 /*-- mpi-bit.c --*/
 void mpi_normalize(MPI a);
 unsigned mpi_get_nbits(MPI a);
diff --git a/lib/mpi/Makefile b/lib/mpi/Makefile
index d5874a7f5ff9..43b8fce14079 100644
--- a/lib/mpi/Makefile
+++ b/lib/mpi/Makefile
@@ -16,6 +16,7 @@ mpi-y = \
 	mpicoder.o			\
 	mpi-bit.o			\
 	mpi-cmp.o			\
+	mpi-sub-ui.o			\
 	mpih-cmp.o			\
 	mpih-div.o			\
 	mpih-mul.o			\
diff --git a/lib/mpi/mpi-sub-ui.c b/lib/mpi/mpi-sub-ui.c
new file mode 100644
index 000000000000..b41b082b5f3e
--- /dev/null
+++ b/lib/mpi/mpi-sub-ui.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* mpi-sub-ui.c - Subtract an unsigned integer from an MPI.
+ *
+ * Copyright 1991, 1993, 1994, 1996, 1999-2002, 2004, 2012, 2013, 2015
+ * Free Software Foundation, Inc.
+ *
+ * This file was based on the GNU MP Library source file:
+ * https://gmplib.org/repo/gmp-6.2/file/510b83519d1c/mpz/aors_ui.h
+ *
+ * The GNU MP Library is free software; you can redistribute it and/or modify
+ * it under the terms of either:
+ *
+ *   * the GNU Lesser General Public License as published by the Free
+ *     Software Foundation; either version 3 of the License, or (at your
+ *     option) any later version.
+ *
+ * or
+ *
+ *   * the GNU General Public License as published by the Free Software
+ *     Foundation; either version 2 of the License, or (at your option) any
+ *     later version.
+ *
+ * or both in parallel, as here.
+ *
+ * The GNU MP Library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ *
+ * You should have received copies of the GNU General Public License and the
+ * GNU Lesser General Public License along with the GNU MP Library.  If not,
+ * see https://www.gnu.org/licenses/.
+ */
+
+#include "mpi-internal.h"
+
+int mpi_sub_ui(MPI w, MPI u, unsigned long vval)
+{
+	if (u->nlimbs == 0) {
+		if (mpi_resize(w, 1) < 0)
+			return -ENOMEM;
+		w->d[0] = vval;
+		w->nlimbs = (vval != 0);
+		w->sign = (vval != 0);
+		return 0;
+	}
+
+	/* If not space for W (and possible carry), increase space. */
+	if (mpi_resize(w, u->nlimbs + 1))
+		return -ENOMEM;
+
+	if (u->sign) {
+		mpi_limb_t cy;
+
+		cy = mpihelp_add_1(w->d, u->d, u->nlimbs, (mpi_limb_t) vval);
+		w->d[u->nlimbs] = cy;
+		w->nlimbs = u->nlimbs + cy;
+		w->sign = 1;
+	} else {
+		/* The signs are different.  Need exact comparison to determine
+		 * which operand to subtract from which.
+		 */
+		if (u->nlimbs == 1 && u->d[0] < vval) {
+			w->d[0] = vval - u->d[0];
+			w->nlimbs = 1;
+			w->sign = 1;
+		} else {
+			mpihelp_sub_1(w->d, u->d, u->nlimbs, (mpi_limb_t) vval);
+			/* Size can decrease with at most one limb. */
+			w->nlimbs = (u->nlimbs - (w->d[u->nlimbs - 1] == 0));
+			w->sign = 0;
+		}
+	}
+
+	mpi_normalize(w);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mpi_sub_ui);
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v3 3/5] crypto: DH - check validity of Z before export
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
  2020-07-20 17:07     ` [PATCH v3 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
  2020-07-20 17:08     ` [PATCH v3 2/5] lib/mpi: Add mpi_sub_ui() Stephan Müller
@ 2020-07-20 17:08     ` Stephan Müller
  2020-07-24 18:02       ` Neil Horman
  2020-07-20 17:08     ` [PATCH v3 4/5] crypto: DH SP800-56A rev 3 local public key validation Stephan Müller
                       ` (3 subsequent siblings)
  6 siblings, 1 reply; 42+ messages in thread
From: Stephan Müller @ 2020-07-20 17:08 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

SP800-56A rev3 section 5.7.1.1 step 2 mandates that the validity of the
calculated shared secret is verified before the data is returned to the
caller. This patch adds the validation check.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/dh.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/crypto/dh.c b/crypto/dh.c
index 566f624a2de2..f84fd50ec79b 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -9,6 +9,7 @@
 #include <crypto/internal/kpp.h>
 #include <crypto/kpp.h>
 #include <crypto/dh.h>
+#include <linux/fips.h>
 #include <linux/mpi.h>
 
 struct dh_ctx {
@@ -179,6 +180,34 @@ static int dh_compute_value(struct kpp_request *req)
 	if (ret)
 		goto err_free_base;
 
+	/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
+	if (fips_enabled && req->src) {
+		MPI pone;
+
+		/* z <= 1 */
+		if (mpi_cmp_ui(val, 1) < 1) {
+			ret = -EBADMSG;
+			goto err_free_base;
+		}
+
+		/* z == p - 1 */
+		pone = mpi_alloc(0);
+
+		if (!pone) {
+			ret = -ENOMEM;
+			goto err_free_base;
+		}
+
+		ret = mpi_sub_ui(pone, ctx->p, 1);
+		if (!ret && !mpi_cmp(pone, val))
+			ret = -EBADMSG;
+
+		mpi_free(pone);
+
+		if (ret)
+			goto err_free_base;
+	}
+
 	ret = mpi_write_to_sgl(val, req->dst, req->dst_len, &sign);
 	if (ret)
 		goto err_free_base;
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v3 4/5] crypto: DH SP800-56A rev 3 local public key validation
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
                       ` (2 preceding siblings ...)
  2020-07-20 17:08     ` [PATCH v3 3/5] crypto: DH - check validity of Z before export Stephan Müller
@ 2020-07-20 17:08     ` Stephan Müller
  2020-07-20 17:09     ` [PATCH v3 5/5] crypto: ECDH " Stephan Müller
                       ` (2 subsequent siblings)
  6 siblings, 0 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-20 17:08 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

After the generation of a local public key, SP800-56A rev 3 section
5.6.2.1.3 mandates a validation of that key with a full validation
compliant to section 5.6.2.3.1.

Only if the full validation passes, the key is allowed to be used.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/dh.c | 59 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/crypto/dh.c b/crypto/dh.c
index f84fd50ec79b..cd4f32092e5c 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -180,32 +180,41 @@ static int dh_compute_value(struct kpp_request *req)
 	if (ret)
 		goto err_free_base;
 
-	/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
-	if (fips_enabled && req->src) {
-		MPI pone;
-
-		/* z <= 1 */
-		if (mpi_cmp_ui(val, 1) < 1) {
-			ret = -EBADMSG;
-			goto err_free_base;
-		}
-
-		/* z == p - 1 */
-		pone = mpi_alloc(0);
-
-		if (!pone) {
-			ret = -ENOMEM;
-			goto err_free_base;
+	if (fips_enabled) {
+		/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
+		if (req->src) {
+			MPI pone;
+
+			/* z <= 1 */
+			if (mpi_cmp_ui(val, 1) < 1) {
+				ret = -EBADMSG;
+				goto err_free_base;
+			}
+
+			/* z == p - 1 */
+			pone = mpi_alloc(0);
+
+			if (!pone) {
+				ret = -ENOMEM;
+				goto err_free_base;
+			}
+
+			ret = mpi_sub_ui(pone, ctx->p, 1);
+			if (!ret && !mpi_cmp(pone, val))
+				ret = -EBADMSG;
+
+			mpi_free(pone);
+
+			if (ret)
+				goto err_free_base;
+
+		/* SP800-56A rev 3 5.6.2.1.3 key check */
+		} else {
+			if (dh_is_pubkey_valid(ctx, val)) {
+				ret = -EAGAIN;
+				goto err_free_val;
+			}
 		}
-
-		ret = mpi_sub_ui(pone, ctx->p, 1);
-		if (!ret && !mpi_cmp(pone, val))
-			ret = -EBADMSG;
-
-		mpi_free(pone);
-
-		if (ret)
-			goto err_free_base;
 	}
 
 	ret = mpi_write_to_sgl(val, req->dst, req->dst_len, &sign);
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v3 5/5] crypto: ECDH SP800-56A rev 3 local public key validation
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
                       ` (3 preceding siblings ...)
  2020-07-20 17:08     ` [PATCH v3 4/5] crypto: DH SP800-56A rev 3 local public key validation Stephan Müller
@ 2020-07-20 17:09     ` Stephan Müller
  2020-07-21 11:35     ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Marcelo Henrique Cerri
  2020-07-31 13:29     ` Herbert Xu
  6 siblings, 0 replies; 42+ messages in thread
From: Stephan Müller @ 2020-07-20 17:09 UTC (permalink / raw)
  To: herbert
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

After the generation of a local public key, SP800-56A rev 3 section
5.6.2.1.3 mandates a validation of that key with a full validation
compliant to section 5.6.2.3.3.

Only if the full validation passes, the key is allowed to be used.

The patch adds the full key validation compliant to 5.6.2.3.3 and
performs the required check on the generated public key.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/ecc.c | 31 ++++++++++++++++++++++++++++++-
 crypto/ecc.h | 14 ++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 52e2d49262f2..7308487e7c55 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1404,7 +1404,9 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
 	}
 
 	ecc_point_mult(pk, &curve->g, priv, NULL, curve, ndigits);
-	if (ecc_point_is_zero(pk)) {
+
+	/* SP800-56A rev 3 5.6.2.1.3 key check */
+	if (ecc_is_pubkey_valid_full(curve, pk)) {
 		ret = -EAGAIN;
 		goto err_free_point;
 	}
@@ -1452,6 +1454,33 @@ int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
 }
 EXPORT_SYMBOL(ecc_is_pubkey_valid_partial);
 
+/* SP800-56A section 5.6.2.3.3 full verification */
+int ecc_is_pubkey_valid_full(const struct ecc_curve *curve,
+			     struct ecc_point *pk)
+{
+	struct ecc_point *nQ;
+
+	/* Checks 1 through 3 */
+	int ret = ecc_is_pubkey_valid_partial(curve, pk);
+
+	if (ret)
+		return ret;
+
+	/* Check 4: Verify that nQ is the zero point. */
+	nQ = ecc_alloc_point(pk->ndigits);
+	if (!nQ)
+		return -ENOMEM;
+
+	ecc_point_mult(nQ, pk, curve->n, NULL, curve, pk->ndigits);
+	if (!ecc_point_is_zero(nQ))
+		ret = -EINVAL;
+
+	ecc_free_point(nQ);
+
+	return ret;
+}
+EXPORT_SYMBOL(ecc_is_pubkey_valid_full);
+
 int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 			      const u64 *private_key, const u64 *public_key,
 			      u64 *secret)
diff --git a/crypto/ecc.h b/crypto/ecc.h
index ab0eb70b9c09..d4e546b9ad79 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -147,6 +147,20 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
 				struct ecc_point *pk);
 
+/**
+ * ecc_is_pubkey_valid_full() - Full public key validation
+ *
+ * @curve:		elliptic curve domain parameters
+ * @pk:			public key as a point
+ *
+ * Valdiate public key according to SP800-56A section 5.6.2.3.3 ECC Full
+ * Public-Key Validation Routine.
+ *
+ * Return: 0 if validation is successful, -EINVAL if validation is failed.
+ */
+int ecc_is_pubkey_valid_full(const struct ecc_curve *curve,
+			     struct ecc_point *pk);
+
 /**
  * vli_is_zero() - Determine is vli is zero
  *
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
                       ` (4 preceding siblings ...)
  2020-07-20 17:09     ` [PATCH v3 5/5] crypto: ECDH " Stephan Müller
@ 2020-07-21 11:35     ` Marcelo Henrique Cerri
  2020-07-31 13:29     ` Herbert Xu
  6 siblings, 0 replies; 42+ messages in thread
From: Marcelo Henrique Cerri @ 2020-07-21 11:35 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Tianjia Zhang, ard.biesheuvel, nhorman, simo

[-- Attachment #1: Type: text/plain, Size: 3075 bytes --]

Reviewed-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Tested-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

On Mon, Jul 20, 2020 at 07:05:45PM +0200, Stephan Müller wrote:
> Hi,
> 
> This patch set adds the required checks to make all aspects of
> (EC)DH compliant with SP800-56A rev 3 assuming that all keys
> are ephemeral. The use of static keys adds yet additional
> validations which are hard to achieve in the kernel.
> 
> SP800-56A rev 3 mandates various checks:
> 
> - validation of remote public key defined in section 5.6.2.2.2
>   is already implemented in:
> 
>   * ECC: crypto_ecdh_shared_secret with the call of
>     ecc_is_pubkey_valid_partial
> 
>   * FFC: dh_compute_val when the req->src is read and validated with
>     dh_is_pubkey_valid
> 
> - validation of generated shared secret: The patch set adds the
>   shared secret validation as defined by SP800-56A rev 3. For
>   ECDH this only implies that the validation of the shared secret
>   is moved before the shared secret is returned to the caller.
> 
>   For DH, the validation is required to be performed against the prime
>   of the domain parameter set.
> 
>   This patch adds the MPI library file mpi_sub_ui that is required
>   to calculate P - 1 for the DH check. It would be possible, though
>   to simply set the LSB of the prime to 0 to obtain P - 1 (since
>   P is odd per definition) which implies that mpi_sub_ui would not
>   be needed. However, this would require a copy operation from
>   the existing prime MPI value into a temporary MPI where the
>   modification can be performed. Such copy operation is not available.
>   Therefore, the solution with the addition of mpi_sub_ui was chosen.
> 
>   NOTE: The function mpi_sub_ui is also added with the patch set
>   "[PATCH v5 2/8] lib/mpi: Extend the MPI library" currently sent
>   to the linux-crypto mailing list.
> 
> - validation of the generated local public key: Patches 4 and 5 of
>   this patch set adds the required checks.
> 
> Changes to v2:
> 
> - add reference to GnuMP providing the basis for patch 2 and updating
>   the copyright note in patch 2
> 
> Changes to v1:
> 
> - fix reference to Gnu MP as outlined by Ard Biesheuvel
> - addition of patches 4 and 5
> 
> Marcelo Henrique Cerri (1):
>   lib/mpi: Add mpi_sub_ui()
> 
> Stephan Mueller (4):
>   crypto: ECDH - check validity of Z before export
>   crypto: DH - check validity of Z before export
>   crypto: DH SP800-56A rev 3 local public key validation
>   crypto: ECDH SP800-56A rev 3 local public key validation
> 
>  crypto/dh.c          | 38 +++++++++++++++++++++
>  crypto/ecc.c         | 42 +++++++++++++++++++++---
>  crypto/ecc.h         | 14 ++++++++
>  include/linux/mpi.h  |  3 ++
>  lib/mpi/Makefile     |  1 +
>  lib/mpi/mpi-sub-ui.c | 78 ++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 172 insertions(+), 4 deletions(-)
>  create mode 100644 lib/mpi/mpi-sub-ui.c
> 
> -- 
> 2.26.2
> 
> 
> 
> 

-- 
Regards,
Marcelo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v3 1/5] crypto: ECDH - check validity of Z before export
  2020-07-20 17:07     ` [PATCH v3 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
@ 2020-07-22 13:11       ` Vitaly Chikunov
  2020-07-24 17:47       ` Neil Horman
  1 sibling, 0 replies; 42+ messages in thread
From: Vitaly Chikunov @ 2020-07-22 13:11 UTC (permalink / raw)
  To: herbert, linux-crypto, Marcelo Cerri, Tianjia Zhang,
	ard.biesheuvel, nhorman, simo
  Cc: Stephan Müller

On Mon, Jul 20, 2020 at 07:07:48PM +0200, Stephan Müller wrote:
> SP800-56A rev3 section 5.7.1.2 step 2 mandates that the validity of the
> calculated shared secret is verified before the data is returned to the
> caller. Thus, the export function and the validity check functions are
> reversed. In addition, the sensitive variables of priv and rand_z are
> zeroized.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/ecc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

This patch seems not changed from v2, thus

Reviewed-by: Vitaly Chikunov <vt@altlinux.org>

> 
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 02d35be7702b..52e2d49262f2 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -1495,11 +1495,16 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  
>  	ecc_point_mult(product, pk, priv, rand_z, curve, ndigits);
>  
> -	ecc_swap_digits(product->x, secret, ndigits);
> -
> -	if (ecc_point_is_zero(product))
> +	if (ecc_point_is_zero(product)) {
>  		ret = -EFAULT;
> +		goto err_validity;
> +	}
> +
> +	ecc_swap_digits(product->x, secret, ndigits);
>  
> +err_validity:
> +	memzero_explicit(priv, sizeof(priv));
> +	memzero_explicit(rand_z, sizeof(rand_z));
>  	ecc_free_point(product);
>  err_alloc_product:
>  	ecc_free_point(pk);
> -- 
> 2.26.2
> 
> 
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v3 1/5] crypto: ECDH - check validity of Z before export
  2020-07-20 17:07     ` [PATCH v3 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
  2020-07-22 13:11       ` Vitaly Chikunov
@ 2020-07-24 17:47       ` Neil Horman
  1 sibling, 0 replies; 42+ messages in thread
From: Neil Horman @ 2020-07-24 17:47 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Marcelo Cerri, Tianjia Zhang,
	ard.biesheuvel, simo

On Mon, Jul 20, 2020 at 07:07:48PM +0200, Stephan Müller wrote:
> SP800-56A rev3 section 5.7.1.2 step 2 mandates that the validity of the
> calculated shared secret is verified before the data is returned to the
> caller. Thus, the export function and the validity check functions are
> reversed. In addition, the sensitive variables of priv and rand_z are
> zeroized.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/ecc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 02d35be7702b..52e2d49262f2 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -1495,11 +1495,16 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  
>  	ecc_point_mult(product, pk, priv, rand_z, curve, ndigits);
>  
> -	ecc_swap_digits(product->x, secret, ndigits);
> -
> -	if (ecc_point_is_zero(product))
> +	if (ecc_point_is_zero(product)) {
>  		ret = -EFAULT;
> +		goto err_validity;
> +	}
> +
> +	ecc_swap_digits(product->x, secret, ndigits);
>  
> +err_validity:
> +	memzero_explicit(priv, sizeof(priv));
> +	memzero_explicit(rand_z, sizeof(rand_z));
>  	ecc_free_point(product);
>  err_alloc_product:
>  	ecc_free_point(pk);
> -- 
> 2.26.2
> 
> 
> 
> 
Acked-by: Neil Horman <nhorman@redhat.com>


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v3 3/5] crypto: DH - check validity of Z before export
  2020-07-20 17:08     ` [PATCH v3 3/5] crypto: DH - check validity of Z before export Stephan Müller
@ 2020-07-24 18:02       ` Neil Horman
  0 siblings, 0 replies; 42+ messages in thread
From: Neil Horman @ 2020-07-24 18:02 UTC (permalink / raw)
  To: Stephan Müller
  Cc: herbert, linux-crypto, Marcelo Cerri, Tianjia Zhang,
	ard.biesheuvel, simo

On Mon, Jul 20, 2020 at 07:08:32PM +0200, Stephan Müller wrote:
> SP800-56A rev3 section 5.7.1.1 step 2 mandates that the validity of the
> calculated shared secret is verified before the data is returned to the
> caller. This patch adds the validation check.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/dh.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/crypto/dh.c b/crypto/dh.c
> index 566f624a2de2..f84fd50ec79b 100644
> --- a/crypto/dh.c
> +++ b/crypto/dh.c
> @@ -9,6 +9,7 @@
>  #include <crypto/internal/kpp.h>
>  #include <crypto/kpp.h>
>  #include <crypto/dh.h>
> +#include <linux/fips.h>
>  #include <linux/mpi.h>
>  
>  struct dh_ctx {
> @@ -179,6 +180,34 @@ static int dh_compute_value(struct kpp_request *req)
>  	if (ret)
>  		goto err_free_base;
>  
> +	/* SP800-56A rev3 5.7.1.1 check: Validation of shared secret */
> +	if (fips_enabled && req->src) {
> +		MPI pone;
> +
> +		/* z <= 1 */
> +		if (mpi_cmp_ui(val, 1) < 1) {
> +			ret = -EBADMSG;
> +			goto err_free_base;
> +		}
> +
> +		/* z == p - 1 */
> +		pone = mpi_alloc(0);
> +
> +		if (!pone) {
> +			ret = -ENOMEM;
> +			goto err_free_base;
> +		}
> +
> +		ret = mpi_sub_ui(pone, ctx->p, 1);
> +		if (!ret && !mpi_cmp(pone, val))
> +			ret = -EBADMSG;
> +
> +		mpi_free(pone);
> +
> +		if (ret)
> +			goto err_free_base;
> +	}
> +
>  	ret = mpi_write_to_sgl(val, req->dst, req->dst_len, &sign);
>  	if (ret)
>  		goto err_free_base;
> -- 
> 2.26.2
> 
> 
> 
> 
Acked-by: Neil Horman <nhorman@redhat.com>



^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks
  2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
                       ` (5 preceding siblings ...)
  2020-07-21 11:35     ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Marcelo Henrique Cerri
@ 2020-07-31 13:29     ` Herbert Xu
  6 siblings, 0 replies; 42+ messages in thread
From: Herbert Xu @ 2020-07-31 13:29 UTC (permalink / raw)
  To: Stephan Müller
  Cc: linux-crypto, Marcelo Cerri, Tianjia Zhang, ard.biesheuvel,
	nhorman, simo

On Mon, Jul 20, 2020 at 07:05:45PM +0200, Stephan Müller wrote:
> Hi,
> 
> This patch set adds the required checks to make all aspects of
> (EC)DH compliant with SP800-56A rev 3 assuming that all keys
> are ephemeral. The use of static keys adds yet additional
> validations which are hard to achieve in the kernel.
> 
> SP800-56A rev 3 mandates various checks:
> 
> - validation of remote public key defined in section 5.6.2.2.2
>   is already implemented in:
> 
>   * ECC: crypto_ecdh_shared_secret with the call of
>     ecc_is_pubkey_valid_partial
> 
>   * FFC: dh_compute_val when the req->src is read and validated with
>     dh_is_pubkey_valid
> 
> - validation of generated shared secret: The patch set adds the
>   shared secret validation as defined by SP800-56A rev 3. For
>   ECDH this only implies that the validation of the shared secret
>   is moved before the shared secret is returned to the caller.
> 
>   For DH, the validation is required to be performed against the prime
>   of the domain parameter set.
> 
>   This patch adds the MPI library file mpi_sub_ui that is required
>   to calculate P - 1 for the DH check. It would be possible, though
>   to simply set the LSB of the prime to 0 to obtain P - 1 (since
>   P is odd per definition) which implies that mpi_sub_ui would not
>   be needed. However, this would require a copy operation from
>   the existing prime MPI value into a temporary MPI where the
>   modification can be performed. Such copy operation is not available.
>   Therefore, the solution with the addition of mpi_sub_ui was chosen.
> 
>   NOTE: The function mpi_sub_ui is also added with the patch set
>   "[PATCH v5 2/8] lib/mpi: Extend the MPI library" currently sent
>   to the linux-crypto mailing list.
> 
> - validation of the generated local public key: Patches 4 and 5 of
>   this patch set adds the required checks.
> 
> Changes to v2:
> 
> - add reference to GnuMP providing the basis for patch 2 and updating
>   the copyright note in patch 2
> 
> Changes to v1:
> 
> - fix reference to Gnu MP as outlined by Ard Biesheuvel
> - addition of patches 4 and 5
> 
> Marcelo Henrique Cerri (1):
>   lib/mpi: Add mpi_sub_ui()
> 
> Stephan Mueller (4):
>   crypto: ECDH - check validity of Z before export
>   crypto: DH - check validity of Z before export
>   crypto: DH SP800-56A rev 3 local public key validation
>   crypto: ECDH SP800-56A rev 3 local public key validation
> 
>  crypto/dh.c          | 38 +++++++++++++++++++++
>  crypto/ecc.c         | 42 +++++++++++++++++++++---
>  crypto/ecc.h         | 14 ++++++++
>  include/linux/mpi.h  |  3 ++
>  lib/mpi/Makefile     |  1 +
>  lib/mpi/mpi-sub-ui.c | 78 ++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 172 insertions(+), 4 deletions(-)
>  create mode 100644 lib/mpi/mpi-sub-ui.c

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2020-07-31 13:30 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-10 10:09 [PATCH 0/3] DH: SP800-56A rev 3 compliant shared secret Stephan Müller
2020-07-10 10:10 ` [PATCH 1/3] crypto: ECDH - check validity of Z before export Stephan Müller
2020-07-10 10:10 ` [PATCH 2/3] lib/mpi: Add mpi_sub_ui() Stephan Müller
2020-07-10 14:42   ` Ard Biesheuvel
2020-07-10 15:10     ` Stephan Mueller
2020-07-10 10:15 ` [PATCH 3/3] crypto: DH - check validity of Z before export Stephan Müller
2020-07-12 16:38 ` [PATCH v2 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
2020-07-12 16:39   ` [PATCH v2 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
2020-07-12 18:02     ` Vitaly Chikunov
2020-07-15 13:17     ` Marcelo Henrique Cerri
2020-07-12 16:39   ` [PATCH v2 2/5] lib/mpi: Add mpi_sub_ui() Stephan Müller
2020-07-16  7:30     ` Herbert Xu
2020-07-16  8:41       ` Ard Biesheuvel
2020-07-16 12:50         ` Marcelo Henrique Cerri
2020-07-16 13:09           ` Ard Biesheuvel
2020-07-16 13:41             ` Marcelo Henrique Cerri
2020-07-16 13:53               ` Ard Biesheuvel
2020-07-16 14:23                 ` Marcelo Henrique Cerri
2020-07-16 14:37                   ` Ard Biesheuvel
2020-07-16 14:56                     ` Marcelo Henrique Cerri
2020-07-16 15:20                       ` Ard Biesheuvel
2020-07-12 16:40   ` [PATCH v2 3/5] crypto: DH - check validity of Z before export Stephan Müller
2020-07-15 13:17     ` Marcelo Henrique Cerri
2020-07-12 16:40   ` [PATCH v2 4/5] crypto: DH SP800-56A rev 3 local public key validation Stephan Müller
2020-07-15 13:18     ` Marcelo Henrique Cerri
2020-07-12 16:42   ` [PATCH v2 5/5] crypto: ECDH " Stephan Müller
2020-07-12 18:06     ` Vitaly Chikunov
2020-07-13  5:04       ` Stephan Mueller
2020-07-13  5:59         ` Vitaly Chikunov
2020-07-13  6:02           ` Stephan Müller
2020-07-15 13:19     ` Marcelo Henrique Cerri
2020-07-20 17:05   ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Stephan Müller
2020-07-20 17:07     ` [PATCH v3 1/5] crypto: ECDH - check validity of Z before export Stephan Müller
2020-07-22 13:11       ` Vitaly Chikunov
2020-07-24 17:47       ` Neil Horman
2020-07-20 17:08     ` [PATCH v3 2/5] lib/mpi: Add mpi_sub_ui() Stephan Müller
2020-07-20 17:08     ` [PATCH v3 3/5] crypto: DH - check validity of Z before export Stephan Müller
2020-07-24 18:02       ` Neil Horman
2020-07-20 17:08     ` [PATCH v3 4/5] crypto: DH SP800-56A rev 3 local public key validation Stephan Müller
2020-07-20 17:09     ` [PATCH v3 5/5] crypto: ECDH " Stephan Müller
2020-07-21 11:35     ` [PATCH v3 0/5] DH: SP800-56A rev 3 compliant validation checks Marcelo Henrique Cerri
2020-07-31 13:29     ` Herbert Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).