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
From 5385865b3f44d331f91c6786a2e7f4e2fb4d8cb2 Mon Sep 17 00:00:00 2001
From: Stephan Mueller <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Stephan Mueller <[email protected]>
---
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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Stephan Mueller <[email protected]>
---
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
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 <[email protected]>
> zeroized.
>
> Signed-off-by: Stephan Mueller <[email protected]>
> ---
> 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
>
>
>
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 <[email protected]>
> ---
> 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
>
>
>
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 <[email protected]>
> > ---
> >
> > 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
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 <[email protected]>
> > > ---
> > >
> > > 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
>
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
Reviewed-by: Marcelo Henrique Cerri <[email protected]>
Tested-by: Marcelo Henrique Cerri <[email protected]>
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 <[email protected]>
> ---
> 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
Reviewed-by: Marcelo Henrique Cerri <[email protected]>
Tested-by: Marcelo Henrique Cerri <[email protected]>
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 <[email protected]>
> ---
> 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
Reviewed-by: Marcelo Henrique Cerri <[email protected]>
Tested-by: Marcelo Henrique Cerri <[email protected]>
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 <[email protected]>
> ---
> 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
Reviewed-by: Marcelo Henrique Cerri <[email protected]>
Tested-by: Marcelo Henrique Cerri <[email protected]>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, 16 Jul 2020 at 10:30, Herbert Xu <[email protected]> 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.
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 <[email protected]> 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
On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
<[email protected]> 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 <[email protected]> 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
>
On Thu, Jul 16, 2020 at 04:09:39PM +0300, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 15:50, Marcelo Henrique Cerri
> <[email protected]> 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 <[email protected]> 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
On Thu, 16 Jul 2020 at 16:41, Marcelo Henrique Cerri
<[email protected]> 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
> > <[email protected]> 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 <[email protected]> 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
>
On Thu, Jul 16, 2020 at 04:53:24PM +0300, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 16:41, Marcelo Henrique Cerri
> <[email protected]> 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
> > > <[email protected]> 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 <[email protected]> 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
On Thu, 16 Jul 2020 at 17:23, Marcelo Henrique Cerri
<[email protected]> 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
> > <[email protected]> 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
> > > > <[email protected]> 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 <[email protected]> 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
>
On Thu, Jul 16, 2020 at 05:37:32PM +0300, Ard Biesheuvel wrote:
> On Thu, 16 Jul 2020 at 17:23, Marcelo Henrique Cerri
> <[email protected]> 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
> > > <[email protected]> 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
> > > > > <[email protected]> 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 <[email protected]> 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
On Thu, 16 Jul 2020 at 17:56, Marcelo Henrique Cerri
<[email protected]> 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
> > <[email protected]> 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
> > > > <[email protected]> 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
> > > > > > <[email protected]> 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 <[email protected]> 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
>
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
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 <[email protected]>
---
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
From: Marcelo Henrique Cerri <[email protected]>
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 <[email protected]>
Signed-off-by: Stephan Mueller <[email protected]>
---
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
Reviewed-by: Marcelo Henrique Cerri <[email protected]>
Tested-by: Marcelo Henrique Cerri <[email protected]>
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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt