Page MenuHomeFreeBSD

Simplify IPsec transform-specific teardown.
ClosedPublic

Authored by jhb on Thu, Jun 25, 12:43 AM.

Details

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)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Thu, Jun 25, 12:43 AM
jhb requested review of this revision.Thu, Jun 25, 12:43 AM
cem resigned from this revision.Thu, Jun 25, 12:44 AM
delphij accepted this revision.Thu, Jun 25, 4:44 AM

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.

This revision is now accepted and ready to land.Thu, Jun 25, 4:44 AM
jhb added a comment.Thu, Jun 25, 8:29 PM

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 added a comment.Thu, Jun 25, 8:47 PM
In D25443#562009, @jhb 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 added a comment.Thu, Jun 25, 8:52 PM
In D25443#562018, @jhb wrote:
In D25443#562009, @jhb 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 added inline comments.Thu, Jun 25, 8:56 PM
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 updated this revision to Diff 73666.Thu, Jun 25, 9:00 PM
  • Restore an empty tcpsignature_cleanup.
  • Restore some NULL pointer assignments.
This revision now requires review to proceed.Thu, Jun 25, 9:00 PM
jhb edited the summary of this revision. (Show Details)Thu, Jun 25, 9:02 PM
jhb removed a reviewer: cem.
delphij accepted this revision.Thu, Jun 25, 9:52 PM
This revision is now accepted and ready to land.Thu, Jun 25, 9:52 PM
This revision was automatically updated to reflect the committed changes.