Hi,
I found a "copy-before-check" issue in ot_ipsec_sa_common_param_fill()
(drivers/common/cnxk/cnxk_security.c). When AES-GMAC is used as the
authentication algorithm, the key is copied into cipher_key[32] before
the AES key length validation switch. If the key length exceeds 32,
the memcpy overflows into the adjacent SA member, and only afterward
is the invalid length detected and rejected with -EINVAL.
While the overflow is transient (the session creation fails and the
staging buffer is cleared on next use), this violates the
validate-before-write principle and should be fixed.
Confirmed against current HEAD (8dc80afd, 2026-03-05).
== Affected Code Flow ==
ot_ipsec_sa_common_param_fill() in cnxk_security.c, lines 13-200.
Step 1 -- AES-GMAC sets key and length from auth xform (line 137-144):
case RTE_CRYPTO_AUTH_AES_GMAC:
w2->s.auth_type = ROC_IE_SA_AUTH_AES_GMAC;
key = auth_xfrm->auth.key.data;
length =
auth_xfrm->auth.key.length; // e.g., 40
memcpy(salt_key, &ipsec_xfrm->salt,
4);
tmp_salt = (uint32_t *)salt_key;
*tmp_salt = rte_be_to_cpu_32(*tmp_salt);
break;
Step 2 -- Key is copied unconditionally (line 172-174):
if (key != NULL && length != 0) {
/* Copy encryption key */
memcpy(cipher_key, key, length);
// OVERFLOW: 40 > 32
...
}
Step 3 -- AES key length is validated AFTER the copy (line 180-198):
/* Set AES key length */
if (w2->s.enc_type == ROC_IE_SA_ENC_AES_CBC ||
w2->s.enc_type == ROC_IE_SA_ENC_AES_CTR ||
w2->s.enc_type == ROC_IE_SA_ENC_AES_GCM ||
w2->s.enc_type == ROC_IE_SA_ENC_AES_CCM ||
w2->s.auth_type ==
ROC_IE_SA_AUTH_AES_GMAC) {
switch (length) {
case ROC_CPT_AES128_KEY_LEN: /*
16 */
...
case ROC_CPT_AES192_KEY_LEN: /*
24 */
...
case ROC_CPT_AES256_KEY_LEN: /*
32 */
...
default:
plt_err("Invalid AES key
length");
return -EINVAL; //
rejected, but memcpy already done
}
}
For AES-GMAC, auth_type == ROC_IE_SA_AUTH_AES_GMAC, so this
validation block IS entered. A key length of 40 hits the default
case and returns -EINVAL. However, the memcpy at line 174 has
already written 40 bytes into cipher_key[32], overflowing 8 bytes
into the adjacent member.
== Affected Structure Layout ==
When called from cnxk_ot_ipsec_inb_sa_fill() (line 324), the
cipher_key parameter points to sa->cipher_key in the inbound SA:
struct roc_ot_ipsec_inb_sa {
(roc_ie_ot.h:284)
...
/* Word4 - Word7 */
uint8_t cipher_key[32];
/* ROC_CTX_MAX_CKEY_LEN */
/* Word8 - Word9 */
union {
struct {
uint32_t rsvd8;
uint8_t
salt[4]; /* active member */
} s;
uint64_t u64;
} w8;
uint64_t rsvd9;
...
};
Static assertions confirm (roc_ie_ot.h:404-407):
PLT_STATIC_ASSERT(offsetof(struct roc_ot_ipsec_inb_sa,
cipher_key) == 4 * sizeof(uint64_t));
/* offset 32 */
PLT_STATIC_ASSERT(offsetof(struct roc_ot_ipsec_inb_sa,
w8) == 8 *
sizeof(uint64_t)); /* offset 64 */
A 40-byte copy writes 32 bytes into cipher_key and 8 bytes into
the adjacent w8 union, whose active member salt[4] is overwritten.
The same pattern applies to the outbound SA (cipher_key[32]
followed by union roc_ot_ipsec_outb_iv iv).
== Why This Is Low-Severity ==
Unlike the DES/3DES cipher key overflow (where the function returns
success and the corrupted SA is used), in this case:
1. The AES key length switch at line 186 DOES catch the invalid
length and returns -EINVAL.
2. The callers properly propagate the error:
- cnxk_ot_ipsec_inb_sa_fill() returns rc at line 327-328.
- cn10k_eth_sec_session_create() checks rc at line 851 and
jumps to the error path. The SA is not written to
hardware.
- cn10k_eth_sec_session_update() similarly checks at line
1152.
3. The overwritten staging buffer (dev->inb.sa_dptr) is memset to
zero before every session creation (line 847), so the
transient
overflow does not persist.
Therefore the corrupted SA is never used, and the overflow is
transient. The practical impact is minimal.
== Affected Call Chains ==
cn10k_eth_sec_session_create()
[cn10k_ethdev_sec.c:734]
inbound: cnxk_ot_ipsec_inb_sa_fill()
[line 850]
outbound: cnxk_ot_ipsec_outb_sa_fill() [line
940]
-> ot_ipsec_sa_common_param_fill()
[line 324 / 437]
-> memcpy(cipher_key, key, length)
[line 174]
-> switch(length) ... return
-EINVAL [line 186-198]
cn10k_eth_sec_session_update()
[cn10k_ethdev_sec.c:1107]
inbound: cnxk_ot_ipsec_inb_sa_fill()
[line 1151]
outbound: cnxk_ot_ipsec_outb_sa_fill() [line
1186]
-> same path as above
The same "copy-before-check" pattern also exists in the OW family
helper ow_ipsec_sa_common_param_fill() (line 1366-1392), affecting
cn20k paths.
== Suggested Fix ==
Move the AES key length validation before the memcpy. The minimal
change:
if (key != NULL && length != 0) {
+ /* Validate key length before copy */
+ if (w2->s.enc_type ==
ROC_IE_SA_ENC_AES_CBC ||
+ w2->s.enc_type ==
ROC_IE_SA_ENC_AES_CTR ||
+ w2->s.enc_type ==
ROC_IE_SA_ENC_AES_GCM ||
+ w2->s.enc_type ==
ROC_IE_SA_ENC_AES_CCM ||
+ w2->s.auth_type ==
ROC_IE_SA_AUTH_AES_GMAC) {
+ switch (length) {
+ case 16: case 24: case 32:
break;
+ default: return -EINVAL;
+ }
+ }
+ /* Also validate DES/3DES key lengths */
+ if (w2->s.enc_type ==
ROC_IE_SA_ENC_DES_CBC && length != 8)
+ return -EINVAL;
+ if (w2->s.enc_type ==
ROC_IE_SA_ENC_3DES_CBC && length != 24)
+ return -EINVAL;
+
memcpy(cipher_key, key, length);
...
}
- /* Set AES key length (move the aes_key_len assignment
here,
- after the switch has been moved above) */
This consolidates all key length validation before the memcpy and
also fixes the DES/3DES gap reported separately. Apply the same
restructuring to ow_ipsec_sa_common_param_fill().
Best regards,
Pengpeng Hou
[email protected]
