Simplify IPsec transform-specific teardown.

[jhb]

Summary

• Rename from the teardown callback from 'zeroize' to 'cleanup' since this no longer zeroes keys.

• Change the callback return type to void. Nothing checked the return value and it was always zero.

• Don't have esp call into ah since it no longer needs to depend on this to clear the auth key. Instead, both are now private and self-contained.

Test Plan

• IPsec tunnels over both IPv4 (AES-CBC + SHA1, AES-GCM) and IPv6 (AES-GCM)

[delphij]

I'm not 100% comfortable with the removal of these = NULL's but it appears that the code didn't cared about their value being non-zero, so the change as-is seemed Okay.

I'd probably change key_cleansav() to perform a bzero on the whole 'sav' structure and remove all sav->* = NULL, but that's only a personal preference.

[jhb]

In D25443#561680, @delphij wrote:

I'm not 100% comfortable with the removal of these = NULL's but it appears that the code didn't cared about their value being non-zero, so the change as-is seemed Okay.

I'd probably change key_cleansav() to perform a bzero on the whole 'sav' structure and remove all sav->* = NULL, but that's only a personal preference.

I'd be fine doing a single zero of the sav. I'd much rather do that centralized than the current approach of partial clearing scattered about. I do think that in all these cases we are doing a zero just before freeing, and that none of these pointers being cleared currently are sensitive in the sense of requiring explicit_bzero().

[jhb]

In D25443#562009, @jhb wrote:

In D25443#561680, @delphij wrote:

I'm not 100% comfortable with the removal of these = NULL's but it appears that the code didn't cared about their value being non-zero, so the change as-is seemed Okay.

I'd probably change key_cleansav() to perform a bzero on the whole 'sav' structure and remove all sav->* = NULL, but that's only a personal preference.

I'd be fine doing a single zero of the sav. I'd much rather do that centralized than the current approach of partial clearing scattered about. I do think that in all these cases we are doing a zero just before freeing, and that none of these pointers being cleared currently are sensitive in the sense of requiring explicit_bzero().

Hmm, this is not quite trivial. Some fields like sav->lock and sav->lftc_c probably need to not be zeroed in key_cleansav. Some of that I can fix by moving key_cleansav down in key_delsav just before the free and in that case we could even use zfree. This would potentially permit removing many of the NULL assignments from key_cleansav. However, key_cleansav is also called for the error case for key_setsaval. Hmm, looking at that more though, if that errors the key always gets freed, and in one of those cases we effectively duplicate key_freeval. Maybe I can clean this up.

[jhb]

In D25443#562018, @jhb wrote:

In D25443#562009, @jhb wrote:

In D25443#561680, @delphij wrote:

I'm not 100% comfortable with the removal of these = NULL's but it appears that the code didn't cared about their value being non-zero, so the change as-is seemed Okay.

I'd probably change key_cleansav() to perform a bzero on the whole 'sav' structure and remove all sav->* = NULL, but that's only a personal preference.

I'd be fine doing a single zero of the sav. I'd much rather do that centralized than the current approach of partial clearing scattered about. I do think that in all these cases we are doing a zero just before freeing, and that none of these pointers being cleared currently are sensitive in the sense of requiring explicit_bzero().

Hmm, this is not quite trivial. Some fields like sav->lock and sav->lftc_c probably need to not be zeroed in key_cleansav. Some of that I can fix by moving key_cleansav down in key_delsav just before the free and in that case we could even use zfree. This would potentially permit removing many of the NULL assignments from key_cleansav. However, key_cleansav is also called for the error case for key_setsaval. Hmm, looking at that more though, if that errors the key always gets freed, and in one of those cases we effectively duplicate key_freeval. Maybe I can clean this up.

Hmmm, I guess for key_setsaval from key_update it is possible the ref count is greater than 0. I can put some of the NULL pointers back I suppose.

[jhb]

sys/netipsec/xform_tcp.c

361 ↗

(On Diff #73615)

I have to put this back since the call to xf_cleanup isn't conditional. I think having an empty function here is the simpler approach than checking xf_cleanup against NULL.

[jhb]

• Restore an empty tcpsignature_cleanup.
• Restore some NULL pointer assignments.