Re: [PATCH encrypted swsusp 1/3] core functionality

[Andreas Steinmetz]

Rafael J. Wysocki wrote:
> Hi,
> 
> On Monday, 11 of April 2005 18:11, Andreas Steinmetz wrote:
> 
>>Pavel Machek wrote:
>>
>>>Was it really neccessary to include "union u"? I don't like its name,
>>
>>Here comes the patch with this reverted. I'm now using casts when
>>'abusing' the space for encryption. Furthermore the iv set up in the tfm
>>is used instead of the local copy.
> 
> 
> I had no time to review your patch earlier, sorry.  I'm inlining it so that I can
> comment it:
> 
> 
>>--- linux-2.6.11.2/kernel/power/swsusp.c.ast	2005-04-10 14:08:55.000000000 +0200
>>+++ linux-2.6.11.2/kernel/power/swsusp.c	2005-04-11 18:05:58.000000000 +0200
>>@@ -31,6 +31,9 @@
>>  * Alex Badea <vampire@xxxxx>:
>>  * Fixed runaway init
>>  *
>>+ * Andreas Steinmetz <ast@xxxxxxxx>:
>>+ * Added encrypted suspend option
>>+ *
>>  * More state savers are welcome. Especially for the scsi layer...
>>  *
>>  * For TODOs,FIXMEs also look in Documentation/power/swsusp.txt
>>@@ -72,6 +75,16 @@
>> 
>> #include "power.h"
>> 
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+#include <linux/random.h>
>>+#include <linux/crypto.h>
>>+#include <asm/scatterlist.h>
>>+#endif
>>+
>>+#define CIPHER "aes"
>>+#define MAXKEY 32
>>+#define MAXIV  32
> 
> 
> Why not to put these definitions under #ifdef?
> 

I keep it the way Pavel likes it here if you don't mind.

> 
>>+
>> /* References to section boundaries */
>> extern const void __nosave_begin, __nosave_end;
>> 
>>@@ -104,7 +117,9 @@
>> #define SWSUSP_SIG	"S1SUSPEND"
>> 
>> static struct swsusp_header {
>>-	char reserved[PAGE_SIZE - 20 - sizeof(swp_entry_t)];
> 
> 
> I would add #ifdef here as well.

Same as above.

> 
> 
>>+	char reserved[PAGE_SIZE - 20 - MAXKEY - MAXIV - sizeof(swp_entry_t)];
>>+	u8 key[MAXKEY];
>>+	u8 iv[MAXIV];
>> 	swp_entry_t swsusp_info;
>> 	char	orig_sig[10];
>> 	char	sig[10];
>>@@ -112,6 +127,11 @@
>> 
>> static struct swsusp_info swsusp_info;
>> 
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+static u8 key[MAXKEY];
>>+static u8 iv[MAXIV];
>>+#endif
>>+
>> /*
>>  * XXX: We try to keep some more pages free so that I/O operations succeed
>>  * without paging. Might this be more?
>>@@ -130,6 +150,52 @@
>> static unsigned short swapfile_used[MAX_SWAPFILES];
>> static unsigned short root_swap;
>> 
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+static struct crypto_tfm *crypto_init(int mode)
> 
> 
> I think it's better if this function returns an int error code and the
> messages are printed where it's called from.  This way, the essential
> part of the code would be easier to grasp (Pavel?).
> 

Pavel doesn't mind printing the errors here as far as I can see. The
messages will be adapted to suspend/resume stae.

> 
>>+{
>>+	struct crypto_tfm *tfm;
>>+	int len;
>>+
>>+	tfm = crypto_alloc_tfm(CIPHER, CRYPTO_TFM_MODE_CBC);
>>+	if(!tfm) {
>>+		printk(KERN_ERR "swsusp: no tfm, suspend not possible\n");
>>+		return NULL;
>>+	}
>>+
>>+	if(sizeof(key) < crypto_tfm_alg_min_keysize(tfm)) {
>>+		printk("swsusp: key buffer too small, suspend not possible\n");
>>+		crypto_free_tfm(tfm);
>>+		return NULL;
>>+	}
>>+
>>+	if (sizeof(iv) < crypto_tfm_alg_ivsize(tfm)) {
>>+		printk("swsusp: iv buffer too small, suspend not possible\n");
>>+		crypto_free_tfm(tfm);
>>+		return NULL;
>>+	}
>>+
>>+	if (mode) {
>>+		get_random_bytes(key, MAXKEY);
> 
> 
> I hope you realize that this may give you a sequence of bits that you should
> not use as a key ...

I don't get what you mean here. As far as I know aes has no weak keys.
The only danger I can see here is that get_get_random_bytes returns a
consecutive sequence of zeroes (or some other constant value) on every
invocation. Otherwise a sequence of zeroes is just one of 2^256 possible
keys. I prefer to keep the key space as wide open as possible.
Please let me know if you did mean something different.

> 
> 
>>+		get_random_bytes(iv, MAXIV);
>>+	}
>>+
>>+	len = crypto_tfm_alg_max_keysize(tfm);
> 
> 
> You have used this value earlier.  Why don't you initialize len at that time?
> 

Not correct. Earlieron it is crypto_tfm_alg_min_keysize().
                                            ^^^
> 
>>+	if (len > sizeof(key))
>>+		len = sizeof(key);
>>+
>>+	if (crypto_cipher_setkey(tfm, key, len)) {
>>+		printk(KERN_ERR "swsusp: key setup failure, suspend not possible\n");
>>+		crypto_free_tfm(tfm);
> 
> 
> On any error, you call crypto_free_tfm(tfm) and return.  I would use a common
> error handling code, like that:
> 
> 	if (error)
> 		goto Error;
> 	/* some code */
> Error:
> 	crypto_free_tfm(tfm);
> 	return error;
> 

Will do.

> 
>>+		return NULL;
>>+	}
>>+
>>+	len = crypto_tfm_alg_blocksize(tfm);
>>+	crypto_cipher_set_iv(tfm, iv, len);
> 
> 
> I would use crypto_tfm_alg_blocksize(tfm) here directly (shorter code).
> 

