On 2025-05-13 2:24 PM, Zilin Guan wrote:
OK, I will resend the patch to the iwl-net branch and include the Fixes
tag. Before I do that, I noticed that in ixgbe_ipsec_add_sa() we clear
the Tx SA struct with memset 0 on key-parsing failure but do not clear
the Rx SA struct in the corresponding error path:
617 /* get the key and salt */
618 ret = ixgbe_ipsec_parse_proto_keys(xs, rsa.key, &rsa.salt);
619 if (ret) {
620 NL_SET_ERR_MSG_MOD(extack,
"Failed to get key data for Rx SA table");
621 return ret; /* <- no memzero_explicit() here */
622 }
...
728 if (ret) {
729 NL_SET_ERR_MSG_MOD(extack,
"Failed to get key data for Tx SA table");
730 memset(&tsa, 0, sizeof(tsa));
731 return ret; /* <- clears tsa on error */
732 }
Both paths return immediately on key-parsing failure, should I add a
memzero_explicit(&rsa, sizeof(rsa)) before Rx-SA's return or remove the
memset(&tsa, ...) in the Tx-SA path to keep them consistent?
From the code in ixgbe_ipsec_parse_proto_keys() it seems that copying
of the salt and key values occurs at the end of the function and only in
case of success, see below.
---
if (key_len == IXGBE_IPSEC_KEY_BITS) {
*mysalt = ((u32 *)key_data)[4];
} else if (key_len != (IXGBE_IPSEC_KEY_BITS - (sizeof(*mysalt) * 8))) {
netdev_err(dev, "IPsec hw offload only supports keys up to 128 bits
with a 32 bit salt\n");
return -EINVAL;
} else {
netdev_info(dev, "IPsec hw offload parameters missing 32 bit salt
value\n");
*mysalt = 0;
}
memcpy(mykey, key_data, 16);
return 0;
---
In my (limited) understanding the memset(&tsa, 0, ...) call in case of
error after the ixgbe_ipsec_parse_proto_keys() is redundant, as there is
nothing to clear in the tsa.key and tsa.salt. The rsa and tsa also
contain the pointer to the xfrm_state and I am unsure whether we should
clear that as well.
Please note that I do not have much experience with ipsec so take my
opinion with a grain of salt. Best for someone more experienced to assess.
Thanks,
Dawid
Best Regards,
Zilin Guan
