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

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

[Zilin Guan]

The function ixgbe_ipsec_add_sa() currently uses memset() to zero out
stack-allocated SA structs (rsa and tsa) before return, but the gcc-11.4.0
compiler optimizes these calls away. This leaves sensitive key and salt
material on the stack after return.

Replace these memset() calls with memzero_explicit() to prevent the
compiler from optimizing them away. This guarantees that the SA key and
salt are reliably cleared from the stack.

Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 07ea1954a276..e8c84f7e937b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -678,7 +678,7 @@ static int ixgbe_ipsec_add_sa(struct xfrm_state *xs,
 		} else {
 			/* no match and no empty slot */
 			NL_SET_ERR_MSG_MOD(extack, "No space for SA in Rx IP SA table");
-			memset(&rsa, 0, sizeof(rsa));
+			memzero_explicit(&rsa, sizeof(rsa));
 			return -ENOSPC;
 		}
 
@@ -727,7 +727,7 @@ static int ixgbe_ipsec_add_sa(struct xfrm_state *xs,
 		ret = ixgbe_ipsec_parse_proto_keys(xs, tsa.key, &tsa.salt);
 		if (ret) {
 			NL_SET_ERR_MSG_MOD(extack, "Failed to get key data for Tx SA table");
-			memset(&tsa, 0, sizeof(tsa));
+			memzero_explicit(&tsa, sizeof(tsa));
 			return ret;
 		}
 
-- 
2.34.1

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

[Dawid Osuchowski]

On 2025-05-12 12:58 PM, Zilin Guan wrote:
> The function ixgbe_ipsec_add_sa() currently uses memset() to zero out
> stack-allocated SA structs (rsa and tsa) before return, but the gcc-11.4.0
> compiler optimizes these calls away. This leaves sensitive key and salt
> material on the stack after return.
> 
> Replace these memset() calls with memzero_explicit() to prevent the
> compiler from optimizing them away. This guarantees that the SA key and
> salt are reliably cleared from the stack.
> 
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>

Thanks for your patch.

Please use the correct target iwl-net for fixes, iwl-next for features 
and others.

Maybe add a tag? Fixes: 63a67fe229ea ("ixgbe: add ipsec offload add and 
remove SA")

In the future when sending patches against Intel networking drivers 
please send them directly To: intel-wired-lan@lists.osuosl.org and Cc: 
netdev@vger.kernel.org.

> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index 07ea1954a276..e8c84f7e937b 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -678,7 +678,7 @@ static int ixgbe_ipsec_add_sa(struct xfrm_state *xs,
>   		} else {
>   			/* no match and no empty slot */
>   			NL_SET_ERR_MSG_MOD(extack, "No space for SA in Rx IP SA table");
> -			memset(&rsa, 0, sizeof(rsa));
> +			memzero_explicit(&rsa, sizeof(rsa));
>   			return -ENOSPC;
>   		}
>   
> @@ -727,7 +727,7 @@ static int ixgbe_ipsec_add_sa(struct xfrm_state *xs,
>   		ret = ixgbe_ipsec_parse_proto_keys(xs, tsa.key, &tsa.salt);
>   		if (ret) {
>   			NL_SET_ERR_MSG_MOD(extack, "Failed to get key data for Tx SA table");
> -			memset(&tsa, 0, sizeof(tsa));
> +			memzero_explicit(&tsa, sizeof(tsa));

As for the code change itself, LGTM.

Acked-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>

Thanks,
Dawid

>   			return ret;
>   		}
>   

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

[Zilin Guan]

On Mon, May 12, 2025 at 02:53:12PM+0200, Dawid Osuchowski wrote:
> Thanks for your patch.
> 
> Please use the correct target iwl-net for fixes, iwl-next for features 
> and others.
> 
> Maybe add a tag? Fixes: 63a67fe229ea ("ixgbe: add ipsec offload add and 
> remove SA")
> 
> In the future when sending patches against Intel networking drivers 
> please send them directly To: intel-wired-lan@lists.osuosl.org and Cc: 
> netdev@vger.kernel.org.
> 
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?

Best Regards,
Zilin Guan

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

[Dawid Osuchowski]

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

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

[Zilin Guan]

On Tue, May 13, 2025 at 12:24:41PM+0000, Zilin Guan wrote:
> 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?
> 
> Best Regards,
> Zilin Guan

If this change is required, should I submit it as a new standalone patch, 
or include it in a v2 of the existing patch series?

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

[Dawid Osuchowski]

On 2025-05-13 3:31 PM, Zilin Guan wrote:
> If this change is required, should I submit it as a new standalone patch,
> or include it in a v2 of the existing patch series?

I think you could include it with the v2, as it touches the same stack 
SA structs (if you decide to reuse memzero_explicit() on them).

Thanks,
Dawid

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

[Przemek Kitszel]

On 5/13/25 15:54, Dawid Osuchowski wrote:
> On 2025-05-13 3:31 PM, Zilin Guan wrote:
>> If this change is required, should I submit it as a new standalone patch,
>> or include it in a v2 of the existing patch series?
> 
> I think you could include it with the v2, as it touches the same stack 
> SA structs (if you decide to reuse memzero_explicit() on them).
> 
the general rule is to memzero_explicit() memory that was holding secure
content
--
to have full picture: it is fine to memset() such storage prior to use,
it is also fine to combine related changes in one commit/one series

re stated purpose of the patch:
I see @rsa cleaned in just one exit point of ixgbe_ipsec_add_sa(),
instead of all of them, so v2 seems warranted

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

[Zilin Guan]

On Thu, May 15, 2025 at 11:27:22AM+0200, Przemek Kitszel wrote:
> the general rule is to memzero_explicit() memory that was holding secure
> content
> --
> to have full picture: it is fine to memset() such storage prior to use,
> it is also fine to combine related changes in one commit/one series
> 
> re stated purpose of the patch:
> I see @rsa cleaned in just one exit point of ixgbe_ipsec_add_sa(),
> instead of all of them, so v2 seems warranted

Hi Przemek,

Thank you for your detailed feedback and clarification.

As Dawid pointed out, while @rsa is cleared at one exit point in 
ixgbe_ipsec_add_sa(), another exit path, at which we fail to acquire the 
RX SA table, leaves rsa.key and rsa.salt zeroed. Does this imply there's
no sensitive data to clear in this case? If so, would using memset() on 
the symmetric error path in @tsa be redundant, or am I overlooking 
something?

I'd appreciate your thoughts on this.

Best regards,
Zilin Guan