Details

Reviewers

glebius
tuexen
jhb
cmiller_netapp.com
rrs

Group Reviewers

transport

Commits

rG85df11a1dec6: ktls: deep copy tls_enable struct for in-kernel tcp consumers

Summary

Doing a deep copy of the keys early allows users of the tls_enable structure to assume kernel memory.
This also enables the socket options to be set by kernel threads.

Taking over the commit process from cmiller_netapp.com after internal coordination.

Sponsored by: NetApp, Inc.
X-NetApp-PR: #79

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 56515
Build 53403: arc lint + arc unit

Event Timeline

cmiller_netapp.com created this revision.Mar 5 2024, 11:07 PM

Herald added a reviewer: transport. · View Herald TranscriptMar 5 2024, 11:07 PM

Herald added a subscriber: melifaro. · View Herald Transcript

cmiller_netapp.com requested review of this revision.Mar 5 2024, 11:07 PM

cmiller_netapp.com added a project: NetApp.Mar 5 2024, 11:55 PM

rscheff added reviewers: tuexen, jhb.Mar 6 2024, 10:56 AM

rscheff requested changes to this revision.Mar 6 2024, 6:34 PM

rscheff added a subscriber: rscheff.

rscheff added inline comments.

sys/kern/uipc_ktls.c
380	in ktls_destroy(), all crypto material is purged before freeing, by using zfree(). It would be prudent to do the same here for all these crypto keys.

This revision now requires changes to proceed.Mar 6 2024, 6:34 PM

rscheff commandeered this revision from cmiller_netapp.com.Mar 6 2024, 7:02 PM

rscheff edited reviewers, added: cmiller_netapp.com; removed: rscheff.

rscheff edited the summary of this revision. (Show Details)

rscheff removed a reviewer: rscheff.

clean before free all crypto material

Herald added subscribers: olce, imp. · View Herald TranscriptMar 6 2024, 7:02 PM

Harbormaster completed remote builds in B56485: Diff 135466.Mar 6 2024, 7:02 PM

rrs accepted this revision.Mar 7 2024, 3:45 PM

This revision is now accepted and ready to land.Mar 7 2024, 3:45 PM

clean key material in the error path too.

This revision now requires review to proceed.Mar 7 2024, 9:15 PM

Harbormaster completed remote builds in B56509: Diff 135530.Mar 7 2024, 9:15 PM

The use of zfree is fine and is the intended use case.

sys/kern/uipc_ktls.c
407	Should really be `if (error != 0)` in new code here and elsewhere.
442	Should check pointers against NULL explicitly, however, free(9) (just like free(3) in userland) handles NULL pointers just fine, so I would just remove all the checks and call `zfree` unconditionally instead.
sys/sys/ktls.h
216	I don't think you need the blank line after these, but they should be sorted alphabetically to match the rest of the prototypes. I would also probably add a `ktls_` prefix to these two functions since the rest of the exported (public) API from uipc_ktls.c uses that prefix.