[v2,2/6] security: add support for DOCSIS protocol

  Add support for DOCSIS protocol to rte_security library. This support
currently comprises the combination of Crypto and CRC operations.

A security operation definition is also added. This allow security
protocol related parameters be specified at the operation level. For
DOCSIS, these parameters include CRC length and offset. The security
operation is accessed via a crypto operation.

Signed-off-by: David Coyle <david.coyle@intel.com>
Signed-off-by: Mairtin o Loingsigh <mairtin.oloingsigh@intel.com>
---
 lib/librte_security/rte_security.c |   7 ++
 lib/librte_security/rte_security.h | 116 ++++++++++++++++++++++++++++-
 2 files changed, 120 insertions(+), 3 deletions(-)
  

> -----Original Message-----
> From: Coyle, David <david.coyle@intel.com>
> Sent: Tuesday, June 23, 2020 11:14 AM
> To: akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>; De
> Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Ryan, Brendan <brendan.ryan@intel.com>;
> hemant.agrawal@nxp.com; anoobj@marvell.com; ruifeng.wang@arm.com;
> lironh@marvell.com; rnagadheeraj@marvell.com; jsrikanth@marvell.com;
> G.Singh@nxp.com; jianjay.zhou@huawei.com; ravi1.kumar@amd.com;
> Richardson, Bruce <bruce.richardson@intel.com>; olivier.matz@6wind.com;
> honnappa.nagarahalli@arm.com; stephen@networkplumber.org;
> alexr@mellanox.com; jerinj@marvell.com; Coyle, David
> <david.coyle@intel.com>; O'loingsigh, Mairtin <mairtin.oloingsigh@intel.com>
> Subject: [PATCH v2 2/6] security: add support for DOCSIS protocol
> 
> Add support for DOCSIS protocol to rte_security library. This support currently
> comprises the combination of Crypto and CRC operations.
> 
> A security operation definition is also added. This allow security protocol related
> parameters be specified at the operation level. For DOCSIS, these parameters
> include CRC length and offset. The security operation is accessed via a crypto
> operation.
> 
> Signed-off-by: David Coyle <david.coyle@intel.com>
> Signed-off-by: Mairtin o Loingsigh <mairtin.oloingsigh@intel.com>

...

