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    [W:0.027 / U:153.032 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site