Re: [patch] UBIFS: Add cryptographic functionality when a key ispassed to the compress / decompress functions

[Artem Bityutskiy]

On Thu, 2012-03-29 at 16:39 +0200, Joel Reardon wrote:
>  /* Fake description object for the "none" compressor */
>  static struct ubifs_compressor none_compr = {
>  	.compr_type = UBIFS_COMPR_NONE,
> @@ -75,6 +78,55 @@ static struct ubifs_compressor zlib_compr = {
>  struct ubifs_compressor *ubifs_compressors[UBIFS_COMPR_TYPES_CNT];
> 
>  /**
> + * ubifs_aes_crypt - encrypt / decrypt data.
> + * @str: data to crypt
> + * @len: length of the data
> + * @crypto_key: the cryptographic key to use to crypt the data
> + * @crypto_key_len: the length of the crypto_key
> + * @iv: the initialization vector to use
> + * @ivlen: the length of the initialization vector
> + *
> + * This function applies aes encryption to the data. It is done in counter
> + * mode, which means that encryption and decryption are the same operation,
> + * i.e., it XORs the same generated bitstream, so it can be used both for
> + * encryption / decryption. The operation is done in-place, so str mutates.
> + */
> +int ubifs_aes_crypt(void *str, int len, u8 *crypto_key,
> +		    int crypto_key_len, u8 *iv, int ivlen)

You support only one length - please, kill ivlen parameter.

Also, should ubifs_aes_crypt be static? I do not see any users outside
of compress.c. In this case remove the "ubifs_" prefix. But a
non-written convention, in UBIFS we _tend_ to prefix only non-static
functions with "ubifs_" and avoid having it for static functions.

> +{
> +	struct crypto_blkcipher *tfm;
> +	struct blkcipher_desc desc;
> +	struct scatterlist sg;
> +	int err = 0;
> +
> +	tfm = crypto_alloc_blkcipher(UBIFS_CRYPTO_ALGORITHM, 0, 0);
> +

Unnecessary empty line.

> +	if (IS_ERR(tfm)) {
> +		ubifs_err("failed to load transform for aes: %ld",
> +			  PTR_ERR(tfm));
> +		return err;
> +	}
> +
> +	err = crypto_blkcipher_setkey(tfm, crypto_key, crypto_key_len);
> +	desc.tfm = tfm;
> +	desc.flags = 0;
> +	if (err) {
> +		ubifs_err("crypto_blkcipher_setkey() failed  flags=%#x",
> +			  crypto_blkcipher_get_flags(tfm));
> +		return err;
> +	}
> +	memset(&sg, 0, sizeof(struct scatterlist));
> +

Empty lines mean grouping, and I think this memeset should be grouped
with sg_set_buf instead.


>  no_compr:
>  	memcpy(out_buf, in_buf, in_len);
>  	*out_len = in_len;
>  	*compr_type = UBIFS_COMPR_NONE;
> +	goto encrypt;
> +
> +encrypt:

I guess the above goto is redundant?

> +	if (crypto_key) {
> +		u8 iv[UBIFS_CRYPTO_KEYSIZE];
> +
> +		memset(iv, 0, UBIFS_CRYPTO_KEYSIZE);
> +		ubifs_aes_crypt(out_buf, *out_len, crypto_key,
> +				UBIFS_CRYPTO_KEYSIZE, iv, UBIFS_CRYPTO_KEYSIZE);
> +	}
>  }
> 
>  /**
> @@ -149,7 +211,7 @@ no_compr:
>   * The length of the uncompressed data is returned in @out_len. This functions
>   * returns %0 on success or a negative error code on failure.
>   */
> -int ubifs_decompress(const void *in_buf, int in_len, void *out_buf,
> +int ubifs_decompress(void *in_buf, int in_len, void *out_buf,
>  		     int *out_len, int compr_type, u8 *crypto_key)

Please, write a fat "WARNING" note in the comment and tell that this
function modifies the input buffer.

> +/* Size of 128 bits in bytes */
> +#define AES_KEYSIZE_128 16

If you have no plans to support keys larger than 128 just kill this
constant please.

-- 
Best Regards,
Artem Bityutskiy

Attachment: signature.asc
Description: This is a digitally signed message part