[dpdk-dev] [PATCH 01/11] lib/rte_security: add security library

[Hemant Agrawal]

Hi,

On 9/14/2017 1:56 PM, Akhil Goyal wrote:
<snip>..

> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> new file mode 100644
> index 0000000..5776246
> --- /dev/null
> +++ b/lib/librte_security/rte_security.c
> @@ -0,0 +1,252 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright 2017 NXP.
> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of NXP nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <rte_malloc.h>
> +#include <rte_dev.h>
> +
> +#include "rte_security.h"
> +#include "rte_security_driver.h"
> +
> +#define RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ	(8)
> +
> +struct rte_security_ctx {
> +	uint16_t id;
> +	enum {
> +		RTE_SECURITY_INSTANCE_INVALID = 0,
> +		RTE_SECURITY_INSTANCE_VALID
> +	} state;
> +	void *device;
> +	struct rte_security_ops *ops;
> +};
> +
> +static struct rte_security_ctx *security_instances;
> +static uint16_t max_nb_security_instances;
> +static uint16_t nb_security_instances;
> +
> +static int
> +rte_security_is_valid_id(uint16_t id)
> +{
> +	if (id >= nb_security_instances ||
> +	    (security_instances[id].state != RTE_SECURITY_INSTANCE_VALID))
> +		return 0;
> +	else
> +		return 1;
> +}
> +
> +/* Macros to check for valid id */
> +#define RTE_SEC_VALID_ID_OR_ERR_RET(id, retval) do { \
> +	if (!rte_security_is_valid_id(id)) { \
> +		RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
> +		return retval; \
> +	} \
> +} while (0)
> +
> +#define RTE_SEC_VALID_ID_OR_RET(id) do { \
> +	if (!rte_security_is_valid_id(id)) { \
> +		RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
> +		return; \
> +	} \
> +} while (0)
> +
> +int
> +rte_security_register(uint16_t *id, void *device,
> +		      struct rte_security_ops *ops)
> +{
> +	if (max_nb_security_instances == 0) {
> +		security_instances = rte_malloc(
> +				"rte_security_instances_ops",
> +				sizeof(*security_instances) *
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ, 0);
> +
> +		if (security_instances == NULL)
> +			return -ENOMEM;
> +		max_nb_security_instances =
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> +	} else if (nb_security_instances >= max_nb_security_instances) {
> +		uint16_t *instances = rte_realloc(security_instances,
> +				sizeof(struct rte_security_ops *) *
> +				(max_nb_security_instances +
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ), 0);

I think "RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ" value as 8 is relatively 
small. you may want to keep it "64" or more.

you may change it into two parts
- Initial block size and incremental block size for realloc.

Also, do you want to make it a configurable variable. as some 
implementation may need really large number of SAs.

> +
> +		if (instances == NULL)
> +			return -ENOMEM;
> +
> +		max_nb_security_instances +=
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> +	}
> +
> +	*id = nb_security_instances++;
> +
> +	security_instances[*id].id = *id;
> +	security_instances[*id].state = RTE_SECURITY_INSTANCE_VALID;
> +	security_instances[*id].device = device;
> +	security_instances[*id].ops = ops;
> +
> +	return 0;
> +}
> +
> +int
> +rte_security_unregister(__rte_unused uint16_t *id)
> +{
> +	/* To be implemented */
> +	return 0;
> +}
> +
> +struct rte_security_session *
> +rte_security_session_create(uint16_t id,
> +			    struct rte_security_session_conf *conf,
> +			    struct rte_mempool *mp)
> +{
> +	struct rte_security_ctx *instance;
> +	struct rte_security_session *sess = NULL;
> +
> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
> +	instance = &security_instances[id];
> +
> +	if (conf == NULL)
> +		return NULL;
> +
> +	if (rte_mempool_get(mp, (void *)&sess))
> +		return NULL;
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);

it will leak the sess memory, if returned on error.

> +	if (instance->ops->session_create(instance->device, conf, sess, mp)) {
> +		rte_mempool_put(mp, (void *)sess);
> +		return NULL;
> +	}

can the mempool operations be part of session_create api?

it will be similar to destroy, which is expected to free the 'sess' 
object to mempool?

> +	return sess;
> +}
> +

<snip>..

> diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
> new file mode 100644
> index 0000000..2faac96
> --- /dev/null
> +++ b/lib/librte_security/rte_security.h
> @@ -0,0 +1,494 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright 2017 NXP.
> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of NXP nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTE_SECURITY_H_
> +#define _RTE_SECURITY_H_
> +
> +/**
> + * @file rte_security.h
> + *
> + * RTE Security Common Definitions
> + *
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
> +#include <netinet/ip6.h>
> +
> +#include <rte_mbuf.h>
> +#include <rte_memory.h>
> +#include <rte_mempool.h>
> +#include <rte_common.h>
> +#include <rte_crypto.h>
> +
> +/** IPSec protocol mode */
> +enum rte_security_ipsec_sa_mode {
> +	RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
> +	/**< IPSec Transport mode */
> +	RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
> +	/**< IPSec Tunnel mode */
> +};
> +
> +/** IPSec Protocol */
> +enum rte_security_ipsec_sa_protocol {
> +	RTE_SECURITY_IPSEC_SA_PROTO_AH,
> +	/**< AH protocol */
> +	RTE_SECURITY_IPSEC_SA_PROTO_ESP,
> +	/**< ESP protocol */
> +};
> +
> +/** IPSEC tunnel type */
> +enum rte_security_ipsec_tunnel_type {
> +	RTE_SECURITY_IPSEC_TUNNEL_IPV4 = 0,
> +	/**< Outer header is IPv4 */
> +	RTE_SECURITY_IPSEC_TUNNEL_IPV6,
> +	/**< Outer header is IPv6 */
> +};
> +
> +/**
> + * IPSEC tunnel parameters
> + *
> + * These parameters are used to build outbound tunnel headers.
> + */
> +struct rte_security_ipsec_tunnel_param {
> +	enum rte_security_ipsec_tunnel_type type;
> +	/**< Tunnel type: IPv4 or IPv6 */
> +	union {
> +		struct {
> +			struct in_addr src_ip;
> +			/**< IPv4 source address */
> +			struct in_addr dst_ip;
> +			/**< IPv4 destination address */
> +			uint8_t dscp;
> +			/**< IPv4 Differentiated Services Code Point */
> +			uint8_t df;
> +			/**< IPv4 Don't Fragment bit */
> +			uint8_t ttl;
> +			/**< IPv4 Time To Live */
> +		} ipv4;
> +		/**< IPv4 header parameters */
> +		struct {
> +			struct in6_addr src_addr;
> +			/**< IPv6 source address */
> +			struct in6_addr dst_addr;
> +			/**< IPv6 destination address */
> +			uint8_t dscp;
> +			/**< IPv6 Differentiated Services Code Point */
> +			uint32_t flabel;
> +			/**< IPv6 flow label */
> +			uint8_t hlimit;
> +			/**< IPv6 hop limit */
> +		} ipv6;
> +		/**< IPv6 header parameters */
> +	};
> +};
> +
> +/**
> + * IPsec Security Association option flags
> + */
> +struct rte_security_ipsec_sa_options {
> +	/** Extended Sequence Numbers (ESN)
> +	  *
> +	  * * 1: Use extended (64 bit) sequence numbers
> +	  * * 0: Use normal sequence numbers
> +	  */
> +	uint32_t esn : 1;
> +
> +	/** UDP encapsulation
> +	  *
> +	  * * 1: Do UDP encapsulation/decapsulation so that IPSEC packets can
> +	  *      traverse through NAT boxes.
> +	  * * 0: No UDP encapsulation
> +	  */
> +	uint32_t udp_encap : 1;
> +
> +	/** Copy DSCP bits
> +	  *
> +	  * * 1: Copy IPv4 or IPv6 DSCP bits from inner IP header to
> +	  *      the outer IP header in encapsulation, and vice versa in
> +	  *      decapsulation.
> +	  * * 0: Use values from odp_ipsec_tunnel_param_t in encapsulation and
> +	  *      do not change DSCP field in decapsulation.
> +	  */
> +	uint32_t copy_dscp : 1;
> +
> +	/** Copy IPv6 Flow Label
> +	  *
> +	  * * 1: Copy IPv6 flow label from inner IPv6 header to the
> +	  *      outer IPv6 header.
> +	  * * 0: Use value from odp_ipsec_tunnel_param_t
> +	  */
> +	uint32_t copy_flabel : 1;
> +
> +	/** Copy IPv4 Don't Fragment bit
> +	  *
> +	  * * 1: Copy the DF bit from the inner IPv4 header to the outer
> +	  *      IPv4 header.
> +	  * * 0: Use value from odp_ipsec_tunnel_param_t
> +	  */
> +	uint32_t copy_df : 1;
> +
> +	/** Decrement inner packet Time To Live (TTL) field
> +	  *
> +	  * * 1: In tunnel mode, decrement inner packet IPv4 TTL or
> +	  *      IPv6 Hop Limit after tunnel decapsulation, or before tunnel
> +	  *      encapsulation.
> +	  * * 0: Inner packet is not modified.
> +	  */
> +	uint32_t dec_ttl : 1;
> +
> +	/** HW constructs/removes trailer of packets
> +	  *
> +	  * * 1: Transmitted packets will have the trailer added to them by
> +	  *      hardawre. The next protocol field will be based on the
> +	  *      mbuf->inner_esp_next_proto field.
> +	  *      Received packets have no trailer, the next protocol field is
> +	  *      supplied in the mbuf->inner_esp_next_proto field.
> +	  * * 0: Inner packet is not modified.
> +	  */
> +	uint32_t no_trailer : 1;
> +};
> +
> +/** IPSec security association direction */
> +enum rte_security_ipsec_sa_direction {
> +	RTE_SECURITY_IPSEC_SA_DIR_EGRESS,
> +	/**< Encrypt and generate digest */
> +	RTE_SECURITY_IPSEC_SA_DIR_INGRESS,
> +	/**< Verify digest and decrypt */
> +};
> +
> +/**
> + * IPsec security association configuration data.
> + *
> + * This structure contains data required to create an IPsec SA security session.
> + */
> +struct rte_security_ipsec_xform {
> +	uint32_t spi;
> +	/**< SA security parameter index */
> +	uint32_t salt;
> +	/**< SA salt */
> +	struct rte_security_ipsec_sa_options options;
> +	/**< various SA options */
> +	enum rte_security_ipsec_sa_direction direction;
> +	/**< IPSec SA Direction - Egress/Ingress */
> +	enum rte_security_ipsec_sa_protocol proto;
> +	/**< IPsec SA Protocol - AH/ESP */
> +	enum rte_security_ipsec_sa_mode mode;
> +	/**< IPsec SA Mode - transport/tunnel */
> +	struct rte_security_ipsec_tunnel_param tunnel;
> +	/**< Tunnel parameters, NULL for transport mode */
> +};
> +
> +/**
> + * MACsec security session configuration
> + */
> +struct rte_security_macsec_xform {
> +	/** To be Filled */
> +};
> +
> +/**
> + * Security session action type.
> + */
> +enum rte_security_session_action_type {
> +	RTE_SECURITY_ACTION_TYPE_NONE,
> +	/**< No security actions */

This is not being used, it seems that you are only using it as marker to 
indicate end of capability set?

> +	RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
> +	/**< Crypto processing for security protocol is processed inline
> +	 * during transmission */
> +	RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL,
> +	/**< All security protocol processing is performed inline during
> +	 * transmission */
> +	RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL
> +	/**< All security protocol processing including crypto is performed
> +	 * on a lookaside accelerator */
> +};
> +
> +/** Security session protocol definition */
> +enum rte_security_session_protocol {
> +	RTE_SECURITY_PROTOCOL_IPSEC,
> +	/**< IPsec Protocol */
> +	RTE_SECURITY_PROTOCOL_MACSEC,
> +	/**< MACSec Protocol */
> +};
> +
> +/**
> + * Security session configuration
> + */
> +struct rte_security_session_conf {
> +	enum rte_security_session_action_type action_type;
> +	/**< Type of action to be performed on the session */
> +	enum rte_security_session_protocol protocol;
> +	/**< Security protocol to be configured */
> +	union {
> +		struct rte_security_ipsec_xform ipsec;
> +		struct rte_security_macsec_xform macsec;
> +	};
> +	/**< Configuration parameters for security session */
> +	struct rte_crypto_sym_xform *crypto_xform;
> +	/**< Security Session Crypto Transformations */
> +};
> +
> +struct rte_security_session {
> +	__extension__ void *sess_private_data;
> +	/**< Private session material */
> +};
> +


Do you need specific error handling for security sessions as well?
In case of full protocol offloads, you will need indications for
1. SEQ number overflow (egress side, if the SA is not refreshed on time)
2. Anti replay window config and err handlings?


> +/**
> + * Create security session as specified by the session configuration
> + *
> + * @param   id		security instance identifier id
> + * @param   conf	session configuration parameters

fix the indentation here and other places in this file.

> + * @param   mp		mempool to allocate session objects from
> + * @return
> + *  - On success, pointer to session
> + *  - On failure, NULL
> + */
> +struct rte_security_session *
> +rte_security_session_create(uint16_t id,
> +			    struct rte_security_session_conf *conf,
> +			    struct rte_mempool *mp);
> +
> +/**