Skip to content

[PATCH API-NEXT v1] api: crypto: deprecate per-session IV #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lumag wants to merge 1 commit into from
Closed

[PATCH API-NEXT v1] api: crypto: deprecate per-session IV #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions example/ipsec/odp_ipsec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ pkt_disposition_e do_ipsec_in_classify(odp_packet_t pkt,
if (esp) {
params.cipher_range.offset = ipv4_data_p(ip) + hdr_len - buf;
params.cipher_range.length = ipv4_data_len(ip) - hdr_len;
params.override_iv_ptr = esp->iv;
params.iv_ptr = esp->iv;
}

/* Issue crypto request */
Expand Down Expand Up @@ -884,7 +884,15 @@ pkt_disposition_e do_ipsec_out_classify(odp_packet_t pkt,
trl_len = encrypt_len - ip_data_len;

esp->spi = odp_cpu_to_be_32(entry->esp.spi);
memcpy(esp + 1, entry->state.iv, entry->esp.iv_len);

/* Generate an IV */
if (entry->esp.iv_len) {
int32_t size = entry->esp.iv_len;
int32_t ret = odp_random_data(esp->iv, size, 1);
Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use enum here: ODP_RANDOM_CRYPTO


if (ret != size)
abort();
}

esp_t = (odph_esptrl_t *)(ip_data + encrypt_len) - 1;
esp_t->pad_len = trl_len - sizeof(*esp_t);
Expand All @@ -894,6 +902,7 @@ pkt_disposition_e do_ipsec_out_classify(odp_packet_t pkt,
esp_t->next_header = ip->proto;
ip->proto = ODPH_IPPROTO_ESP;

params.iv_ptr = esp->iv;
params.cipher_range.offset = ip_data - buf;
params.cipher_range.length = encrypt_len;
}
Expand Down
15 changes: 2 additions & 13 deletions example/ipsec/odp_ipsec_cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,11 @@ int create_ipsec_cache_entry(sa_db_entry_t *cipher_sa,
params.cipher_alg = cipher_sa->alg.u.cipher;
params.cipher_key.data = cipher_sa->key.data;
params.cipher_key.length = cipher_sa->key.length;
params.iv.data = entry->state.iv;
params.iv.length = cipher_sa->iv_len;
params.iv_length = cipher_sa->iv_len;
mode = cipher_sa->mode;
} else {
params.cipher_alg = ODP_CIPHER_ALG_NULL;
params.iv.data = NULL;
params.iv.length = 0;
params.iv_length = 0;
}

/* Auth */
Expand All @@ -105,15 +103,6 @@ int create_ipsec_cache_entry(sa_db_entry_t *cipher_sa,
params.auth_alg = ODP_AUTH_ALG_NULL;
}

/* Generate an IV */
if (params.iv.length) {
int32_t size = params.iv.length;

int32_t ret = odp_random_data(params.iv.data, size, 1);
if (ret != size)
return -1;
}

/* Synchronous session create for now */
if (odp_crypto_session_create(&params, &session, &ses_create_rc))
return -1;
Expand Down
1 change: 0 additions & 1 deletion example/ipsec/odp_ipsec_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ typedef struct ipsec_cache_entry_s {
odp_crypto_session_t session; /**< Crypto session handle */
uint32_t esp_seq; /**< ESP TX sequence number */
uint32_t ah_seq; /**< AH TX sequence number */
uint8_t iv[MAX_IV_LEN]; /**< ESP IV storage */
odp_u16be_t tun_hdr_id; /**< Tunnel header IP ID */
} state;
} ipsec_cache_entry_t;
Expand Down
18 changes: 12 additions & 6 deletions include/odp/api/spec/crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,14 +234,14 @@ typedef struct odp_crypto_key {
/**
* Crypto API IV structure
*/
typedef struct odp_crypto_iv {
typedef struct ODP_DEPRECATE(odp_crypto_iv) {
/** IV data */
uint8_t *data;

/** IV length in bytes */
uint32_t length;

} odp_crypto_iv_t;
} ODP_DEPRECATE(odp_crypto_iv_t);
Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is really a bug correction, perhaps this should just be removed without deprecation? Why would anyone want to retain the incorrect behavior "for compatibility"?

Copy link
Author

@lumag lumag May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bill-Fischofer-Linaro I would like to hear a word from other parties. This bug might be local to linux-generic. And there might be use cases for the IV specified at session creation time (e.g. if packets should be encrypted in one continuous stream).

Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During today's ARCH call this was discussed briefly and Nikhil pointed out that per-session IVs are useful for many cases, so deprecation may be premature, especially as we already permit per-packet IV overrides. We can continue this discussion on Monday's ARCH call if the mailing list can't reach resolution before then.


/**
* Crypto API data range specifier
Expand Down Expand Up @@ -285,8 +285,11 @@ typedef struct odp_crypto_session_param_t {
*/
odp_crypto_key_t cipher_key;

/** Cipher Initialization Vector (IV) */
odp_crypto_iv_t iv;
/** @deprecated use iv_length and per-packet IV instead */
ODP_DEPRECATE(odp_crypto_iv_t) ODP_DEPRECATE(iv);
Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear why checkpatch gets confused here. It seems to think this is a cast of some sort. Another reason perhaps to simply correct this issue without deprecation.


/** Cipher Initialization Vector (IV) length */
uint32_t iv_length;

/** Authentication algorithm
*
Expand Down Expand Up @@ -359,8 +362,11 @@ typedef struct odp_crypto_op_param_t {
*/
odp_packet_t out_pkt;

/** Override session IV pointer */
uint8_t *override_iv_ptr;
/** @deprecated use iv_ptr instead */
uint8_t *ODP_DEPRECATE(override_iv_ptr);

/** IV pointer */
uint8_t *iv_ptr;

/** Offset from start of packet for hash result
*
Expand Down
4 changes: 4 additions & 0 deletions platform/linux-generic/include/odp_crypto_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#ifndef ODP_CRYPTO_INTERNAL_H_
#define ODP_CRYPTO_INTERNAL_H_

#include <odp/api/deprecated.h>

#ifdef __cplusplus
extern "C" {
#endif
Expand Down Expand Up @@ -40,8 +42,10 @@ struct odp_crypto_generic_session {
odp_bool_t do_cipher_first;

struct {
#if ODP_DEPRECATED_API
/* Copy of session IV data */
uint8_t iv_data[MAX_IV_LEN];
#endif
Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needing an #ifdef looks ugly here and elsewhere in this series. Better to correct the code than retain "compatibility" here.


union {
struct {
Expand Down
65 changes: 54 additions & 11 deletions platform/linux-generic/odp_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,14 @@ odp_crypto_alg_err_t aes_encrypt(odp_crypto_op_param_t *param,
unsigned char iv_enc[AES_BLOCK_SIZE];
void *iv_ptr;

if (param->override_iv_ptr)
if (param->iv_ptr)
iv_ptr = param->iv_ptr;
#if ODP_DEPRECATED_API
else if (param->override_iv_ptr)
iv_ptr = param->override_iv_ptr;
else if (session->p.iv.data)
iv_ptr = session->cipher.iv_data;
#endif
else
return ODP_CRYPTO_ALG_ERR_IV_INVALID;

Expand Down Expand Up @@ -216,10 +220,14 @@ odp_crypto_alg_err_t aes_decrypt(odp_crypto_op_param_t *param,
unsigned char iv_enc[AES_BLOCK_SIZE];
void *iv_ptr;

if (param->override_iv_ptr)
if (param->iv_ptr)
iv_ptr = param->iv_ptr;
#if ODP_DEPRECATED_API
else if (param->override_iv_ptr)
iv_ptr = param->override_iv_ptr;
else if (session->p.iv.data)
iv_ptr = session->cipher.iv_data;
#endif
else
return ODP_CRYPTO_ALG_ERR_IV_INVALID;

Expand All @@ -241,9 +249,14 @@ odp_crypto_alg_err_t aes_decrypt(odp_crypto_op_param_t *param,

static int process_aes_param(odp_crypto_generic_session_t *session)
{
/* Verify IV len is either 0 or 16 */
if (!((0 == session->p.iv.length) || (16 == session->p.iv.length)))
/* Verify IV len is 16 */
#if ODP_DEPRECATED_API
if (!((16 == session->p.iv_length) || (16 == session->p.iv.length)))
return -1;
#else
if (16 != session->p.iv_length)
Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above got deleted. Magic numbers should either be enums or at least retain a comment explaining their use/significance.

return -1;
#endif

/* Set function */
if (ODP_CRYPTO_OP_ENCODE == session->p.op) {
Expand Down Expand Up @@ -273,10 +286,14 @@ odp_crypto_alg_err_t aes_gcm_encrypt(odp_crypto_op_param_t *param,
void *iv_ptr;
uint8_t *tag = data + param->hash_result_offset;

if (param->override_iv_ptr)
if (param->iv_ptr)
iv_ptr = param->iv_ptr;
#if ODP_DEPRECATED_API
else if (param->override_iv_ptr)
iv_ptr = param->override_iv_ptr;
else if (session->p.iv.data)
iv_ptr = session->cipher.iv_data;
#endif
else
return ODP_CRYPTO_ALG_ERR_IV_INVALID;

Expand Down Expand Up @@ -338,10 +355,14 @@ odp_crypto_alg_err_t aes_gcm_decrypt(odp_crypto_op_param_t *param,
void *iv_ptr;
uint8_t *tag = data + param->hash_result_offset;

if (param->override_iv_ptr)
if (param->iv_ptr)
iv_ptr = param->iv_ptr;
#if ODP_DEPRECATED_API
else if (param->override_iv_ptr)
iv_ptr = param->override_iv_ptr;
else if (session->p.iv.data)
iv_ptr = session->cipher.iv_data;
#endif
else
return ODP_CRYPTO_ALG_ERR_IV_INVALID;

Expand Down Expand Up @@ -392,6 +413,13 @@ odp_crypto_alg_err_t aes_gcm_decrypt(odp_crypto_op_param_t *param,

static int process_aes_gcm_param(odp_crypto_generic_session_t *session)
{
uint32_t iv_length = session->p.iv_length;

#if ODP_DEPRECATED_API
if (0 != session->p.iv.length)
iv_length = session->p.iv.length;
#endif

/* Verify Key len is 16 */
if (session->p.cipher_key.length != 16)
return -1;
Expand All @@ -409,7 +437,7 @@ static int process_aes_gcm_param(odp_crypto_generic_session_t *session)
}

EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_IVLEN,
session->p.iv.length, NULL);
iv_length, NULL);
if (ODP_CRYPTO_OP_ENCODE == session->p.op) {
EVP_EncryptInit_ex(ctx, NULL, NULL,
session->p.cipher_key.data, NULL);
Expand All @@ -430,10 +458,14 @@ odp_crypto_alg_err_t des_encrypt(odp_crypto_op_param_t *param,
DES_cblock iv;
void *iv_ptr;

if (param->override_iv_ptr)
if (param->iv_ptr)
iv_ptr = param->iv_ptr;
#if ODP_DEPRECATED_API
else if (param->override_iv_ptr)
iv_ptr = param->override_iv_ptr;
else if (session->p.iv.data)
iv_ptr = session->cipher.iv_data;
#endif
else
return ODP_CRYPTO_ALG_ERR_IV_INVALID;

Expand Down Expand Up @@ -468,10 +500,14 @@ odp_crypto_alg_err_t des_decrypt(odp_crypto_op_param_t *param,
DES_cblock iv;
void *iv_ptr;

if (param->override_iv_ptr)
if (param->iv_ptr)
iv_ptr = param->iv_ptr;
#if ODP_DEPRECATED_API
else if (param->override_iv_ptr)
iv_ptr = param->override_iv_ptr;
else if (session->p.iv.data)
iv_ptr = session->cipher.iv_data;
#endif
else
return ODP_CRYPTO_ALG_ERR_IV_INVALID;

Expand Down Expand Up @@ -500,9 +536,14 @@ odp_crypto_alg_err_t des_decrypt(odp_crypto_op_param_t *param,

static int process_des_param(odp_crypto_generic_session_t *session)
{
/* Verify IV len is either 0 or 8 */
if (!((0 == session->p.iv.length) || (8 == session->p.iv.length)))
/* Verify IV len is 8 */
#if ODP_DEPRECATED_API
if (!((8 == session->p.iv_length) || (8 == session->p.iv.length)))
return -1;
#else
if (8 != session->p.iv_length)
return -1;
#endif

/* Set function */
if (ODP_CRYPTO_OP_ENCODE == session->p.op)
Expand Down Expand Up @@ -679,6 +720,7 @@ odp_crypto_session_create(odp_crypto_session_param_t *param,
session->p = *param;

/* Copy IV data */
#if ODP_DEPRECATED_API
if (session->p.iv.data) {
if (session->p.iv.length > MAX_IV_LEN) {
ODP_DBG("Maximum IV length exceeded\n");
Expand All @@ -689,6 +731,7 @@ odp_crypto_session_create(odp_crypto_session_param_t *param,
memcpy(session->cipher.iv_data, session->p.iv.data,
session->p.iv.length);
}
#endif

/* Derive order */
if (ODP_CRYPTO_OP_ENCODE == param->op)
Expand Down
11 changes: 3 additions & 8 deletions test/common_plat/performance/odp_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,7 @@ static crypto_alg_config_t algs_config[] = {
.data = test_key24,
.length = sizeof(test_key24)
},
.iv = {
.data = test_iv,
.length = 8,
},
.iv_length = 8,
.auth_alg = ODP_AUTH_ALG_NULL
},
},
Expand All @@ -201,10 +198,7 @@ static crypto_alg_config_t algs_config[] = {
.data = test_key24,
.length = sizeof(test_key24)
},
.iv = {
.data = test_iv,
.length = 8,
},
.iv_length = 8,
.auth_alg = ODP_AUTH_ALG_MD5_HMAC,
.auth_key = {
.data = test_key16,
Expand Down Expand Up @@ -552,6 +546,7 @@ run_measure_one(crypto_args_t *cargs,
}
mem = odp_packet_data(newpkt);
memset(mem, 1, payload_length);
params.iv_ptr = test_iv;
params.pkt = newpkt;
params.out_pkt = cargs->in_place ? newpkt :
ODP_PACKET_INVALID;
Expand Down
Loading