Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S262357AbVDLLaB (ORCPT ); Tue, 12 Apr 2005 07:30:01 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S262351AbVDLL05 (ORCPT ); Tue, 12 Apr 2005 07:26:57 -0400 Received: from hermes.domdv.de ([193.102.202.1]:43531 "EHLO hermes.domdv.de") by vger.kernel.org with ESMTP id S262270AbVDLKw0 (ORCPT ); Tue, 12 Apr 2005 06:52:26 -0400 Message-ID: <425BA85F.3030908@domdv.de> Date: Tue, 12 Apr 2005 12:52:15 +0200 From: Andreas Steinmetz User-Agent: Mozilla Thunderbird 1.0.2 (X11/20050322) X-Accept-Language: en-us, en MIME-Version: 1.0 To: "Rafael J. Wysocki" CC: Pavel Machek , Linux Kernel Mailinglist Subject: Re: [PATCH encrypted swsusp 1/3] core functionality References: <4259B474.4040407@domdv.de> <20050411110822.GA10401@elf.ucw.cz> <425AA19F.6040802@domdv.de> <200504112257.39708.rjw@sisk.pl> In-Reply-To: <200504112257.39708.rjw@sisk.pl> X-Enigmail-Version: 0.90.2.0 X-Enigmail-Supports: pgp-inline, pgp-mime Content-Type: text/plain; charset=ISO-8859-2 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9650 Lines: 399 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 : >> * Fixed runaway init >> * >>+ * Andreas Steinmetz : >>+ * 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 >>+#include >>+#include >>+#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@domdv.de - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/