On 14 September 2013 10:08, Jussi Kivilinna <jussi.kivilinna@xxxxxx> wrote:
> On 13.09.2013 18:08, Ard Biesheuvel wrote:
>> This adds ARMv8 Crypto Extensions based implemenations of
>> AES in CBC, CTR and XTS mode.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> ---
> ..snip..
>> +static int xts_set_key(struct crypto_tfm *tfm, const u8 *in_key,
>> + unsigned int key_len)
>> +{
>> + struct crypto_aes_xts_ctx *ctx = crypto_tfm_ctx(tfm);
>> + u32 *flags = &tfm->crt_flags;
>> + int ret;
>> +
>> + ret = crypto_aes_expand_key(&ctx->key1, in_key, key_len/2);
>> + if (!ret)
>> + ret = crypto_aes_expand_key(&ctx->key2, &in_key[key_len/2],
>> + key_len/2);
>
> Use checkpatch.
>
Um, I did get a bunch of errors and warnings from checkpatch.pl tbh,
put not in this particular location. Care to elaborate?
>> + if (!ret)
>> + return 0;
>> +
>> + *flags |= CRYPTO_TFM_RES_BAD_KEY_LEN;
>> + return -EINVAL;
>> +}
>> +
>> +static int cbc_encrypt(struct blkcipher_desc *desc, struct scatterlist *dst,
>> + struct scatterlist *src, unsigned int nbytes)
>> +{
>> + struct crypto_aes_ctx *ctx = crypto_blkcipher_ctx(desc->tfm);
>> + int err, first, rounds = 6 + ctx->key_length/4;
>> + struct blkcipher_walk walk;
>> + unsigned int blocks;
>> +
>> + blkcipher_walk_init(&walk, dst, src, nbytes);
>> + err = blkcipher_walk_virt(desc, &walk);
>> +
>> + kernel_neon_begin();
>
> Is sleeping allowed within kernel_neon_begin/end block? If not, you need to
> clear CRYPTO_TFM_REQ_MAY_SLEEP on desc->flags. Otherwise blkcipher_walk_done
> might sleep.
>
Good point. No, sleeping is not allowed in this case, so I should
clear the flag.
>> + for (first = 1; (blocks = (walk.nbytes / AES_BLOCK_SIZE)); first = 0) {
>> + aesce_cbc_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
>> + (u8*)ctx->key_enc, rounds, blocks, walk.iv,
>> + first);
>> +
>> + err = blkcipher_walk_done(desc, &walk, blocks * AES_BLOCK_SIZE);
>> + }
>> + kernel_neon_end();
>> +
>> + /* non-integral sizes are not supported in CBC */
>> + if (unlikely(walk.nbytes))
>> + err = -EINVAL;
>
> I think blkcipher_walk_done already does this check by comparing against
> alg.cra_blocksize.
>
You're right, it does. I will remove it.
>> +
>> + return err;
>> +}
> ..snip..
>> +
>> +static int ctr_encrypt(struct blkcipher_desc *desc, struct scatterlist *dst,
>> + struct scatterlist *src, unsigned int nbytes)
>> +{
>> + struct crypto_aes_ctx *ctx = crypto_blkcipher_ctx(desc->tfm);
>> + int err, first, rounds = 6 + ctx->key_length/4;
>> + struct blkcipher_walk walk;
>> + u8 ctr[AES_BLOCK_SIZE];
>> +
>> + blkcipher_walk_init(&walk, dst, src, nbytes);
>> + err = blkcipher_walk_virt(desc, &walk);
>> +
>> + memcpy(ctr, walk.iv, AES_BLOCK_SIZE);
>> +
>> + kernel_neon_begin();
>> + for (first = 1; (nbytes = walk.nbytes); first = 0) {
>> + aesce_ctr_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
>> + (u8*)ctx->key_enc, rounds, nbytes, ctr, first);
>> +
>> + err = blkcipher_walk_done(desc, &walk, 0);
>> +
>> + /* non-integral block *must* be the last one */
>> + if (unlikely(walk.nbytes && (nbytes & (AES_BLOCK_SIZE-1)))) {
>> + err = -EINVAL;
>
> Other CTR implementations do not have this.. not needed?
>
I should be using blkcipher_walk_virt_block() here with a block size
of AES_BLOCK_SIZE. In that case, all calls but the last one will be at
least AES_BLOCK_SIZE long. But that still means I need to take care to
only consume multiples of AES_BLOCK_SIZE in all iterations except the
last one.
>> + break;
>> + }
>> + }
> ..snip..
>> +static struct crypto_alg aesce_cbc_algs[] = { {
>> + .cra_name = "__cbc-aes-aesce",
>> + .cra_driver_name = "__driver-cbc-aes-aesce",
>> + .cra_priority = 0,
>> + .cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER,
>> + .cra_blocksize = AES_BLOCK_SIZE,
>> + .cra_ctxsize = sizeof(struct crypto_aes_ctx),
>> + .cra_alignmask = 0,
>> + .cra_type = &crypto_blkcipher_type,
>> + .cra_module = THIS_MODULE,
>> + .cra_u = {
>> + .blkcipher = {
>> + .min_keysize = AES_MIN_KEY_SIZE,
>> + .max_keysize = AES_MAX_KEY_SIZE,
>> + .ivsize = AES_BLOCK_SIZE,
>> + .setkey = crypto_aes_set_key,
>> + .encrypt = cbc_encrypt,
>> + .decrypt = cbc_decrypt,
>> + },
>> + },
>> +}, {
>> + .cra_name = "__ctr-aes-aesce",
>> + .cra_driver_name = "__driver-ctr-aes-aesce",
>> + .cra_priority = 0,
>> + .cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER,
>> + .cra_blocksize = AES_BLOCK_SIZE,
>
> CTR mode is stream cipher, cra_blocksize must be set to 1.
>
> This should have been picked up by in-kernel run-time tests, check
> CONFIG_CRYPTO_MANAGER_DISABLE_TESTS (and CONFIG_CRYPTO_TEST/tcrypt
> module).
>
Well, run-time implies access to hardware :-) As I indicated in the
cover letter, these bits are only compile tested.
[...]
>> +.Lencsteal:
>> + ldrb w6, [x1], #1
>> + ldrb w7, [x2, #-16]
>> + strb w6, [x2, #-16]
>> + strb w7, [x2], #1
>> + subs x5, x5, #1
>> + bne .Lencsteal
>
> Cipher text stealing here is dead-code, since alg.cra_blocksize is set
> to 16 bytes.
>
> Currently other XTS implementations do not have CTS either so this is
> not really needed anyway atm (crypto/xts.c: "sector sizes which are not
> a multiple of 16 bytes are, however currently unsupported").
>
I'll remove it then, if we can't test it anyway.
Cheers,
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
