Patchwork UBIFS: Add cryptographic functionality when a key is passed to the compress / decompress functions

login
register
mail settings
Submitter Joel Reardon
Date March 29, 2012, 2:39 p.m.
Message ID <alpine.DEB.2.00.1203291639200.912@eristoteles.iwoars.net>
Download mbox | patch
Permalink /patch/149398/
State New
Headers show

Comments

Joel Reardon - March 29, 2012, 2:39 p.m.
This patch adds a function to perform AES encryption. The compress and
decompress routines use this function if they are called with a non-NULL
key parameter. It uses AES counter mode (where encryption and decryption
are the same function) and performs the operation in place on the data. It
uses a default IV of 0, since each key is only evey used to encrypt one
data item the IV does not matter.

The const qualifier was removed from the decompress routine for the
following reason. Encrypted data is not compressable, so compression is
first applied then the result is encrypted. In the reverse, decryption is
first applied and the result decompressed. This means that either the
input buffer for decompression is used to perform an in-place decryption
before decompression, or a third buffer is added and data is copied around.


Signed-off-by: Joel Reardon <reardonj@inf.ethz.ch>
---
 fs/ubifs/compress.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/ubifs/ubifs.h    |   12 +++++++-
 2 files changed, 85 insertions(+), 4 deletions(-)
Artem Bityutskiy - April 2, 2012, 2:36 p.m.
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.
Joel Reardon - April 2, 2012, 2:48 p.m.
>
> 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.
>

Should length for key remain, and the IV is just the same? Or should the
global #define just be used inside the aes function.

There is another use where the data is decrypted and reencrypted with a
different key. (during GC and if an erase block becomes bad.) In this
case, the data is not decompressed and recompressed, only the encryption
changes. However, for simplicity, and because its not frequent, we can
make it static and use the compress functions to handle this.

>
> I guess the above goto is redundant?
>

It is, but I put it in for future developers who may add a new control
case there after without expecting the above to 'fall through'.
Artem Bityutskiy - April 2, 2012, 2:57 p.m.
On Mon, 2012-04-02 at 16:48 +0200, Joel Reardon wrote:
> >
> > 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.
> >
> 
> Should length for key remain, and the IV is just the same? Or should the
> global #define just be used inside the aes function.

I guess I was a little confused WRT iv vs cryptokey. Anyway, I thought
you just use UBIFS_CRYPTO_KEYSIZE for all of this, why not to just use
the constant in this function? Why passing the length as an argument?
Joel Reardon - April 2, 2012, 2:58 p.m.
>
> I guess I was a little confused WRT iv vs cryptokey. Anyway, I thought
> you just use UBIFS_CRYPTO_KEYSIZE for all of this, why not to just use
> the constant in this function? Why passing the length as an argument?
>

There's no actually reason... in my head, if I have a char buffer then the
next parameter is its length. I'll change it so the comments discuss the
length of the buffer.

Patch

diff --git a/fs/ubifs/compress.c b/fs/ubifs/compress.c
index c91974a..f94cf21 100644
--- a/fs/ubifs/compress.c
+++ b/fs/ubifs/compress.c
@@ -27,9 +27,12 @@ 
  * decompression.
  */

-#include <linux/crypto.h>
 #include "ubifs.h"

+#include <linux/crypto.h>
+#include <linux/scatterlist.h>
+
+
 /* 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)
+{
+	struct crypto_blkcipher *tfm;
+	struct blkcipher_desc desc;
+	struct scatterlist sg;
+	int err = 0;
+
+	tfm = crypto_alloc_blkcipher(UBIFS_CRYPTO_ALGORITHM, 0, 0);
+
+	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));
+
+	sg_set_buf(&sg, str, len);
+	desc.info = iv;
+	err = crypto_blkcipher_encrypt(&desc, &sg, &sg, len);
+	crypto_free_blkcipher(tfm);
+	if (err)
+		return err;
+	return 0;
+}
+
+/**
  * ubifs_compress - compress data.
  * @in_buf: data to compress
  * @in_len: length of the data to compress
@@ -127,12 +179,22 @@  void ubifs_compress(const void *in_buf, int in_len, void *out_buf, int *out_len,
 	if (in_len - *out_len < UBIFS_MIN_COMPRESS_DIFF)
 		goto no_compr;

-	return;
+	goto encrypt;

 no_compr:
 	memcpy(out_buf, in_buf, in_len);
 	*out_len = in_len;
 	*compr_type = UBIFS_COMPR_NONE;
+	goto encrypt;
+
+encrypt:
+	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)
 {
 	int err;
@@ -167,6 +229,15 @@  int ubifs_decompress(const void *in_buf, int in_len, void *out_buf,
 		return -EINVAL;
 	}

+	if (crypto_key) {
+		u8 iv[UBIFS_CRYPTO_KEYSIZE];
+
+		memset(iv, 0, UBIFS_CRYPTO_KEYSIZE);
+		ubifs_aes_crypt(in_buf, in_len, crypto_key,
+				UBIFS_CRYPTO_KEYSIZE, iv,
+				UBIFS_CRYPTO_KEYSIZE);
+	}
+
 	if (compr_type == UBIFS_COMPR_NONE) {
 		memcpy(out_buf, in_buf, in_len);
 		*out_len = in_len;
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index b425264..dbfa508 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -160,6 +160,14 @@ 
 /* Maximum number of data nodes to bulk-read */
 #define UBIFS_MAX_BULK_READ 32

+/* Size of 128 bits in bytes */
+#define AES_KEYSIZE_128 16
+
+/* Key size in bytes for UBIFS */
+#define UBIFS_CRYPTO_KEYSIZE AES_KEYSIZE_128
+/* AES in counter mode is the encryption algorithm. */
+#define UBIFS_CRYPTO_ALGORITHM "ctr(aes)"
+
 /*
  * Lockdep classes for UBIFS inode @ui_mutex.
  */
@@ -1771,9 +1779,11 @@  long ubifs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 /* compressor.c */
 int __init ubifs_compressors_init(void);
 void ubifs_compressors_exit(void);
+int ubifs_aes_crypt(void *str, int len, u8 *crypto_key, int crypto_key_len,
+		    u8 *iv, int ivlen);
 void ubifs_compress(const void *in_buf, int in_len, void *out_buf, int *out_len,
 		    int *compr_type, u8 *crypto_key);
-int ubifs_decompress(const void *buf, int len, void *out, int *out_len,
+int ubifs_decompress(void *buf, int len, void *out, int *out_len,
 		     int compr_type, u8 *crypto_key);

 #include "debug.h"