Page MenuHomeFreeBSD

cryptosoft: Fully support per-operation keys for auth algorithms.
ClosedPublic

Authored by jhb on Dec 7 2021, 12:37 AM.
Tags
None
Referenced Files
F102684436: D33316.diff
Fri, Nov 15, 8:20 PM
F102678861: D33316.id.diff
Fri, Nov 15, 6:28 PM
Unknown Object (File)
Mon, Nov 11, 12:16 PM
Unknown Object (File)
Sun, Nov 10, 12:45 PM
Unknown Object (File)
Sun, Nov 10, 12:44 PM
Unknown Object (File)
Wed, Oct 30, 11:40 AM
Unknown Object (File)
Oct 15 2024, 11:15 PM
Unknown Object (File)
Oct 15 2024, 5:19 AM
Subscribers

Details

Summary

Only pre-allocate auth contexts when a session-wide key is provided or
for sessions without keys. For sessions with per-operation keys,
always initialize the on-stack context directly rather than
initializing the session context in swcr_authprepare (now removed) and
then copying that session context into the on-stack context.

This approach permits parallel auth operations without needing a
serializing lock. In addition, the previous code assumed that auth
sessions always provided an initial key unlike cipher sessions which
assume either an initial key or per-op keys.

While here, fix the Blake2 auth transforms to function like other auth
transforms where Setkey is invoked after Init rather than before.

Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.Dec 7 2021, 12:37 AM
sys/opencrypto/cryptosoft.c
1227–1228

Just an observation: newsession implementations are generally allowed to sleep, so I'm not sure why this function and swcr_setup_cipher() specify M_NOWAIT everywhere.

1227–1228

How can we end up with auth_klen != 0 && auth_key == NULL? I understand that for BLAKE2 the key is optional but we need to allocate a context regardless, so I don't understand why this is conditional.

sys/opencrypto/cryptosoft.c
1227–1228

It is permissible to not provide a session-wide key (csp->csp_auth_key) so long as each operation provides a key (crp->crp_auth_key). I think right now GELI might even provide some dummy keys to workaround this. I think at least some other drivers (ccr) might currently require a per-op key. Hmm, in fact in crypto_session(9) I did document the stronger assertion that I don't currently assert in crp_sanity which is that a given session should _either_ use a session-wide key, or it should only use per-op keys with a NULL session key:

     csp_cipher_key   Pointer to encryption or decryption key.  If all
                      requests for a session use request-specific keys, this
                      field should be left as NULL.  This pointer and
                      associated key must remain valid for the duration of the
                      crypto session.

...

     csp_auth_key     Pointer to the authentication key.  If all requests for
                      a session use request-specific keys, this field should
                      be left as NULL.  This pointer and associated key must
                      remain valid for the duration of the crypto session.
1227–1228

I would like to fix them all, yes. I wasn't quite sure if that is what we allowed or not. I think it might be true that newsession can sleep, and we should document it in crypto_driver as well as for crypto_newsession if we haven't already.

sys/opencrypto/cryptosoft.c
1227–1228

Hmm, in crypto_driver(9) I documented NEWSESSION as not being allowed to sleep. We should perhaps change that though. crypto_newsession() uses M_WAITOK.

sys/opencrypto/cryptosoft.c
1227–1228

Ah, I forgot that the key length still comes from the session even for per-op requests. This makes more sense to me now.

1227–1228

Yeah, the M_WAITOK allocation in crypto_newsession() is what I was thinking of. Though, I see that crypto_invoke() may call crypto_newsession() in rare cases, and crypto_invoke() shouldn't sleep. That said, this isn't the only problem with our handling of crypto driver detach.

This revision is now accepted and ready to land.Dec 7 2021, 7:54 PM