Re: [Intel-wired-lan] [PATCH] ixgbe/ipsec: use memzero_explicit() for stack SA structs

[Dawid Osuchowski <dawid.osuchowski@linux.intel.com>]

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