lkml.org 
[lkml]   [2005]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH encrypted swsusp 1/3] core functionality
    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@go.ro>:
    >> * Fixed runaway init
    >> *
    >>+ * Andreas Steinmetz <ast@domdv.de>:
    >>+ * 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@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/

    \
     
     \ /
      Last update: 2005-04-12 14:38    [W:0.048 / U:30.452 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site