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