Page MenuHomeFreeBSD

test/sys/opencrypto: Fix NIST KAT parser iterator
ClosedPublic

Authored by kd on Oct 3 2022, 10:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 12 2024, 8:58 AM
Unknown Object (File)
Dec 20 2023, 3:27 AM
Unknown Object (File)
Dec 14 2023, 9:00 AM
Unknown Object (File)
Aug 26 2023, 7:10 AM
Unknown Object (File)
Aug 26 2023, 7:09 AM
Unknown Object (File)
Aug 26 2023, 7:09 AM
Unknown Object (File)
Aug 26 2023, 7:09 AM
Unknown Object (File)
Aug 26 2023, 6:59 AM
Subscribers

Details

Summary

When "yield" a.k.a a "generator" iterator is used we need to return all data using "yield", before actually returning from the function.
Because of that only encryption tests were run for AES-CBC.
I believe that other modes suffered from this bug as well.
Add one more loop to the iterator "next" routine to fix that.
This unveiled a problem in the GCM AEAD parser logic, which didn't correctly handle tests cases with empty plaintext, i.e. AAD only.
Include the fix in this patch as it's a rather trivial one.

Test Plan

Do ./runtests.sh on an x86 machine

Before:

kern.crypto.allow_soft: 1 -> 1
1..1
..................
----------------------------------------------------------------------
Ran 18 tests in 0.868s

After:

kern.crypto.allow_soft: 1 -> 1
1..1
..................
----------------------------------------------------------------------
Ran 18 tests in 4.102s

OK
ok 1
kern.crypto.allow_soft: 1 -> 1

Note the x5 difference in execution time.

Diff Detail

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

Event Timeline

kd requested review of this revision.Oct 3 2022, 10:06 AM
kd created this revision.

FWIW, here are the before/after stats for runs on ccr(4) which give a more detailed breakdown of the change in stats.

Before:

dev.ccr.0.stats.port.1.completed: 2601
dev.ccr.0.stats.port.1.queued: 2601
dev.ccr.0.stats.port.1.active_sessions: 0
dev.ccr.0.stats.port.0.completed: 2609
dev.ccr.0.stats.port.0.queued: 2609
dev.ccr.0.stats.port.0.active_sessions: 0
dev.ccr.0.stats.sw_fallback: 390
dev.ccr.0.stats.process_error: 0
dev.ccr.0.stats.sglist_error: 0
dev.ccr.0.stats.pad_error: 0
dev.ccr.0.stats.mac_error: 240
dev.ccr.0.stats.inflight: 0
dev.ccr.0.stats.wr_nomem: 0
dev.ccr.0.stats.ccm_decrypt: 360
dev.ccr.0.stats.ccm_encrypt: 2130
dev.ccr.0.stats.gcm_decrypt: 0
dev.ccr.0.stats.gcm_encrypt: 0
dev.ccr.0.stats.eta_decrypt: 0
dev.ccr.0.stats.eta_encrypt: 0
dev.ccr.0.stats.cipher_decrypt: 0
dev.ccr.0.stats.cipher_encrypt: 1639
dev.ccr.0.stats.hmac: 180
dev.ccr.0.stats.hash: 901

After:

dev.ccr.0.stats.port.1.completed: 7404
dev.ccr.0.stats.port.1.queued: 7404
dev.ccr.0.stats.port.1.active_sessions: 0
dev.ccr.0.stats.port.0.completed: 7410
dev.ccr.0.stats.port.0.queued: 7410
dev.ccr.0.stats.port.0.active_sessions: 0
dev.ccr.0.stats.sw_fallback: 2190
dev.ccr.0.stats.process_error: 0
dev.ccr.0.stats.sglist_error: 0
dev.ccr.0.stats.pad_error: 0
dev.ccr.0.stats.mac_error: 696
dev.ccr.0.stats.inflight: 0
dev.ccr.0.stats.wr_nomem: 0
dev.ccr.0.stats.ccm_decrypt: 360
dev.ccr.0.stats.ccm_encrypt: 2130
dev.ccr.0.stats.gcm_decrypt: 900
dev.ccr.0.stats.gcm_encrypt: 6300
dev.ccr.0.stats.eta_decrypt: 0
dev.ccr.0.stats.eta_encrypt: 0
dev.ccr.0.stats.cipher_decrypt: 1639
dev.ccr.0.stats.cipher_encrypt: 1639
dev.ccr.0.stats.hmac: 945
dev.ccr.0.stats.hash: 901

From this it seems like none of the GCM tests were actually run previously. The total number of tests run went from 5210 to 14814.

This revision is now accepted and ready to land.Oct 3 2022, 9:47 PM
tests/sys/opencrypto/cryptodev.py
291

Why not just use a truthiness statement instead?

377–386

didread is only used once; this logic can be compressed, avoiding the need for an intermediate variable.

389–393

This seems better expressed with a regular expression, e.g.,

# `field_re` should be initialized outside the loops for performance reasons.
field_re = re.compile(r"\[(?P<field>[^]]+)\]")
matches = field_re.match(i)
if matches is None:
    raise ValueError("Unknown line: %r" % (i))
yield matches.group("field"), self.fielditer()

Update the patch based on ngie@ comments.

@jhb Could you please run the test suite again on ccr?
I just want to make sure that the numbers of tests run hasn't decreased in this version of the patch.

This revision now requires review to proceed.Oct 4 2022, 7:55 AM
tests/sys/opencrypto/cryptodev.py
291

I wanted to match the condition in line 261.
I've changed both of them instead.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 6 2022, 2:53 PM
This revision was automatically updated to reflect the committed changes.