If you'd like an alternate testing tool (one that can test AES-CBC chained with SHA for example), you can try 'cryptocheck' from github/bsdjhb/freebsd.git on the 'cryptocheck' branch. You can also run IPSec across a session using the ciphers for testing but that requires a bit more setup.

Thanks

In D12437#258495, @cem wrote:

In D12437#258465, @jhb wrote:

Unfortunately we can't just assume 'blocksize' because some algorithms (keyed SHA1, NULL, etc.) have 0 for the keysize. I just think 'max_keysize' is a bit of a misnomer because I consider the 'auth_xform' structures to provide values related to "what the algorithm uses" in which case 'keysize' is a single fixed-length field much like 'blocksize'. Handling "user provided keys" of different sizes is something that needs to happen at a slightly higher level similar to how one uses PKCS padding to deal with data that isn't an even multiple of a block. It's not the end of the world though. I looked and OpenSSL doesn't provide anything like 'keysize' in its EVP_MD structures, and HMAC() requires a key and assumes a block size (so for OpenSSL it seems you have to provide a bogus key for NULL HMAC which seems odd)

Ok. Would you like me to rename max_keysize back to keysize? Are there any other changes you'd like to see before this goes in?

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.

In D12437#258463, @cem wrote:

In D12437#258455, @jhb wrote:

Hmm, the commit message is perhaps not clear as it conflates the two meanings of "key" when it first states "there is no limit", which applies to "user supplied key" but it is about correcting the size of the "key the algorithm uses".

I'm not sure I understand the concern or what changes you would like made. I'll try to clarify the commit message slightly.

I would perhaps leave the member as 'keysize' instead of 'max_keysize'.  Perhaps we can replace it in the future with a flag indicating if a key is valid at all and if it is the implied value is 'blocksize'.

I did consider just removing keysize entirely and using the blocksize field as the limit, but worried about missing an instance of the struct (they do not use C99 initializers). Maybe I should just replace it with a padding field. I'd be happy to make this change.

Hmm, the commit message is perhaps not clear as it conflates the two meanings of "key" when it first states "there is no limit", which applies to "user supplied key" but it is about correcting the size of the "key the algorithm uses". I would perhaps leave the member as 'keysize' instead of 'max_keysize'. Perhaps we can replace it in the future with a flag indicating if a key is valid at all and if it is the implied value is 'blocksize'.

On a general note, I would almost consider making this part of aesni.ko instead if the effective sets of CPUs that support both instruction sets is the same (or nearly so). In particular, the existing aesni.ko code does not accelerate things that combine AES with an HMAC (like an IPSec session using AES-CBC with a SHA HMAC). Having a single module that registers support for whatever algorithms the CPU supports (only SHA if AESNI isn't available for example) would permit the driver to handle these chained operations by combining AESNI with accelerated SHA hashing. (In this case aesni.ko would perhaps be better named "x86_crypto.ko" or some such to mean "accelerated software crypto for x86".)

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

In D12437#258088, @cem wrote:

In D12437#258077, @jhb wrote:

By larger keys I meant keys larger than the block size. ccr(4) handles this explicitly in ccr_init_hmac_digest() for example. However, this same bit of code needs to be duplicated in all drivers which seems a bit odd.

Ah. None of the drivers I reviewed handles larger than block size correctly. It would be a little odd to have to duplicate that in all of the drivers.

• Style.

By larger keys I meant keys larger than the block size. ccr(4) handles this explicitly in ccr_init_hmac_digest() for example. However, this same bit of code needs to be duplicated in all drivers which seems a bit odd. For keys larger than the block size I would rather have the crypto framework handle this I think by hashing the key before passing it down to the driver. Similarly, I would expect a layer above the drivers to handle any padding required for "short" keys so that drivers always get a key that is a block size.

Have you checked any drivers to see if they handle the larger keys? ccr(4) does I believe (and out-of-tree qat(4) does). I haven't checked our software ones to see if they do.

Note that 'ps -p <X>' is supposed to only output the header and not fail it seems, so you need to ensure you aren't breaking that. See rS127513 and rS127149 which added the nentries check and explain why. (svn annotate is your friend, there was only one "noise" change of s/0/NULL/ to skip over to find these two commits for the line you are changing)

• Use warn() for errors from pathconf().
• Add braces in foreach routines.

• Add explicit padding in vfpreg.

In D12290#256189, @kib wrote:

In D12290#255968, @jhb wrote:

One question is if 'u_long' is the right type for 'sv_hwcap'.

De-facto long becomes the way to express the 'native word size' of a machine. It is used more often than register_t, and I think that register_t is actually wrong e.g. on mips n32 for this purpose.

So the actual initialization of *sv_hwcap for native sysents will follow in a separate patch, I suppose ?

• Assert threads are suspended for get/set_vfpcontext.

Thanks

Gerald Pfeifer has recently changed the default for USE_GCC to GCC 6 and will soon fix Uses/compiler.mk to use GCC 6 (instead of 5) for the C++14 case. At that point I think this patch can be committed as is.

• Add NT_ARM_VFP support to gcore.

• Trim _ARM from HWCAP names.

• Update for sv_hwcap.

Re: AT_HWCAP2, once elf.h is unified (which I will probably work on soon) it will be fairly easy to add.

• Fix feature typo.

• Replace elf32/64_hwcap with a sv_hwcap pointer in sysentvec.
• Only provide AT_HWCAP if the pointer exists.