I reworked this a bit. New patch will follow later.

> 
>>+
>>+	return tfm;
>>+}
>>+#endif
>>+
>> static int mark_swapfiles(swp_entry_t prev)
>> {
>> 	int error;
>>@@ -141,6 +207,10 @@
> 
> 
> If you used -p while making diffs, it would be easier to find out where the
> changes actually started.
> 

I'll try to remember.

> 
>> 	    !memcmp("SWAPSPACE2",swsusp_header.sig, 10)) {
>> 		memcpy(swsusp_header.orig_sig,swsusp_header.sig, 10);
>> 		memcpy(swsusp_header.sig,SWSUSP_SIG, 10);
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+		memcpy(swsusp_header.key, key, MAXKEY);
>>+		memcpy(swsusp_header.iv, iv, MAXIV);
>>+#endif
>> 		swsusp_header.swsusp_info = prev;
>> 		error = rw_swap_page_sync(WRITE, 
>> 					  swp_entry(root_swap, 0),
>>@@ -294,6 +364,19 @@
> 
> 
> This change will not apply to the current swsusp.c (eg as in 2.6.12-rc2).
> 

Going to have a look at 2.6.12-rc2 later today.

> 
>> 	int error = 0;
>> 	int i;
>> 	unsigned int mod = nr_copy_pages / 100;
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+	struct crypto_tfm *tfm;
>>+	struct scatterlist src, dst;
>>+
>>+	if (!(tfm = crypto_init(1)))
>>+		return -EINVAL;
> 
> 
> It's not necessarily -EINVAL, I think.  It's better to return different
> error codes on different error conditions.
> 

Different error codes are on their way.

> 
>>+
>>+	src.offset = 0;
>>+	src.length = PAGE_SIZE;
>>+	dst.page   = virt_to_page((void *)&swsusp_header);
>>+	dst.offset = 0;
>>+	dst.length = PAGE_SIZE;
>>+#endif
>> 
>> 	if (!mod)
>> 		mod = 1;
>>@@ -302,10 +385,21 @@
> 
> 
> This change will not apply to the current swsusp.c (eg as in 2.6.12-rc2).
> 

As above.

> 
>> 	for (i = 0; i < nr_copy_pages && !error; i++) {
>> 		if (!(i%mod))
>> 			printk( "\b\b\b\b%3d%%", i / mod );
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+		src.page = virt_to_page((pagedir_nosave+i)->address);
>>+		error = crypto_cipher_encrypt(tfm, &dst, &src, PAGE_SIZE);
>>+		if (!error)
>>+			error = write_page((unsigned long)&swsusp_header,
>>+					  &((pagedir_nosave+i)->swap_address));
> 
> 
> I wouldn't use swsusp_header directly in this statement.  It's a bit confusing.
> 

Hmm, I tried to solve this with a union which Pavel didn't like. An
extra buffer doesn't make sense here as it would be a lost page. Any
ideas? BTW, I need to use a buffer here as when swsusp fails later the
pages would have been encrypted in-place...

> 
>>+#else
>> 		error = write_page((pagedir_nosave+i)->address,
>> 					  &((pagedir_nosave+i)->swap_address));
>>+#endif
>> 	}
>> 	printk("\b\b\b\bdone\n");
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+	crypto_free_tfm(tfm);
>>+#endif
>> 	return error;
>> }
>> 
>>@@ -404,6 +498,10 @@
>> 	if ((error = close_swap()))
>> 		goto FreePagedir;
>>  Done:
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+	memset(key, 0, MAXKEY);
>>+	memset(iv, 0, MAXIV);
>>+#endif
>> 	return error;
>>  FreePagedir:
>> 	free_pagedir_entries();
>>@@ -1124,6 +1222,12 @@
>> 	if (!memcmp(SWSUSP_SIG, swsusp_header.sig, 10)) {
>> 		memcpy(swsusp_header.sig, swsusp_header.orig_sig, 10);
>> 
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+		memcpy(key, swsusp_header.key, MAXKEY);
>>+		memcpy(iv, swsusp_header.iv, MAXIV);
>>+		memset(swsusp_header.key, 0, MAXKEY);
>>+		memset(swsusp_header.iv, 0, MAXIV);
>>+#endif
>> 		/*
>> 		 * Reset swap signature now.
>> 		 */
>>@@ -1150,6 +1254,18 @@
> 
> 
> This change will not apply to the current swsusp.c (eg as in 2.6.12-rc2).
> 

As above.

> 
>> 	int error;
>> 	int i;
>> 	int mod = nr_copy_pages / 100;
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+	struct crypto_tfm *tfm;
>>+	struct scatterlist src, dst;
>>+
>>+	if (!(tfm = crypto_init(0)))
>>+		return -EINVAL;
> 
> 
> Same as in data_write() (ie not necessarily -EINVAL).
> 

As above, consider this done.

> 
>>+
>>+	src.offset = 0;
>>+	src.length = PAGE_SIZE;
>>+	dst.offset = 0;
>>+	dst.length = PAGE_SIZE;
>>+#endif
>> 
>> 	if (!mod)
>> 		mod = 1;
>>@@ -1163,8 +1279,18 @@
>> 			printk( "\b\b\b\b%3d%%", i / mod );
>> 		error = bio_read_page(swp_offset(p->swap_address),
>> 				  (void *)p->address);
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+		if (!error) {
>>+			src.page = dst.page = virt_to_page((void *)p->address);
>>+			error = crypto_cipher_decrypt(tfm, &dst, &src,
>>+							PAGE_SIZE);
>>+		}
>>+#endif
>> 	}
>> 	printk(" %d done.\n",i);
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+	crypto_free_tfm(tfm);
>>+#endif
>> 	return error;
>> 
>> }
>>@@ -1233,6 +1359,11 @@
>> 	} else
>> 		error = PTR_ERR(resume_bdev);
>> 
>>+#ifdef CONFIG_SWSUSP_ENCRYPT
>>+	memset(key, 0, MAXKEY);
>>+	memset(iv, 0, MAXIV);
>>+#endif
>>+
>> 	if (!error)
>> 		pr_debug("Reading resume file was successful\n");
>> 	else
> 
> 
> Greets,
> Rafael
> 
> 


-- 
Andreas Steinmetz                       SPAMmers use robotrap@xxxxxxxx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/