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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dbc58b6f-b15e-42d9-b4d7-344b9ab53f74@linux.intel.com>
Date: Tue, 13 May 2025 15:21:24 +0200
From: Dawid Osuchowski <dawid.osuchowski@...ux.intel.com>
To: Zilin Guan <zilin@....edu.cn>
Cc: andrew+netdev@...n.ch, anthony.l.nguyen@...el.com, davem@...emloft.net,
 edumazet@...gle.com, intel-wired-lan@...ts.osuosl.org,
 jianhao.xu@....edu.cn, kuba@...nel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, pabeni@...hat.com, przemyslaw.kitszel@...el.com
Subject: Re: [PATCH] ixgbe/ipsec: use memzero_explicit() for stack SA structs

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