lkml.org 
[lkml]   [2012]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [patch] UBIFS: Add cryptographic functionality when a key is passed to the compress / decompress functions
From
Date
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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-04-02 16:35    [from the cache]
©2003-2011 Jasper Spaans