Created attachment 221152 [details] ZFS OCF Offload of SHA256 Patch to add OCF Offload for SHA256. AESNI, OSSL, SAFEXCEL, and Cryptosoft did not have implementations that matched the documentation and ASSERT()'s to implement SHA256. Tested on Ryzen (SHA instructions), Intel (without SHA) and Espressobin (arm64)
Created attachment 222488 [details] The patch with OpenZFS contrib files excluded
I don't quite understand the bug here. Can you provide a test case which illustrates where e.g., safexcel, is going wrong?
The original patch includes adding a consumer for OCF to offload SHA256 in ZFS, it can be used to test. When implementing that patch I encountered an issue where none of the OCF drivers were handling crp_digest_start on a crypto_request. So the drivers would run Init and Finish without running Update producing an incorrect result and causing ZFS to output checksum errors as the data written prior to patching was not matching the miscalculated checksum. After discussing it with Allan Jude I've resubmitted the patch without the OpenZFS parts and will submit those parts upstream after some modification so it will not break Linux compiles.
Created attachment 234523 [details] OCF KASSERT patch After BSDCan I revisited this patch and found that the man page and one of the KASSERTs that lead me to attempt to work around the asserts were removed/changed. This patch to change another assert to allow a 128K data buffer to be checksumed into a 256bit output buffer permits the offload of SHA256 from ZFS.
I don't think the patch is correct. I think the issue is probably that you aren't using a separate output buffer, and in the case that we aren't using a separate buffer, we should just assert that the output_payload_start is 0 and not check it otherwise. Or perhaps you are using the output only for the digest and not doing encryption? In that case your patch might indeed be correct. I think though in that case we'd like to assert that payload_output_start is not used (i.e. 0), something like: if (csp->mode == CSP_MODE_DIGEST) KASSERT(payload_output_start == 0) else /* existing test that output region fits in olen */
Created attachment 234528 [details] OCF KASSERT patch w/ jhb suggested changes I am using using an output buffer and only calculating the sha256 hash, ZFS encryption is offloaded to OCF elsewhere. My patch for ZFS is here: https://github.com/gldisater/freebsd/commit/ca66e8943dc3be3f4266ddd663750859c82c1291
This looks good to me. I would perhaps adjust the assertion message for the digest case as the existing messages tell you what is wrong vs what is right, so maybe something like "output digest is not at offset 0" or some such? There is probably some room to add some other assertions if not present already such as rejecting separate output buffers for requests that want to verify a digest vs computing a digest.
Do you need me to push this for you, and if so, is the e-mail address used here ok to use?
Created attachment 234665 [details] Second jhb patch I'm not a committer if that's what you mean by push.
Hmm, the new assertion would trip on ETA and AEAD sessions with separate output buffers doing decryption. It's only pure-digest verify operations that shouldn't use an output buffer. I've restructured the patch a bit instead.
Created attachment 234740 [details] zfs_crypto_hash_assert.patch
The second assert isn't asserting a verify op but the message says it's a "digest verify". Should probably be "digest operation" or just "digest".
What other operations use crp_payload_length? It's not asserted to be > 0, only less than ilen/olen, which are derived from the buffer using crypto_buffer_len().
(In reply to Jeremy Faulkner from comment #13) Every operation uses crp_payload_length. It determines the length of both the input and output payloads (except for compression which has a separate output length in crp_olen). crp_payload_output_start is only used for requests which both have a separate output buffer and generate an output payload (everything except CSP_MODE_DIGEST which only generates output digests at crp_digest_start).
Also, zero-byte payloads are permitted for all but CSP_MODE_CIPHER (where it would be a no-op) as you can compute a digest / MAC on a zero-byte payload. (Plus CSP_MODE_ETA and CSP_MODE_AEAD can have an AAD region and a zero-length payload.)
(In reply to Jeremy Faulkner from comment #12) Thanks, I've changed it to "digest operation" locally.
I've put the latest patch up for review at https://reviews.freebsd.org/D35578
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=c71f2370c5d480cf70f12ee276e044681c57aefc commit c71f2370c5d480cf70f12ee276e044681c57aefc Author: John Baldwin <jhb@FreeBSD.org> AuthorDate: 2022-06-30 17:10:00 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2022-06-30 17:10:00 +0000 crypto: Fix assertions for digest-only sessions with separate output. Digest-only sessions do not generate modified payload as an output, so don't bother asserting anything about the payload with respect to the output buffer other than the payload output start being zero. In addition, a verify request on a digest-only session doesn't generate any output at all so should never have a separate output buffer. PR: 252316 Reviewed by: markj Co-authored-by: Jeremy Faulkner <gldisater@gmail.com> MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D35578 sys/opencrypto/crypto.c | 5 +++++ 1 file changed, 5 insertions(+)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=b53f356918f872063b098741e797b8ebea3d03b9 commit b53f356918f872063b098741e797b8ebea3d03b9 Author: John Baldwin <jhb@FreeBSD.org> AuthorDate: 2022-06-30 17:10:00 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2022-07-13 16:25:14 +0000 crypto: Fix assertions for digest-only sessions with separate output. Digest-only sessions do not generate modified payload as an output, so don't bother asserting anything about the payload with respect to the output buffer other than the payload output start being zero. In addition, a verify request on a digest-only session doesn't generate any output at all so should never have a separate output buffer. PR: 252316 Reviewed by: markj Co-authored-by: Jeremy Faulkner <gldisater@gmail.com> MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D35578 (cherry picked from commit c71f2370c5d480cf70f12ee276e044681c57aefc) sys/opencrypto/crypto.c | 5 +++++ 1 file changed, 5 insertions(+)
I think this addresses the in-tree changes for this bug as the ZFS bits went upstream to OpenZFS directly.