> +++ b/lib/librte_security/rte_security.h
> @@ -293,6 +293,30 @@ struct rte_security_pdcp_xform {
>  	uint32_t hfn_ovrd;
>  };
> 
> +/** DOCSIS direction */
> +enum rte_security_docsis_direction {
> +	RTE_SECURITY_DOCSIS_UPLINK,
> +	/**< Uplink
> +	 * - Decryption, followed by CRC Verification
> +	 */
> +	RTE_SECURITY_DOCSIS_DOWNLINK,
> +	/**< Downlink
> +	 * - CRC Generation, followed by Encryption
> +	 */
> +};
> +
> +/**
> + * DOCSIS security session configuration.
> + *
> + * This structure contains data required to create a DOCSIS security session.
> + */
> +struct rte_security_docsis_xform {
> +	enum rte_security_docsis_direction direction;
> +	/** DOCSIS direction */

Missing "<" here.
  

Hi David,

> +/** DOCSIS direction */
> +enum rte_security_docsis_direction {
> +	RTE_SECURITY_DOCSIS_UPLINK,
> +	/**< Uplink
> +	 * - Decryption, followed by CRC Verification
> +	 */
> +	RTE_SECURITY_DOCSIS_DOWNLINK,
> +	/**< Downlink
> +	 * - CRC Generation, followed by Encryption
> +	 */
> +};
> +
> +/**
> + * DOCSIS security session configuration.
> + *
> + * This structure contains data required to create a DOCSIS security session.
> + */
> +struct rte_security_docsis_xform {
> +	enum rte_security_docsis_direction direction;
> +	/** DOCSIS direction */
> +	uint16_t crc_size;
> +	/**< CRC size in bytes */
> +};
> +
>  /**
>   * Security session action type.
>   */
> @@ -325,6 +349,8 @@ enum rte_security_session_protocol {
>  	/**< MACSec Protocol */
>  	RTE_SECURITY_PROTOCOL_PDCP,
>  	/**< PDCP Protocol */
> +	RTE_SECURITY_PROTOCOL_DOCSIS,
> +	/**< DOCSIS Protocol */
>  };
> 
>  /**
> @@ -340,6 +366,7 @@ struct rte_security_session_conf {
>  		struct rte_security_ipsec_xform ipsec;
>  		struct rte_security_macsec_xform macsec;
>  		struct rte_security_pdcp_xform pdcp;
> +		struct rte_security_docsis_xform docsis;
>  	};
>  	/**< Configuration parameters for security session */
>  	struct rte_crypto_sym_xform *crypto_xform;
> @@ -355,6 +382,63 @@ struct rte_security_session {
>  	/**< Opaque user defined data */
>  };
> 
> +/**
> + * DOCSIS operation parameters
> + */
> +struct rte_security_docsis_op {
> +	struct rte_crypto_sym_op crypto_sym;
> +	/**< Symmetric crypto operation parameters */
> +
> +	struct {
> +		uint16_t offset;
> +		/**<
> +		 * Starting point for CRC processing, specified
> +		 * as the number of bytes from start of the packet in
> +		 * the source mbuf in crypto_sym
> +		 */
> +		uint16_t length;
> +		/**<
> +		 * The length, in bytes, of the source mbuf on which the
> +		 * CRC will be computed
> +		 */
> +	} crc;
> +	/**< CRC operation parameters */

As per my understanding, CRC is a kind of authentication. Can we reuse the fields of rte_crypto_sym_op
Auth.data.offset and auth.data.length. This way you can save the unnecessary 4 bytes here. Probably add
Comment in the structure definition that it can be used as offset and length for CRC.

And if you feel that reserved field is needed in near future, then you can add a proper name to it or else
You can do away with the rte_security_docsis_op itself as there will be no other fields in it.

> +
> +	uint64_t reserved;
> +	/**< Reserved for future use */
> +};
> +
> +/**
> + * Security operation types
> + */
> +enum rte_security_op_type {
> +	RTE_SECURITY_OP_TYPE_DOCSIS = 1
> +	/**< DOCSIS operation */
> +};
> +
> +/**
> + * Security operation parameters
> + *
> + * @note If the size of this struct changes, it may be also necessary to update
> + * the RTE_CRYPTO_OP_SECURITY_MAX_SZ define
> + */
> +struct rte_security_op {
> +	enum rte_security_op_type type;
> +	/**< Type of operation */
> +	RTE_STD_C11
> +	union {
> +		struct rte_security_docsis_op docsis;
> +	};
> +	/**< Parameters for security operation */
> +};
> +
> +/* Macro to check the size of a struct at compile time */
> +#define _SECURITY_STRUCT_LEN_CHECK(n, X) enum
> security_static_assert_enum_##X \
> +	{ security_static_assert_##X = (n)/((sizeof(struct X) <= (n)) ? 1 : 0) }
> +
> +/* Check the size of the rte_security_op struct */
> +_SECURITY_STRUCT_LEN_CHECK(RTE_CRYPTO_OP_SECURITY_MAX_SZ,
> rte_security_op);
> +
>  /**
>   * Create security session as specified by the session configuration
>   *
> @@ -496,12 +580,22 @@ static inline int
>  rte_security_attach_session(struct rte_crypto_op *op,
>  			    struct rte_security_session *sess)
>  {
> -	if (unlikely(op->type != RTE_CRYPTO_OP_TYPE_SYMMETRIC))
> -		return -EINVAL;
> +	struct rte_security_op *s_op;
> +	int ret = -EINVAL;
> +
> +	if (likely(op->type == RTE_CRYPTO_OP_TYPE_SYMMETRIC)) {
> +		ret = __rte_security_attach_session(op->sym, sess);
> +	} else if (op->type == RTE_CRYPTO_OP_TYPE_SECURITY) {
> +		s_op = (struct rte_security_op *)&op->security;
> +		if (s_op->type == RTE_SECURITY_OP_TYPE_DOCSIS)
> +			ret = __rte_security_attach_session(
> +						&s_op->docsis.crypto_sym,
> +						sess);
> +	}
> 
>  	op->sess_type =  RTE_CRYPTO_OP_SECURITY_SESSION;
> 
> -	return __rte_security_attach_session(op->sym, sess);
> +	return ret;
>  }
> 
>  struct rte_security_macsec_stats {
> @@ -523,6 +617,10 @@ struct rte_security_pdcp_stats {
>  	uint64_t reserved;
>  };
> 
> +struct rte_security_docsis_stats {
> +	uint64_t reserved;
> +};
> +
>  struct rte_security_stats {
>  	enum rte_security_session_protocol protocol;
>  	/**< Security protocol to be configured */
> @@ -532,6 +630,7 @@ struct rte_security_stats {
>  		struct rte_security_macsec_stats macsec;
>  		struct rte_security_ipsec_stats ipsec;
>  		struct rte_security_pdcp_stats pdcp;
> +		struct rte_security_docsis_stats docsis;
>  	};
>  };
> 
> @@ -591,6 +690,13 @@ struct rte_security_capability {
>  			/**< Capability flags, see RTE_SECURITY_PDCP_* */
>  		} pdcp;
>  		/**< PDCP capability */
> +		struct {
> +			enum rte_security_docsis_direction direction;
> +			/**< DOCSIS direction */
> +			uint16_t crc_size;
> +			/**< CRC size in bytes */
> +		} docsis;
> +		/**< DOCSIS capability */
>  	};
> 
>  	const struct rte_cryptodev_capabilities *crypto_capabilities;
> @@ -649,6 +755,10 @@ struct rte_security_capability_idx {
>  			enum rte_security_pdcp_domain domain;
>  			uint32_t capa_flags;
>  		} pdcp;
> +		struct {
> +			enum rte_security_docsis_direction direction;
> +			uint16_t crc_size;
> +		} docsis;
>  	};
>  };
> 
> --
> 2.17.1
  

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Tuesday, June 23, 2020 7:07 PM
> >
> > +/**
> > + * DOCSIS operation parameters
> > + */
> > +struct rte_security_docsis_op {
> > +	struct rte_crypto_sym_op crypto_sym;
> > +	/**< Symmetric crypto operation parameters */
> > +
> > +	struct {
> > +		uint16_t offset;
> > +		/**<
> > +		 * Starting point for CRC processing, specified
> > +		 * as the number of bytes from start of the packet in
> > +		 * the source mbuf in crypto_sym
> > +		 */
> > +		uint16_t length;
> > +		/**<
> > +		 * The length, in bytes, of the source mbuf on which the
> > +		 * CRC will be computed
> > +		 */
> > +	} crc;
> > +	/**< CRC operation parameters */
> 
> As per my understanding, CRC is a kind of authentication. Can we reuse the
> fields of rte_crypto_sym_op Auth.data.offset and auth.data.length. This way
> you can save the unnecessary 4 bytes here. Probably add Comment in the
> structure definition that it can be used as offset and length for CRC.
> 
> And if you feel that reserved field is needed in near future, then you can add
> a proper name to it or else You can do away with the rte_security_docsis_op
> itself as there will be no other fields in it.

[DC] As per my reply on the v1 patchset, I am happy to use the auth offset and
length fields for CRC if there are no objections from others on this approach.

Strictly speaking, a CRC is not an authentication algorithm like the other auth
algos in cryptodev - if it were we would have just added CRC as a new auth algo.
However, using the auth offset and length fields of the crypto op does simplify
things, removes unnecessary bytes and levels of indirection. It also means the
'uint8_t security[0]' field can be removed from rte_crypto_op.

The 'reserved' field was to accommodate other DOCSIS protocol features which
could be offloaded in the future - such as the DOCSIS header checksum. For this
feature, we would need to know the DOCSIS header length. The header length
though is equal to the CRC offset, so we can get the header length that way.
The reserved field and the entire rte_security_docsis_op can therefore be
removed
  

Hi Pablo

> -----Original Message-----
> From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > +/**
> > + * DOCSIS security session configuration.
> > + *
> > + * This structure contains data required to create a DOCSIS security
> session.
> > + */
> > +struct rte_security_docsis_xform {
> > +	enum rte_security_docsis_direction direction;
> > +	/** DOCSIS direction */
> 
> Missing "<" here.

[DC] Very good spot... will be fixed in v3
>