cryptodev: add support for 25519 and 448 curves

Message ID 20220407134334.20226-1-arkadiuszx.kusztal@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series cryptodev: add support for 25519 and 448 curves |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS

Commit Message

Arkadiusz Kusztal April 7, 2022, 1:43 p.m. UTC
  This commit adds support for following elliptic curves:
1) Curve25519
2) Curve448

Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
---
 lib/cryptodev/rte_crypto_asym.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
  

Comments

Akhil Goyal May 16, 2022, 6:57 p.m. UTC | #1
> This commit adds support for following elliptic curves:
> 1) Curve25519
> 2) Curve448
> 
> Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> ---
>  lib/cryptodev/rte_crypto_asym.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
> index cd24d4b07b..775b2f6277 100644
> --- a/lib/cryptodev/rte_crypto_asym.h
> +++ b/lib/cryptodev/rte_crypto_asym.h
> @@ -48,6 +48,8 @@ enum rte_crypto_ec_group {
>  	RTE_CRYPTO_EC_GROUP_SECP256R1 = 23,
>  	RTE_CRYPTO_EC_GROUP_SECP384R1 = 24,
>  	RTE_CRYPTO_EC_GROUP_SECP521R1 = 25,
> +	RTE_CRYPTO_EC_GROUP_CURVE25519 = 29,
> +	RTE_CRYPTO_EC_GROUP_CURVE448 = 30,
>  };
> 
>  /**
> @@ -180,9 +182,17 @@ typedef rte_crypto_param rte_crypto_uint;
>   */
>  struct rte_crypto_ec_point {
>  	rte_crypto_param x;
> -	/**< X coordinate */
> +	/**<
> +	 * X coordinate
> +	 * For curve25519 and curve448 - little-endian integer
> +	 * otherwise, big-endian integer
> +	 */
>  	rte_crypto_param y;
> -	/**< Y coordinate */
> +	/**<
> +	 * Y coordinate
> +	 * For curve25519 and curve448 - little-endian integer
> +	 * otherwise, big-endian integer
> +	 */
Can you give reference of the document which specify this endianness?

And if it is implicit as per the protocol, do we need to add explicit comments here?
  
Arkadiusz Kusztal May 17, 2022, 11:45 a.m. UTC | #2
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Monday, May 16, 2022 8:58 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [EXT] [PATCH] cryptodev: add support for 25519 and 448 curves
> 
> > This commit adds support for following elliptic curves:
> > 1) Curve25519
> > 2) Curve448
> >
> > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > ---
> >  lib/cryptodev/rte_crypto_asym.h | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/cryptodev/rte_crypto_asym.h
> > b/lib/cryptodev/rte_crypto_asym.h index cd24d4b07b..775b2f6277 100644
> > --- a/lib/cryptodev/rte_crypto_asym.h
> > +++ b/lib/cryptodev/rte_crypto_asym.h
> > @@ -48,6 +48,8 @@ enum rte_crypto_ec_group {
> >  	RTE_CRYPTO_EC_GROUP_SECP256R1 = 23,
> >  	RTE_CRYPTO_EC_GROUP_SECP384R1 = 24,
> >  	RTE_CRYPTO_EC_GROUP_SECP521R1 = 25,
> > +	RTE_CRYPTO_EC_GROUP_CURVE25519 = 29,
> > +	RTE_CRYPTO_EC_GROUP_CURVE448 = 30,
> >  };
> >
> >  /**
> > @@ -180,9 +182,17 @@ typedef rte_crypto_param rte_crypto_uint;
> >   */
> >  struct rte_crypto_ec_point {
> >  	rte_crypto_param x;
> > -	/**< X coordinate */
> > +	/**<
> > +	 * X coordinate
> > +	 * For curve25519 and curve448 - little-endian integer
> > +	 * otherwise, big-endian integer
> > +	 */
> >  	rte_crypto_param y;
> > -	/**< Y coordinate */
> > +	/**<
> > +	 * Y coordinate
> > +	 * For curve25519 and curve448 - little-endian integer
> > +	 * otherwise, big-endian integer
> > +	 */
> Can you give reference of the document which specify this endianness?
[Arek] - sure, I may give rfc reference here, but if it will go into crypodev in this form I am not yet sure.
These curves could be used with DH, but cannot be used with ECDSA. Even with DH it may be that we will go with separate {dh_op, ecdh_op, x25519_op, x448_op} but this would make TLS group reference pointless, and we would not add Montgomery/Edwards curves at all as an enum.
> 
> And if it is implicit as per the protocol, do we need to add explicit comments
> here?
  
Akhil Goyal May 26, 2022, 4:38 p.m. UTC | #3
> > > This commit adds support for following elliptic curves:
> > > 1) Curve25519
> > > 2) Curve448
> > >
> > > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > > ---
Acked-by: Akhil Goyal <gakhil@marvell.com>

Applied to dpdk-next-crypto
  
Arkadiusz Kusztal May 31, 2022, 2:33 p.m. UTC | #4
Hi Akhil,

Sorry I have missed that, and I think we should revert this patch.
It would make sense to have TLS derived numbers for these curves if DH and ECDH would be in the same op.
But since we decided to split it we are going to go with separate structs for x448 and x25519 as per:
https://patchwork.dpdk.org/project/dpdk/patch/20220531040439.15862-7-arkadiuszx.kusztal@intel.com/


> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, May 26, 2022 6:38 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [EXT] [PATCH] cryptodev: add support for 25519 and 448 curves
> 
> > > > This commit adds support for following elliptic curves:
> > > > 1) Curve25519
> > > > 2) Curve448
> > > >
> > > > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > > > ---
> Acked-by: Akhil Goyal <gakhil@marvell.com>
> 
> Applied to dpdk-next-crypto
  
Akhil Goyal May 31, 2022, 2:40 p.m. UTC | #5
Hi Arek,
> Hi Akhil,
> 
> Sorry I have missed that, and I think we should revert this patch.
> It would make sense to have TLS derived numbers for these curves if DH and
> ECDH would be in the same op.
> But since we decided to split it we are going to go with separate structs for x448
> and x25519 as per:
> https://patchwork.dpdk.org/project/dpdk/patch/20220531040439.15862-7-arkadiuszx.kusztal@intel.com/

I have asked you previously also to update patchworks with superseded
When you are sending new versions or if the patch is not valid.
I will remove this patch from the tree, since the patch is not pulled in main.
But please take care in future.
  
Arkadiusz Kusztal May 31, 2022, 2:42 p.m. UTC | #6
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, May 31, 2022 4:40 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [EXT] [PATCH] cryptodev: add support for 25519 and 448 curves
> 
> Hi Arek,
> > Hi Akhil,
> >
> > Sorry I have missed that, and I think we should revert this patch.
> > It would make sense to have TLS derived numbers for these curves if DH
> > and ECDH would be in the same op.
> > But since we decided to split it we are going to go with separate
> > structs for x448 and x25519 as per:
> > https://patchwork.dpdk.org/project/dpdk/patch/20220531040439.15862-7-a
> > rkadiuszx.kusztal@intel.com/
> 
> I have asked you previously also to update patchworks with superseded When
> you are sending new versions or if the patch is not valid.
> I will remove this patch from the tree, since the patch is not pulled in main.
> But please take care in future.
> 
Thank you!
  
Akhil Goyal May 31, 2022, 3:25 p.m. UTC | #7
> -----Original Message-----
> From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Sent: Tuesday, May 31, 2022 8:03 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org
> Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [EXT] [PATCH] cryptodev: add support for 25519 and 448 curves
> 
> Hi Akhil,
> 
> Sorry I have missed that, and I think we should revert this patch.
> It would make sense to have TLS derived numbers for these curves if DH and
> ECDH would be in the same op.
> But since we decided to split it we are going to go with separate structs for x448
> and x25519 as per:

Even after removing this patch, the patches are not getting applied.
Applying: cryptodev: move RSA padding into separate struct
error: patch failed: drivers/crypto/qat/qat_asym.c:345
error: drivers/crypto/qat/qat_asym.c: patch does not apply
Patch failed at 0010 cryptodev: move RSA padding into separate struct
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
  
Arkadiusz Kusztal May 31, 2022, 3:39 p.m. UTC | #8
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, May 31, 2022 5:26 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [EXT] [PATCH] cryptodev: add support for 25519 and 448 curves
> 
> 
> 
> > -----Original Message-----
> > From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> > Sent: Tuesday, May 31, 2022 8:03 PM
> > To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org
> > Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> > Subject: RE: [EXT] [PATCH] cryptodev: add support for 25519 and 448
> > curves
> >
> > Hi Akhil,
> >
> > Sorry I have missed that, and I think we should revert this patch.
> > It would make sense to have TLS derived numbers for these curves if DH
> > and ECDH would be in the same op.
> > But since we decided to split it we are going to go with separate
> > structs for x448 and x25519 as per:
> 
> Even after removing this patch, the patches are not getting applied.
> Applying: cryptodev: move RSA padding into separate struct
> error: patch failed: drivers/crypto/qat/qat_asym.c:345
> error: drivers/crypto/qat/qat_asym.c: patch does not apply Patch failed at 0010
> cryptodev: move RSA padding into separate struct
> hint: Use 'git am --show-current-patch' to see the failed patch When you have
> resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".

Yes, it needs to be rebased against:
crypto/qat: refactor asym algorithm macros and logs
I will do it in v5.
  

Patch

diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
index cd24d4b07b..775b2f6277 100644
--- a/lib/cryptodev/rte_crypto_asym.h
+++ b/lib/cryptodev/rte_crypto_asym.h
@@ -48,6 +48,8 @@  enum rte_crypto_ec_group {
 	RTE_CRYPTO_EC_GROUP_SECP256R1 = 23,
 	RTE_CRYPTO_EC_GROUP_SECP384R1 = 24,
 	RTE_CRYPTO_EC_GROUP_SECP521R1 = 25,
+	RTE_CRYPTO_EC_GROUP_CURVE25519 = 29,
+	RTE_CRYPTO_EC_GROUP_CURVE448 = 30,
 };
 
 /**
@@ -180,9 +182,17 @@  typedef rte_crypto_param rte_crypto_uint;
  */
 struct rte_crypto_ec_point {
 	rte_crypto_param x;
-	/**< X coordinate */
+	/**<
+	 * X coordinate
+	 * For curve25519 and curve448 - little-endian integer
+	 * otherwise, big-endian integer
+	 */
 	rte_crypto_param y;
-	/**< Y coordinate */
+	/**<
+	 * Y coordinate
+	 * For curve25519 and curve448 - little-endian integer
+	 * otherwise, big-endian integer
+	 */
 };
 
 /**