Page MenuHomeFreeBSD

crypto(9): Use a more specific error code when a capable driver is not found
ClosedPublic

Authored by cem on Sep 22 2017, 4:44 AM.

Details

Summary

When crypto_newsession() is given a request for an unsupported
capability, raise a more specific error than EINVAL.

This allows cryptotest.py to skip some HMAC tests that a driver does not
support.

Test Plan

Ran cryptotest.py with a SHA driver that only supports SHA1 and 256. SHA1 and
256 test cases are executed and pass. Others are skipped.

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

cem created this revision.Sep 22 2017, 4:44 AM
jhb added a comment.Sep 22 2017, 7:07 PM

I was going to say that you needed to update crypto(4) but it doesn't have an ERRORS section yet. :(

Note that using this should also let you remove the hardcoded lists of supported "cipher groups" for device names and all the 'skipif' stuff. Do the tests report as FAIL or SKIP when you raise an exception like this?

sys/opencrypto/crypto.c
466 ↗(On Diff #33298)

Why not change this instead? I think it will be more consistent and less confusing to have 'EOPNOTSUPP' mean "driver doesn't support algorithm or could not find a driver that does" than to use EINVAL still for the case that 'crid' was a wildcard and we couldn't find a driver that supports the algorithm.

cem added a comment.Sep 22 2017, 7:16 PM
In D12451#258445, @jhb wrote:

Note that using this should also let you remove the hardcoded lists of supported "cipher groups" for device names and all the 'skipif' stuff.

Potentially, although I wouldn't do that. The goal is to allow running any supported NIST-KAT HMAC tests against an HMAC-supporting driver. For example, sha_sse only supports SHA1 and SHA256, while the NIST-KAT suite includes SHA224, 384, and 512 vectors as well. It seems wasteful to pummel an AES driver with a whole suite of unsupported HMAC vectors.

Do the tests report as FAIL or SKIP when you raise an exception like this?

Uncaught exceptions result in failure. This change catches only EOPNOTSUPP error exceptions.

cem added inline comments.Sep 22 2017, 7:18 PM
sys/opencrypto/crypto.c
466 ↗(On Diff #33298)

I think it might be useful to distinguish the two cases with different error codes.

The "could not find a driver" case could be ENOENT or ENXIO or something.

jmg added a comment.Sep 23 2017, 6:17 AM

It would be good to get a proper errors section added to the documentation.

tests/sys/opencrypto/cryptotest.py
290 ↗(On Diff #33298)

I'd recommend moving this comment after the except so that it has sane indenting.

cem added a comment.Sep 23 2017, 8:24 PM
In D12451#258497, @jmg wrote:

It would be good to get a proper errors section added to the documentation.

Do you consider that an essential part of this change, or could that go in as a separate revision?

tests/sys/opencrypto/cryptotest.py
290 ↗(On Diff #33298)

Seems sane and clear to me. Comments generally go before the line(s) they describe.

jmg added a comment.Sep 24 2017, 5:50 PM
In D12451#258622, @cem wrote:
In D12451#258497, @jmg wrote:

It would be good to get a proper errors section added to the documentation.

Do you consider that an essential part of this change, or could that go in as a separate revision?

Any change that changes behavior, IMO, requires documentation updates as part of the change. W/o documentation, the chance is incomplete.

cem added a comment.Sep 24 2017, 5:55 PM
In D12451#258784, @jmg wrote:

Any change that changes behavior, IMO, requires documentation updates as part of the change. W/o documentation, the chance is incomplete.

The existing behavior is already undocumented. This isn't reducing the accuracy of the documentation.

jhb added a comment.Sep 25 2017, 4:24 PM

I still prefer a single errno value (it's also a smaller code patch and is a bit simpler to read post-patch). I don't think adding the errors section is a hard requirement for this change though, that is an old bug.

tests/sys/opencrypto/cryptotest.py
290 ↗(On Diff #33298)

I think it's a little clearer if it's inside the except as it is describing what the except clause does. Either that or before the try (as it is describing the reason for having a try/except at all), but of those two I prefer it inside the except.

cem planned changes to this revision.Sep 25 2017, 6:29 PM
In D12451#258930, @jhb wrote:

I still prefer a single errno value (it's also a smaller code patch and is a bit simpler to read post-patch).

Ok.

tests/sys/opencrypto/cryptotest.py
290 ↗(On Diff #33298)

Yeah, I think that's backwards, but it seems to be consensus — will do.

cem updated this revision to Diff 33408.Sep 25 2017, 6:35 PM
cem marked 4 inline comments as done.
  • Return NOTSUPP for any missed capability
  • Move a comment below the line it describes by popular demand
cem retitled this revision from crypto(9): Use a more specific error code when a driver isn't capable to crypto(9): Use a more specific error code when a capable driver is not found.Sep 25 2017, 6:36 PM
cem edited the summary of this revision. (Show Details)
jhb accepted this revision.Sep 25 2017, 8:02 PM

Thanks

This revision is now accepted and ready to land.Sep 25 2017, 8:02 PM
rlibby accepted this revision.Sep 25 2017, 9:34 PM

The change itself seems fine to me. I'll defer to others with respect to whether this is the right direction and whether the documentation needs to be in this revision.

I don't think it is necessarily inconsistent that the various cryptodev_newsession implementations return EINVAL for the corresponding cases where they don't support an algorithm. It may be worth a comment somewhere though.

I guess an alternative might have been to make a more fine-grained capability map for each driver in the test. This would be more of a pain to maintain, but on the positive side it would not require (undocumented) error interpretation, and it would allow checking for erroneous lack of support (as in, we want to test that $driver supports $algo and if it returns EINVAL/EOPNOTSUPP instead then we want to fail the test).

cem added a comment.Sep 26 2017, 1:28 AM

I don't think it is necessarily inconsistent that the various cryptodev_newsession implementations return EINVAL for the corresponding cases where they don't support an algorithm. It may be worth a comment somewhere though.

I think those could maybe even be turned into ASSERTs. It seems like crypto(4) should prevent drivers from being invoked with capabilities they do not advertise.

This revision was automatically updated to reflect the committed changes.