[PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

This patch series increase security of suspend and hibernate actions. It allows
user to safely wipe crypto keys before suspend and hibernate actions starts
without race conditions on userspace process with heavy I/O.

To automatically wipe cryto key for <device> before hibernate action call:
$ dmsetup message <device> 0 key wipe_on_hibernation 1

To automatically wipe cryto key for <device> before suspend action call:
$ dmsetup message <device> 0 key wipe_on_suspend 1

(Value 0 after wipe_* string reverts original behaviour - to not wipe key)

Pali Rohár (3):
  PM suspend/hibernate: Call notifier after freezing processes
  dm: Export function dm_suspend_md()
  dm-crypt: Adds support for wiping key when doing suspend/hibernation

 drivers/md/dm-crypt.c    |  109 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/md/dm.c          |    6 +++
 drivers/md/dm.h          |    5 +++
 include/linux/suspend.h  |    2 +
 kernel/power/hibernate.c |    2 +
 kernel/power/suspend.c   |    4 +-
 6 files changed, 120 insertions(+), 8 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Pali Rohár]

To prevent race conditions on userspace processes with I/O some taks must be
called after processes are freezed. This patch adds new events which are
delivered by pm_notifier_call_chain() after freezing processes when doing
suspend or hibernate action.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 include/linux/suspend.h  |    2 ++
 kernel/power/hibernate.c |    2 ++
 kernel/power/suspend.c   |    4 +++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 5efe743..bc743c8 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; }
 #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
 #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
 #define PM_POST_RESTORE		0x0006 /* Restore failed */
+#define PM_HIBERNATION_AFTER_FREEZE	0x0007 /* After hibernation freeze */
+#define PM_SUSPEND_AFTER_FREEZE		0x0008 /* After suspend freeze */
 
 extern struct mutex pm_mutex;
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 2329daa..184f7ee 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -671,6 +671,8 @@ int hibernate(void)
 	if (error)
 		goto Exit;
 
+	pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);
+
 	lock_device_hotplug();
 	/* Allocate memory management structures */
 	error = create_basic_memory_bitmaps();
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index b7d6b3a..1776938 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -268,8 +268,10 @@ static int suspend_prepare(suspend_state_t state)
 	trace_suspend_resume(TPS("freeze_processes"), 0, true);
 	error = suspend_freeze_processes();
 	trace_suspend_resume(TPS("freeze_processes"), 0, false);
-	if (!error)
+	if (!error) {
+		pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
 		return 0;
+	}
 
 	suspend_stats.failed_freeze++;
 	dpm_save_failed_step(SUSPEND_FREEZE);
-- 
1.7.9.5

Re: [PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Rafael J. Wysocki]

On Sunday, April 05, 2015 07:20:17 PM Pali Rohár wrote:
> To prevent race conditions on userspace processes with I/O some taks must be
> called after processes are freezed. This patch adds new events which are
> delivered by pm_notifier_call_chain() after freezing processes when doing
> suspend or hibernate action.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

Please don't add more notifiers.  Just call whatever you need directly from
where you need to call that.

If that is device-related, try to use device PM suspend/hibernate callbacks
instead.

> ---
>  include/linux/suspend.h  |    2 ++
>  kernel/power/hibernate.c |    2 ++
>  kernel/power/suspend.c   |    4 +++-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 5efe743..bc743c8 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; }
>  #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
>  #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
>  #define PM_POST_RESTORE		0x0006 /* Restore failed */
> +#define PM_HIBERNATION_AFTER_FREEZE	0x0007 /* After hibernation freeze */
> +#define PM_SUSPEND_AFTER_FREEZE		0x0008 /* After suspend freeze */
>  
>  extern struct mutex pm_mutex;
>  
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 2329daa..184f7ee 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -671,6 +671,8 @@ int hibernate(void)
>  	if (error)
>  		goto Exit;
>  
> +	pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);
> +
>  	lock_device_hotplug();
>  	/* Allocate memory management structures */
>  	error = create_basic_memory_bitmaps();
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index b7d6b3a..1776938 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -268,8 +268,10 @@ static int suspend_prepare(suspend_state_t state)
>  	trace_suspend_resume(TPS("freeze_processes"), 0, true);
>  	error = suspend_freeze_processes();
>  	trace_suspend_resume(TPS("freeze_processes"), 0, false);
> -	if (!error)
> +	if (!error) {
> +		pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
>  		return 0;
> +	}
>  
>  	suspend_stats.failed_freeze++;
>  	dpm_save_failed_step(SUSPEND_FREEZE);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2972 bytes --]

On Thursday 09 April 2015 02:28:41 Rafael J. Wysocki wrote:
> On Sunday, April 05, 2015 07:20:17 PM Pali Rohár wrote:
> > To prevent race conditions on userspace processes with I/O
> > some taks must be called after processes are freezed. This
> > patch adds new events which are delivered by
> > pm_notifier_call_chain() after freezing processes when
> > doing suspend or hibernate action.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Please don't add more notifiers.  Just call whatever you need
> directly from where you need to call that.
> 
> If that is device-related, try to use device PM
> suspend/hibernate callbacks instead.
> 

Hi! It is not possible to use any exiting pm notifiers! This is 
reason why I added new ones. As I wrote wiping dm crypt keys must 
be done *after* userspace processes are freezed to prevent race 
conditions...

> > ---
> > 
> >  include/linux/suspend.h  |    2 ++
> >  kernel/power/hibernate.c |    2 ++
> >  kernel/power/suspend.c   |    4 +++-
> >  3 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/suspend.h
> > b/include/linux/suspend.h index 5efe743..bc743c8 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -368,6 +368,8 @@ static inline bool
> > hibernation_available(void) { return false; }
> > 
> >  #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
> >  #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a
> >  saved image */ #define PM_POST_RESTORE		0x0006 /*
> >  Restore
> >  failed */
> > 
> > +#define PM_HIBERNATION_AFTER_FREEZE	0x0007 /* After
> > hibernation freeze */ +#define
> > PM_SUSPEND_AFTER_FREEZE		0x0008 /* After suspend freeze */
> > 
> >  extern struct mutex pm_mutex;
> > 
> > diff --git a/kernel/power/hibernate.c
> > b/kernel/power/hibernate.c index 2329daa..184f7ee 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -671,6 +671,8 @@ int hibernate(void)
> > 
> >  	if (error)
> >  	
> >  		goto Exit;
> > 
> > +	pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);
> > +
> > 
> >  	lock_device_hotplug();
> >  	/* Allocate memory management structures */
> >  	error = create_basic_memory_bitmaps();
> > 
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index b7d6b3a..1776938 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -268,8 +268,10 @@ static int
> > suspend_prepare(suspend_state_t state)
> > 
> >  	trace_suspend_resume(TPS("freeze_processes"), 0, true);
> >  	error = suspend_freeze_processes();
> >  	trace_suspend_resume(TPS("freeze_processes"), 0, false);
> > 
> > -	if (!error)
> > +	if (!error) {
> > +		pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
> > 
> >  		return 0;
> > 
> > +	}
> > 
> >  	suspend_stats.failed_freeze++;
> >  	dpm_save_failed_step(SUSPEND_FREEZE);

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Rafael J. Wysocki]

On Thursday, April 09, 2015 08:36:57 AM Pali Rohár wrote:
> 
> --nextPart2566388.gOmNIJrIqI
> Content-Type: Text/Plain;
>   charset="utf-8"
> Content-Transfer-Encoding: quoted-printable
> 
> On Thursday 09 April 2015 02:28:41 Rafael J. Wysocki wrote:
> > On Sunday, April 05, 2015 07:20:17 PM Pali Roh=C3=A1r wrote:
> > > To prevent race conditions on userspace processes with I/O
> > > some taks must be called after processes are freezed. This
> > > patch adds new events which are delivered by
> > > pm_notifier_call_chain() after freezing processes when
> > > doing suspend or hibernate action.
> > >=20
> > > Signed-off-by: Pali Roh=C3=A1r <pali.rohar@gmail.com>
> >=20
> > Please don't add more notifiers.  Just call whatever you need
> > directly from where you need to call that.
> >=20
> > If that is device-related, try to use device PM
> > suspend/hibernate callbacks instead.
> >=20
> 
> Hi! It is not possible to use any exiting pm notifiers! This is=20
> reason why I added new ones. As I wrote wiping dm crypt keys must=20
> be done *after* userspace processes are freezed to prevent race=20
> conditions...

I'm not talking about using the existing notifiers.  I'm talking about
calling the function you need to call directly from a suitable place
in the system suspend code.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1776 bytes --]

On Thursday 09 April 2015 19:13:55 Rafael J. Wysocki wrote:
> On Thursday, April 09, 2015 08:36:57 AM Pali Rohár wrote:
> > --nextPart2566388.gOmNIJrIqI
> > Content-Type: Text/Plain;
> > 
> >   charset="utf-8"
> > 
> > Content-Transfer-Encoding: quoted-printable
> > 
> > On Thursday 09 April 2015 02:28:41 Rafael J. Wysocki wrote:
> > > On Sunday, April 05, 2015 07:20:17 PM Pali Roh=C3=A1r
> > > wrote:
> > > > To prevent race conditions on userspace processes with
> > > > I/O some taks must be called after processes are
> > > > freezed. This patch adds new events which are delivered
> > > > by
> > > > pm_notifier_call_chain() after freezing processes when
> > > > doing suspend or hibernate action.
> > > >
> > > >=20
> > > >
> > > > Signed-off-by: Pali Roh=C3=A1r <pali.rohar@gmail.com>
> > >
> > >=20
> > >
> > > Please don't add more notifiers.  Just call whatever you
> > > need directly from where you need to call that.
> > >
> > >=20
> > >
> > > If that is device-related, try to use device PM
> > > suspend/hibernate callbacks instead.
> > >
> > >=20
> > 
> > Hi! It is not possible to use any exiting pm notifiers! This
> > is=20 reason why I added new ones. As I wrote wiping dm
> > crypt keys must=20 be done *after* userspace processes are
> > freezed to prevent race=20 conditions...
> 
> I'm not talking about using the existing notifiers.  I'm
> talking about calling the function you need to call directly
> from a suitable place in the system suspend code.

I need to wipe crypto keys from dm-crypt module. That module can 
be compiled as external .ko file and so kernel cannot call 
directly needed function. This is reason why I'm adding new 
notifier event.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/3] dm: Export function dm_suspend_md()

[Pali Rohár]

This patch exports function dm_suspend_md() which suspend mapped device so other
kernel drivers can use it and could suspend mapped device when needed.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/md/dm.c |    6 ++++++
 drivers/md/dm.h |    5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 8001fe9..919ce95 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -3053,6 +3053,12 @@ out:
 	return r;
 }
 
+int dm_suspend_md(struct mapped_device *md)
+{
+	return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
+}
+EXPORT_SYMBOL_GPL(dm_suspend_md);
+
 /*
  * Internal suspend/resume works like userspace-driven suspend. It waits
  * until all bios finish and prevents issuing new bios to the target drivers.
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 59f53e7..623c9a8 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -152,6 +152,11 @@ int dm_test_deferred_remove_flag(struct mapped_device *md);
 void dm_deferred_remove(void);
 
 /*
+ * Suspend mapped_device
+ */
+int dm_suspend_md(struct mapped_device *md);
+
+/*
  * The device-mapper can be driven through one of two interfaces;
  * ioctl or filesystem, depending which patch you have applied.
  */
-- 
1.7.9.5

[PATCH 3/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

This patch adds dm message commands and option strings to optionally wipe key
from dm-crypt device before entering suspend or hibernate state.

Before key is wiped dm device must be suspended. To prevent race conditions with
I/O and userspace processes, wiping action must be called after processes are
freezed. Otherwise userspace processes could start reading/writing to disk after
dm device is suspened and freezing processes before suspend/hibernate action
will fail.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/md/dm-crypt.c |  109 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 102 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 713a962..9b02824 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -23,6 +23,7 @@
 #include <linux/atomic.h>
 #include <linux/scatterlist.h>
 #include <linux/rbtree.h>
+#include <linux/suspend.h>
 #include <asm/page.h>
 #include <asm/unaligned.h>
 #include <crypto/hash.h>
@@ -31,6 +32,8 @@
 
 #include <linux/device-mapper.h>
 
+#include "dm.h"
+
 #define DM_MSG_PREFIX "crypt"
 
 /*
@@ -112,13 +115,18 @@ struct iv_tcw_private {
  * and encrypts / decrypts at the same time.
  */
 enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
-	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD };
+	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
+	     DM_CRYPT_KEY_WIPE_ON_HIBERNATION,
+	     DM_CRYPT_KEY_WIPE_ON_SUSPEND,
+};
 
 /*
  * The fields in here must be read only after initialization.
  */
 struct crypt_config {
 	struct dm_dev *dev;
+	struct dm_target *ti;
+	struct list_head entry;
 	sector_t start;
 
 	/*
@@ -181,6 +189,9 @@ struct crypt_config {
 
 #define MIN_IOS        16
 
+static LIST_HEAD(crypt_list);
+static DEFINE_MUTEX(crypt_list_mtx);
+
 static void clone_init(struct dm_crypt_io *, struct bio *);
 static void kcryptd_queue_crypt(struct dm_crypt_io *io);
 static u8 *iv_of_dmreq(struct crypt_config *cc, struct dm_crypt_request *dmreq);
@@ -1497,12 +1508,26 @@ out:
 
 static int crypt_wipe_key(struct crypt_config *cc)
 {
+	int ret;
+
+	if (cc->iv_gen_ops && cc->iv_gen_ops->wipe) {
+		ret = cc->iv_gen_ops->wipe(cc);
+		if (ret)
+			return ret;
+	}
+
 	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 	memset(&cc->key, 0, cc->key_size * sizeof(u8));
 
 	return crypt_setkey_allcpus(cc);
 }
 
+static void crypt_suspend_and_wipe_key(struct crypt_config *cc)
+{
+	dm_suspend_md(dm_table_get_md(cc->ti->table));
+	crypt_wipe_key(cc);
+}
+
 static void crypt_dtr(struct dm_target *ti)
 {
 	struct crypt_config *cc = ti->private;
@@ -1512,6 +1537,10 @@ static void crypt_dtr(struct dm_target *ti)
 	if (!cc)
 		return;
 
+	mutex_lock(&crypt_list_mtx);
+	list_del(&cc->entry);
+	mutex_unlock(&crypt_list_mtx);
+
 	if (cc->write_thread)
 		kthread_stop(cc->write_thread);
 
@@ -1738,6 +1767,7 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	cc->key_size = key_size;
 
 	ti->private = cc;
+	cc->ti = ti;
 	ret = crypt_ctr_cipher(ti, argv[0], argv[1]);
 	if (ret < 0)
 		goto bad;
@@ -1832,7 +1862,14 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 			else if (!strcasecmp(opt_string, "submit_from_crypt_cpus"))
 				set_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
 
+			else if (!strcasecmp(opt_string, "key_wipe_on_hibernation"))
+				set_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags);
+
+			else if (!strcasecmp(opt_string, "key_wipe_on_suspend"))
+				set_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags);
+
 			else {
+				ret = -EINVAL;
 				ti->error = "Invalid feature arguments";
 				goto bad;
 			}
@@ -1871,6 +1908,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->num_flush_bios = 1;
 	ti->discard_zeroes_data_unsupported = true;
 
+	mutex_lock(&crypt_list_mtx);
+	list_add(&cc->entry, &crypt_list);
+	mutex_unlock(&crypt_list_mtx);
+
 	return 0;
 
 bad:
@@ -1979,6 +2020,8 @@ static void crypt_resume(struct dm_target *ti)
 /* Message interface
  *	key set <key>
  *	key wipe
+ *	key wipe_on_hibernation <0|1>
+ *	key wipe_on_suspend <0|1>
  */
 static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 {
@@ -1989,6 +2032,30 @@ static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 		goto error;
 
 	if (!strcasecmp(argv[0], "key")) {
+		if (argc == 3 && !strcasecmp(argv[1], "wipe_on_hibernation")) {
+			if (!strcmp(argv[2], "1")) {
+				set_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags);
+				return 0;
+			} else if (!strcmp(argv[2], "0")) {
+				clear_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags);
+				return 0;
+			} else {
+				DMWARN("unrecognised message received.");
+				return -EINVAL;
+			}
+		}
+		if (argc == 3 && !strcasecmp(argv[1], "wipe_on_suspend")) {
+			if (!strcmp(argv[2], "1")) {
+				set_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags);
+				return 0;
+			} else if (!strcmp(argv[2], "0")) {
+				clear_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags);
+				return 0;
+			} else {
+				DMWARN("unrecognised message received.");
+				return -EINVAL;
+			}
+		}
 		if (!test_bit(DM_CRYPT_SUSPENDED, &cc->flags)) {
 			DMWARN("not suspended during key manipulation.");
 			return -EINVAL;
@@ -2002,11 +2069,6 @@ static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 			return ret;
 		}
 		if (argc == 2 && !strcasecmp(argv[1], "wipe")) {
-			if (cc->iv_gen_ops && cc->iv_gen_ops->wipe) {
-				ret = cc->iv_gen_ops->wipe(cc);
-				if (ret)
-					return ret;
-			}
 			return crypt_wipe_key(cc);
 		}
 	}
@@ -2055,19 +2117,52 @@ static struct target_type crypt_target = {
 	.iterate_devices = crypt_iterate_devices,
 };
 
+static int dm_crypt_pm_notifier_call(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	struct crypt_config *cc;
+
+	mutex_lock(&crypt_list_mtx);
+
+	list_for_each_entry(cc, &crypt_list, entry) {
+		if ((action == PM_HIBERNATION_AFTER_FREEZE &&
+		     test_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags)) ||
+		    (action == PM_SUSPEND_AFTER_FREEZE &&
+		     test_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags))) {
+			crypt_suspend_and_wipe_key(cc);
+		}
+	}
+
+	mutex_unlock(&crypt_list_mtx);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block dm_crypt_pm_notifier_block = {
+	.notifier_call = dm_crypt_pm_notifier_call,
+};
+
 static int __init dm_crypt_init(void)
 {
 	int r;
 
 	r = dm_register_target(&crypt_target);
-	if (r < 0)
+	if (r < 0) {
 		DMERR("register failed %d", r);
+		return r;
+	}
+
+	r = register_pm_notifier(&dm_crypt_pm_notifier_block);
+	if (r) {
+		DMWARN("register_pm_notifier failed %d", r);
+	}
 
 	return r;
 }
 
 static void __exit dm_crypt_exit(void)
 {
+	unregister_pm_notifier(&dm_crypt_pm_notifier_block);
 	dm_unregister_target(&crypt_target);
 }
 
-- 
1.7.9.5

Re: [dm-devel] [PATCH 3/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Alasdair G Kergon]

On Sun, Apr 05, 2015 at 07:20:19PM +0200, Pali Rohár wrote:
> This patch adds dm message commands and option strings to optionally wipe key
> from dm-crypt device before entering suspend or hibernate state.
 
Try to avoid 0/1 - use descriptive options instead. 
E.g. key wipe_on_hibernation / key retain_on_hibernation  (message)
     wipe_key_on_hiberation ('dmsetup table' - don't forget the reporting interface!)

Have you tested against every state the driver might be in at the time of
suspend/hibernation?

> +static void crypt_suspend_and_wipe_key(struct crypt_config *cc)
> +{
> +	dm_suspend_md(dm_table_get_md(cc->ti->table));

I'm not particularly keen on this - silently ignoring expected error states
like -EINVAL rather than checking first and not calling the function at all
when it's known not to be needed.

Alasdair

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Mike Snitzer]

On Sun, Apr 05 2015 at  1:20pm -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:

> This patch series increase security of suspend and hibernate actions. It allows
> user to safely wipe crypto keys before suspend and hibernate actions starts
> without race conditions on userspace process with heavy I/O.
> 
> To automatically wipe cryto key for <device> before hibernate action call:
> $ dmsetup message <device> 0 key wipe_on_hibernation 1
> 
> To automatically wipe cryto key for <device> before suspend action call:
> $ dmsetup message <device> 0 key wipe_on_suspend 1
> 
> (Value 0 after wipe_* string reverts original behaviour - to not wipe key)

Can you elaborate on the attack vector your changes are meant to protect
against?  The user already authorized access, why is it inherently
dangerous to _not_ wipe the associated key across these events?

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pavel Machek]

On Mon 2015-04-06 09:00:46, Mike Snitzer wrote:
> On Sun, Apr 05 2015 at  1:20pm -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
> 
> > This patch series increase security of suspend and hibernate actions. It allows
> > user to safely wipe crypto keys before suspend and hibernate actions starts
> > without race conditions on userspace process with heavy I/O.
> > 
> > To automatically wipe cryto key for <device> before hibernate action call:
> > $ dmsetup message <device> 0 key wipe_on_hibernation 1
> > 
> > To automatically wipe cryto key for <device> before suspend action call:
> > $ dmsetup message <device> 0 key wipe_on_suspend 1
> > 
> > (Value 0 after wipe_* string reverts original behaviour - to not wipe key)
> 
> Can you elaborate on the attack vector your changes are meant to protect
> against?  The user already authorized access, why is it inherently
> dangerous to _not_ wipe the associated key across these events?

Umm. You are using your notebook. It is unlikely to be stolen at that
point. You close the lid and board the airplane, stowing it in
overhead bin. There's much better chance of notebook being stolen now.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Mike Snitzer]

On Mon, Apr 06 2015 at  9:25am -0400,
Pavel Machek <pavel@ucw.cz> wrote:

> On Mon 2015-04-06 09:00:46, Mike Snitzer wrote:
> > On Sun, Apr 05 2015 at  1:20pm -0400,
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > 
> > > This patch series increase security of suspend and hibernate actions. It allows
> > > user to safely wipe crypto keys before suspend and hibernate actions starts
> > > without race conditions on userspace process with heavy I/O.
> > > 
> > > To automatically wipe cryto key for <device> before hibernate action call:
> > > $ dmsetup message <device> 0 key wipe_on_hibernation 1
> > > 
> > > To automatically wipe cryto key for <device> before suspend action call:
> > > $ dmsetup message <device> 0 key wipe_on_suspend 1
> > > 
> > > (Value 0 after wipe_* string reverts original behaviour - to not wipe key)
> > 
> > Can you elaborate on the attack vector your changes are meant to protect
> > against?  The user already authorized access, why is it inherently
> > dangerous to _not_ wipe the associated key across these events?
> 
> Umm. You are using your notebook. It is unlikely to be stolen at that
> point. You close the lid and board the airplane, stowing it in
> overhead bin. There's much better chance of notebook being stolen now.

Yes, pretty straight forward but the thief would need to then login upon
resume (at least with most common desktop configs)... the barrier then
is only the strength of the user's password and not the crypt
passphrase.

Why wipe crypto keys during suspend (was Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation)

[Pavel Machek]

On Mon 2015-04-06 16:51:45, Mike Snitzer wrote:
> On Mon, Apr 06 2015 at  9:25am -0400,
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > On Mon 2015-04-06 09:00:46, Mike Snitzer wrote:
> > > On Sun, Apr 05 2015 at  1:20pm -0400,
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > 
> > > > This patch series increase security of suspend and hibernate actions. It allows
> > > > user to safely wipe crypto keys before suspend and hibernate actions starts
> > > > without race conditions on userspace process with heavy I/O.
> > > > 
> > > > To automatically wipe cryto key for <device> before hibernate action call:
> > > > $ dmsetup message <device> 0 key wipe_on_hibernation 1
> > > > 
> > > > To automatically wipe cryto key for <device> before suspend action call:
> > > > $ dmsetup message <device> 0 key wipe_on_suspend 1
> > > > 
> > > > (Value 0 after wipe_* string reverts original behaviour - to not wipe key)
> > > 
> > > Can you elaborate on the attack vector your changes are meant to protect
> > > against?  The user already authorized access, why is it inherently
> > > dangerous to _not_ wipe the associated key across these events?
> > 
> > Umm. You are using your notebook. It is unlikely to be stolen at that
> > point. You close the lid and board the airplane, stowing it in
> > overhead bin. There's much better chance of notebook being stolen now.
> 
> Yes, pretty straight forward but the thief would need to then login upon
> resume (at least with most common desktop configs)... the barrier then
> is only the strength of the user's password and not the crypt
> passphrase.

Why would he want to do that? :-).

No; at that point, attacker would either wait for something remotely
exploitable to exploit, or attach JTAG debugger to the machine, or use
liquid nitrogen on RAMs and then attach them to running machine.

Yes, it is better when keys are not on your machine when it is stolen.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3187 bytes --]

On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> On Sun, Apr 05 2015 at  1:20pm -0400,
> 
> Pali Rohár <pali.rohar@gmail.com> wrote:
> > This patch series increase security of suspend and hibernate
> > actions. It allows user to safely wipe crypto keys before
> > suspend and hibernate actions starts without race
> > conditions on userspace process with heavy I/O.
> > 
> > To automatically wipe cryto key for <device> before
> > hibernate action call: $ dmsetup message <device> 0 key
> > wipe_on_hibernation 1
> > 
> > To automatically wipe cryto key for <device> before suspend
> > action call: $ dmsetup message <device> 0 key
> > wipe_on_suspend 1
> > 
> > (Value 0 after wipe_* string reverts original behaviour - to
> > not wipe key)
> 
> Can you elaborate on the attack vector your changes are meant
> to protect against?  The user already authorized access, why
> is it inherently dangerous to _not_ wipe the associated key
> across these events?

Hi,

yes, I will try to explain current problems with cryptsetup 
luksSuspend command and hibernation.

First, sometimes it is needed to put machine into other hands. 
You can still watch other person what is doing with machine, but 
once if you let machine unlocked (e.g opened luks disk), she/he 
can access encrypted data.

If you turn off machine, it could be safe, because luks disk 
devices are locked. But if you enter machine into suspend or 
hibernate state luks devices are still open. And my patches try 
to achieve similar security as when machine is off (= no crypto 
keys in RAM or on swap).

When doing hibernate on unencrypted swap it is to prevent leaking 
crypto keys to hibernate image (which is stored in swap).

When doing suspend action it is again to prevent leaking crypto 
keys. E.g when you suspend laptop and put it off (somebody can 
remove RAMs and do some cold boot attack).

The most common situation is:
You have mounted partition from dm-crypt device (e.g. /home/), 
some userspace processes access it (e.g opened firefox which 
still reads/writes to cache ~/.firefox/) and you want to drop 
crypto keys from kernel for some time.

For that operation there is command cryptsetup luksSuspend, which 
suspend dm device and then tell kernel to wipe crypto keys. All 
I/O operations are then stopped and userspace processes which 
want to do some those I/O operations are stopped too (until you 
call cryptsetup luksResume and enter correct key).

Now if you want to suspend/hiberate your machine (when some of dm 
devices are suspeneded and some processes are stopped due to 
pending I/O) it is not possible. Kernel freeze_processes function 
will fail because userspace processes are still stopped inside 
some I/O syscall (read/write, etc,...).

My patches fixes this problem and do those operations (suspend dm 
device, wipe crypto keys, enter suspend/hiberate) in correct 
order and without race condition.

dm device is suspended *after* userspace processes are freezed 
and after that are crypto keys wiped. And then computer/laptop 
enters into suspend/hibernate state.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pavel Machek]

On Mon 2015-04-06 15:29:57, Pali Rohár wrote:
> On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > On Sun, Apr 05 2015 at  1:20pm -0400,
> > 
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > This patch series increase security of suspend and hibernate
> > > actions. It allows user to safely wipe crypto keys before
> > > suspend and hibernate actions starts without race
> > > conditions on userspace process with heavy I/O.
> > > 
> > > To automatically wipe cryto key for <device> before
> > > hibernate action call: $ dmsetup message <device> 0 key
> > > wipe_on_hibernation 1
> > > 
> > > To automatically wipe cryto key for <device> before suspend
> > > action call: $ dmsetup message <device> 0 key
> > > wipe_on_suspend 1
> > > 
> > > (Value 0 after wipe_* string reverts original behaviour - to
> > > not wipe key)
> > 
> > Can you elaborate on the attack vector your changes are meant
> > to protect against?  The user already authorized access, why
> > is it inherently dangerous to _not_ wipe the associated key
> > across these events?
> 
> Hi,
> 
> yes, I will try to explain current problems with cryptsetup 
> luksSuspend command and hibernation.
> 
> First, sometimes it is needed to put machine into other hands. 
> You can still watch other person what is doing with machine, but 
> once if you let machine unlocked (e.g opened luks disk), she/he 
> can access encrypted data.
> 
> If you turn off machine, it could be safe, because luks disk 
> devices are locked. But if you enter machine into suspend or 
> hibernate state luks devices are still open. And my patches try 
> to achieve similar security as when machine is off (= no crypto 
> keys in RAM or on swap).
> 
> When doing hibernate on unencrypted swap it is to prevent leaking 
> crypto keys to hibernate image (which is stored in swap).
> 
> When doing suspend action it is again to prevent leaking crypto 
> keys. E.g when you suspend laptop and put it off (somebody can 
> remove RAMs and do some cold boot attack).
> 
> The most common situation is:
> You have mounted partition from dm-crypt device (e.g. /home/), 
> some userspace processes access it (e.g opened firefox which 
> still reads/writes to cache ~/.firefox/) and you want to drop 
> crypto keys from kernel for some time.
> 
> For that operation there is command cryptsetup luksSuspend, which 
> suspend dm device and then tell kernel to wipe crypto keys. All 
> I/O operations are then stopped and userspace processes which 
> want to do some those I/O operations are stopped too (until you 
> call cryptsetup luksResume and enter correct key).

Actually... is the list of sites where the process wait small enough?
Could we modify them to be freezeable? Suspend should work even if
user stopped the his crypto partitions...

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3337 bytes --]

On Monday 06 April 2015 20:17:38 Pavel Machek wrote:
> On Mon 2015-04-06 15:29:57, Pali Rohár wrote:
> > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > On Sun, Apr 05 2015 at  1:20pm -0400,
> > > 
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > This patch series increase security of suspend and
> > > > hibernate actions. It allows user to safely wipe crypto
> > > > keys before suspend and hibernate actions starts
> > > > without race conditions on userspace process with heavy
> > > > I/O.
> > > > 
> > > > To automatically wipe cryto key for <device> before
> > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > wipe_on_hibernation 1
> > > > 
> > > > To automatically wipe cryto key for <device> before
> > > > suspend action call: $ dmsetup message <device> 0 key
> > > > wipe_on_suspend 1
> > > > 
> > > > (Value 0 after wipe_* string reverts original behaviour
> > > > - to not wipe key)
> > > 
> > > Can you elaborate on the attack vector your changes are
> > > meant to protect against?  The user already authorized
> > > access, why is it inherently dangerous to _not_ wipe the
> > > associated key across these events?
> > 
> > Hi,
> > 
> > yes, I will try to explain current problems with cryptsetup
> > luksSuspend command and hibernation.
> > 
> > First, sometimes it is needed to put machine into other
> > hands. You can still watch other person what is doing with
> > machine, but once if you let machine unlocked (e.g opened
> > luks disk), she/he can access encrypted data.
> > 
> > If you turn off machine, it could be safe, because luks disk
> > devices are locked. But if you enter machine into suspend or
> > hibernate state luks devices are still open. And my patches
> > try to achieve similar security as when machine is off (=
> > no crypto keys in RAM or on swap).
> > 
> > When doing hibernate on unencrypted swap it is to prevent
> > leaking crypto keys to hibernate image (which is stored in
> > swap).
> > 
> > When doing suspend action it is again to prevent leaking
> > crypto keys. E.g when you suspend laptop and put it off
> > (somebody can remove RAMs and do some cold boot attack).
> > 
> > The most common situation is:
> > You have mounted partition from dm-crypt device (e.g.
> > /home/), some userspace processes access it (e.g opened
> > firefox which still reads/writes to cache ~/.firefox/) and
> > you want to drop crypto keys from kernel for some time.
> > 
> > For that operation there is command cryptsetup luksSuspend,
> > which suspend dm device and then tell kernel to wipe crypto
> > keys. All I/O operations are then stopped and userspace
> > processes which want to do some those I/O operations are
> > stopped too (until you call cryptsetup luksResume and enter
> > correct key).
> 
> Actually... is the list of sites where the process wait small
> enough? Could we modify them to be freezeable? Suspend should
> work even if user stopped the his crypto partitions...
> 
> 								Pavel

If you suspend dm device and then you want to read file from fs 
which is on that device, then process freeze and you even cannot 
kill it with SIGKILL. Before entering suspend kernel tries to do 
sync and that operation also fails...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Mike Snitzer]

On Mon, Apr 06 2015 at  9:29am -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:

> On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > On Sun, Apr 05 2015 at  1:20pm -0400,
> > 
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > This patch series increase security of suspend and hibernate
> > > actions. It allows user to safely wipe crypto keys before
> > > suspend and hibernate actions starts without race
> > > conditions on userspace process with heavy I/O.
> > > 
> > > To automatically wipe cryto key for <device> before
> > > hibernate action call: $ dmsetup message <device> 0 key
> > > wipe_on_hibernation 1
> > > 
> > > To automatically wipe cryto key for <device> before suspend
> > > action call: $ dmsetup message <device> 0 key
> > > wipe_on_suspend 1
> > > 
> > > (Value 0 after wipe_* string reverts original behaviour - to
> > > not wipe key)
> > 
> > Can you elaborate on the attack vector your changes are meant
> > to protect against?  The user already authorized access, why
> > is it inherently dangerous to _not_ wipe the associated key
> > across these events?
> 
> Hi,
> 
> yes, I will try to explain current problems with cryptsetup 
> luksSuspend command and hibernation.
> 
> First, sometimes it is needed to put machine into other hands. 
> You can still watch other person what is doing with machine, but 
> once if you let machine unlocked (e.g opened luks disk), she/he 
> can access encrypted data.
> 
> If you turn off machine, it could be safe, because luks disk 
> devices are locked. But if you enter machine into suspend or 
> hibernate state luks devices are still open. And my patches try 
> to achieve similar security as when machine is off (= no crypto 
> keys in RAM or on swap).
> 
> When doing hibernate on unencrypted swap it is to prevent leaking 
> crypto keys to hibernate image (which is stored in swap).
> 
> When doing suspend action it is again to prevent leaking crypto 
> keys. E.g when you suspend laptop and put it off (somebody can 
> remove RAMs and do some cold boot attack).
> 
> The most common situation is:
> You have mounted partition from dm-crypt device (e.g. /home/), 
> some userspace processes access it (e.g opened firefox which 
> still reads/writes to cache ~/.firefox/) and you want to drop 
> crypto keys from kernel for some time.
> 
> For that operation there is command cryptsetup luksSuspend, which 
> suspend dm device and then tell kernel to wipe crypto keys. All 
> I/O operations are then stopped and userspace processes which 
> want to do some those I/O operations are stopped too (until you 
> call cryptsetup luksResume and enter correct key).
> 
> Now if you want to suspend/hiberate your machine (when some of dm 
> devices are suspeneded and some processes are stopped due to 
> pending I/O) it is not possible. Kernel freeze_processes function 
> will fail because userspace processes are still stopped inside 
> some I/O syscall (read/write, etc,...).
> 
> My patches fixes this problem and do those operations (suspend dm 
> device, wipe crypto keys, enter suspend/hiberate) in correct 
> order and without race condition.
> 
> dm device is suspended *after* userspace processes are freezed 
> and after that are crypto keys wiped. And then computer/laptop 
> enters into suspend/hibernate state.

Wouldn't it be better to fix freeze_processes() to be tolerant of
processes that are hung as a side-effect of their backing storage being
suspended?  A hibernate shouldn't fail simply because a user chose to
suspend a DM device.

Then this entire problem goes away and the key can be wiped from
userspace (like you said above).

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> On Mon, Apr 06 2015 at  9:29am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
> 
> > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > On Sun, Apr 05 2015 at  1:20pm -0400,
> > > 
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > This patch series increase security of suspend and hibernate
> > > > actions. It allows user to safely wipe crypto keys before
> > > > suspend and hibernate actions starts without race
> > > > conditions on userspace process with heavy I/O.
> > > > 
> > > > To automatically wipe cryto key for <device> before
> > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > wipe_on_hibernation 1
> > > > 
> > > > To automatically wipe cryto key for <device> before suspend
> > > > action call: $ dmsetup message <device> 0 key
> > > > wipe_on_suspend 1
> > > > 
> > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > not wipe key)
> > > 
> > > Can you elaborate on the attack vector your changes are meant
> > > to protect against?  The user already authorized access, why
> > > is it inherently dangerous to _not_ wipe the associated key
> > > across these events?
> > 
> > Hi,
> > 
> > yes, I will try to explain current problems with cryptsetup 
> > luksSuspend command and hibernation.
> > 
> > First, sometimes it is needed to put machine into other hands. 
> > You can still watch other person what is doing with machine, but 
> > once if you let machine unlocked (e.g opened luks disk), she/he 
> > can access encrypted data.
> > 
> > If you turn off machine, it could be safe, because luks disk 
> > devices are locked. But if you enter machine into suspend or 
> > hibernate state luks devices are still open. And my patches try 
> > to achieve similar security as when machine is off (= no crypto 
> > keys in RAM or on swap).
> > 
> > When doing hibernate on unencrypted swap it is to prevent leaking 
> > crypto keys to hibernate image (which is stored in swap).
> > 
> > When doing suspend action it is again to prevent leaking crypto 
> > keys. E.g when you suspend laptop and put it off (somebody can 
> > remove RAMs and do some cold boot attack).
> > 
> > The most common situation is:
> > You have mounted partition from dm-crypt device (e.g. /home/), 
> > some userspace processes access it (e.g opened firefox which 
> > still reads/writes to cache ~/.firefox/) and you want to drop 
> > crypto keys from kernel for some time.
> > 
> > For that operation there is command cryptsetup luksSuspend, which 
> > suspend dm device and then tell kernel to wipe crypto keys. All 
> > I/O operations are then stopped and userspace processes which 
> > want to do some those I/O operations are stopped too (until you 
> > call cryptsetup luksResume and enter correct key).
> > 
> > Now if you want to suspend/hiberate your machine (when some of dm 
> > devices are suspeneded and some processes are stopped due to 
> > pending I/O) it is not possible. Kernel freeze_processes function 
> > will fail because userspace processes are still stopped inside 
> > some I/O syscall (read/write, etc,...).
> > 
> > My patches fixes this problem and do those operations (suspend dm 
> > device, wipe crypto keys, enter suspend/hiberate) in correct 
> > order and without race condition.
> > 
> > dm device is suspended *after* userspace processes are freezed 
> > and after that are crypto keys wiped. And then computer/laptop 
> > enters into suspend/hibernate state.
> 
> Wouldn't it be better to fix freeze_processes() to be tolerant of
> processes that are hung as a side-effect of their backing storage being
> suspended?  A hibernate shouldn't fail simply because a user chose to
> suspend a DM device.
> 
> Then this entire problem goes away and the key can be wiped from
> userspace (like you said above).

Still there will be race condition. Before hibernation (and device
poweroff) we should have synced disks and filesystems to prevent data
lose (or other damage) as more as we can. And if there will be some
application which using lot of I/O (e.g normal firefox) then there
always will be race condtion.

So proper way is to wipe luks crypto keys *after* userspace processes
are freezed.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Mike Snitzer]

On Thu, Apr 09 2015 at  9:28am -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:

> On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> > On Mon, Apr 06 2015 at  9:29am -0400,
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > 
> > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > > On Sun, Apr 05 2015 at  1:20pm -0400,
> > > > 
> > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > This patch series increase security of suspend and hibernate
> > > > > actions. It allows user to safely wipe crypto keys before
> > > > > suspend and hibernate actions starts without race
> > > > > conditions on userspace process with heavy I/O.
> > > > > 
> > > > > To automatically wipe cryto key for <device> before
> > > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > > wipe_on_hibernation 1
> > > > > 
> > > > > To automatically wipe cryto key for <device> before suspend
> > > > > action call: $ dmsetup message <device> 0 key
> > > > > wipe_on_suspend 1
> > > > > 
> > > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > > not wipe key)
> > > > 
> > > > Can you elaborate on the attack vector your changes are meant
> > > > to protect against?  The user already authorized access, why
> > > > is it inherently dangerous to _not_ wipe the associated key
> > > > across these events?
> > > 
> > > Hi,
> > > 
> > > yes, I will try to explain current problems with cryptsetup 
> > > luksSuspend command and hibernation.
> > > 
> > > First, sometimes it is needed to put machine into other hands. 
> > > You can still watch other person what is doing with machine, but 
> > > once if you let machine unlocked (e.g opened luks disk), she/he 
> > > can access encrypted data.
> > > 
> > > If you turn off machine, it could be safe, because luks disk 
> > > devices are locked. But if you enter machine into suspend or 
> > > hibernate state luks devices are still open. And my patches try 
> > > to achieve similar security as when machine is off (= no crypto 
> > > keys in RAM or on swap).
> > > 
> > > When doing hibernate on unencrypted swap it is to prevent leaking 
> > > crypto keys to hibernate image (which is stored in swap).
> > > 
> > > When doing suspend action it is again to prevent leaking crypto 
> > > keys. E.g when you suspend laptop and put it off (somebody can 
> > > remove RAMs and do some cold boot attack).
> > > 
> > > The most common situation is:
> > > You have mounted partition from dm-crypt device (e.g. /home/), 
> > > some userspace processes access it (e.g opened firefox which 
> > > still reads/writes to cache ~/.firefox/) and you want to drop 
> > > crypto keys from kernel for some time.
> > > 
> > > For that operation there is command cryptsetup luksSuspend, which 
> > > suspend dm device and then tell kernel to wipe crypto keys. All 
> > > I/O operations are then stopped and userspace processes which 
> > > want to do some those I/O operations are stopped too (until you 
> > > call cryptsetup luksResume and enter correct key).
> > > 
> > > Now if you want to suspend/hiberate your machine (when some of dm 
> > > devices are suspeneded and some processes are stopped due to 
> > > pending I/O) it is not possible. Kernel freeze_processes function 
> > > will fail because userspace processes are still stopped inside 
> > > some I/O syscall (read/write, etc,...).
> > > 
> > > My patches fixes this problem and do those operations (suspend dm 
> > > device, wipe crypto keys, enter suspend/hiberate) in correct 
> > > order and without race condition.
> > > 
> > > dm device is suspended *after* userspace processes are freezed 
> > > and after that are crypto keys wiped. And then computer/laptop 
> > > enters into suspend/hibernate state.
> > 
> > Wouldn't it be better to fix freeze_processes() to be tolerant of
> > processes that are hung as a side-effect of their backing storage being
> > suspended?  A hibernate shouldn't fail simply because a user chose to
> > suspend a DM device.
> > 
> > Then this entire problem goes away and the key can be wiped from
> > userspace (like you said above).
> 
> Still there will be race condition. Before hibernation (and device
> poweroff) we should have synced disks and filesystems to prevent data
> lose (or other damage) as more as we can. And if there will be some
> application which using lot of I/O (e.g normal firefox) then there
> always will be race condtion.

The DM suspend will take care of flushing any pending I/O.  So I don't
see where the supposed race is...

Anything else that is trapped in userspace memory will be there when the
machine resumes.

> So proper way is to wipe luks crypto keys *after* userspace processes
> are freezed.

I know you believe that I'm just not accepting that at face value.

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

On Thursday 09 April 2015 10:08:43 Mike Snitzer wrote:
> On Thu, Apr 09 2015 at  9:28am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
> 
> > On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> > > On Mon, Apr 06 2015 at  9:29am -0400,
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > 
> > > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > > > On Sun, Apr 05 2015 at  1:20pm -0400,
> > > > > 
> > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > This patch series increase security of suspend and hibernate
> > > > > > actions. It allows user to safely wipe crypto keys before
> > > > > > suspend and hibernate actions starts without race
> > > > > > conditions on userspace process with heavy I/O.
> > > > > > 
> > > > > > To automatically wipe cryto key for <device> before
> > > > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > > > wipe_on_hibernation 1
> > > > > > 
> > > > > > To automatically wipe cryto key for <device> before suspend
> > > > > > action call: $ dmsetup message <device> 0 key
> > > > > > wipe_on_suspend 1
> > > > > > 
> > > > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > > > not wipe key)
> > > > > 
> > > > > Can you elaborate on the attack vector your changes are meant
> > > > > to protect against?  The user already authorized access, why
> > > > > is it inherently dangerous to _not_ wipe the associated key
> > > > > across these events?
> > > > 
> > > > Hi,
> > > > 
> > > > yes, I will try to explain current problems with cryptsetup 
> > > > luksSuspend command and hibernation.
> > > > 
> > > > First, sometimes it is needed to put machine into other hands. 
> > > > You can still watch other person what is doing with machine, but 
> > > > once if you let machine unlocked (e.g opened luks disk), she/he 
> > > > can access encrypted data.
> > > > 
> > > > If you turn off machine, it could be safe, because luks disk 
> > > > devices are locked. But if you enter machine into suspend or 
> > > > hibernate state luks devices are still open. And my patches try 
> > > > to achieve similar security as when machine is off (= no crypto 
> > > > keys in RAM or on swap).
> > > > 
> > > > When doing hibernate on unencrypted swap it is to prevent leaking 
> > > > crypto keys to hibernate image (which is stored in swap).
> > > > 
> > > > When doing suspend action it is again to prevent leaking crypto 
> > > > keys. E.g when you suspend laptop and put it off (somebody can 
> > > > remove RAMs and do some cold boot attack).
> > > > 
> > > > The most common situation is:
> > > > You have mounted partition from dm-crypt device (e.g. /home/), 
> > > > some userspace processes access it (e.g opened firefox which 
> > > > still reads/writes to cache ~/.firefox/) and you want to drop 
> > > > crypto keys from kernel for some time.
> > > > 
> > > > For that operation there is command cryptsetup luksSuspend, which 
> > > > suspend dm device and then tell kernel to wipe crypto keys. All 
> > > > I/O operations are then stopped and userspace processes which 
> > > > want to do some those I/O operations are stopped too (until you 
> > > > call cryptsetup luksResume and enter correct key).
> > > > 
> > > > Now if you want to suspend/hiberate your machine (when some of dm 
> > > > devices are suspeneded and some processes are stopped due to 
> > > > pending I/O) it is not possible. Kernel freeze_processes function 
> > > > will fail because userspace processes are still stopped inside 
> > > > some I/O syscall (read/write, etc,...).
> > > > 
> > > > My patches fixes this problem and do those operations (suspend dm 
> > > > device, wipe crypto keys, enter suspend/hiberate) in correct 
> > > > order and without race condition.
> > > > 
> > > > dm device is suspended *after* userspace processes are freezed 
> > > > and after that are crypto keys wiped. And then computer/laptop 
> > > > enters into suspend/hibernate state.
> > > 
> > > Wouldn't it be better to fix freeze_processes() to be tolerant of
> > > processes that are hung as a side-effect of their backing storage being
> > > suspended?  A hibernate shouldn't fail simply because a user chose to
> > > suspend a DM device.
> > > 
> > > Then this entire problem goes away and the key can be wiped from
> > > userspace (like you said above).
> > 
> > Still there will be race condition. Before hibernation (and device
> > poweroff) we should have synced disks and filesystems to prevent data
> > lose (or other damage) as more as we can. And if there will be some
> > application which using lot of I/O (e.g normal firefox) then there
> > always will be race condtion.
> 
> The DM suspend will take care of flushing any pending I/O.  So I don't
> see where the supposed race is...
> 

Any I/O operation after DM suspend is race condition and could cause
data lost.

> Anything else that is trapped in userspace memory will be there when the
> machine resumes.
> 

You are expecting that machine resumes always at 100% and correctly. But
this is not truth in real world. There are planty of users who reported
lot of random problems with suspend or hibernate...

> > So proper way is to wipe luks crypto keys *after* userspace processes
> > are freezed.
> 
> I know you believe that I'm just not accepting that at face value.

If disks are synced before any DM suspend operation then we have higher
chance of preventing data corruption.

I still think that correct order is only:

* freeze processes (which doing continous I/O)
* fs & disk sync
* DM suspend
* wipe crypto keys
* enter hibernate

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Mike Snitzer]

On Thu, Apr 09 2015 at 10:16am -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:

> On Thursday 09 April 2015 10:08:43 Mike Snitzer wrote:
> > On Thu, Apr 09 2015 at  9:28am -0400,
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > 
> > > On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> > > > On Mon, Apr 06 2015 at  9:29am -0400,
> > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > 
> > > > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > > > > On Sun, Apr 05 2015 at  1:20pm -0400,
> > > > > > 
> > > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > > This patch series increase security of suspend and hibernate
> > > > > > > actions. It allows user to safely wipe crypto keys before
> > > > > > > suspend and hibernate actions starts without race
> > > > > > > conditions on userspace process with heavy I/O.
> > > > > > > 
> > > > > > > To automatically wipe cryto key for <device> before
> > > > > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > > > > wipe_on_hibernation 1
> > > > > > > 
> > > > > > > To automatically wipe cryto key for <device> before suspend
> > > > > > > action call: $ dmsetup message <device> 0 key
> > > > > > > wipe_on_suspend 1
> > > > > > > 
> > > > > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > > > > not wipe key)
> > > > > > 
> > > > > > Can you elaborate on the attack vector your changes are meant
> > > > > > to protect against?  The user already authorized access, why
> > > > > > is it inherently dangerous to _not_ wipe the associated key
> > > > > > across these events?
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > yes, I will try to explain current problems with cryptsetup 
> > > > > luksSuspend command and hibernation.
> > > > > 
> > > > > First, sometimes it is needed to put machine into other hands. 
> > > > > You can still watch other person what is doing with machine, but 
> > > > > once if you let machine unlocked (e.g opened luks disk), she/he 
> > > > > can access encrypted data.
> > > > > 
> > > > > If you turn off machine, it could be safe, because luks disk 
> > > > > devices are locked. But if you enter machine into suspend or 
> > > > > hibernate state luks devices are still open. And my patches try 
> > > > > to achieve similar security as when machine is off (= no crypto 
> > > > > keys in RAM or on swap).
> > > > > 
> > > > > When doing hibernate on unencrypted swap it is to prevent leaking 
> > > > > crypto keys to hibernate image (which is stored in swap).
> > > > > 
> > > > > When doing suspend action it is again to prevent leaking crypto 
> > > > > keys. E.g when you suspend laptop and put it off (somebody can 
> > > > > remove RAMs and do some cold boot attack).
> > > > > 
> > > > > The most common situation is:
> > > > > You have mounted partition from dm-crypt device (e.g. /home/), 
> > > > > some userspace processes access it (e.g opened firefox which 
> > > > > still reads/writes to cache ~/.firefox/) and you want to drop 
> > > > > crypto keys from kernel for some time.
> > > > > 
> > > > > For that operation there is command cryptsetup luksSuspend, which 
> > > > > suspend dm device and then tell kernel to wipe crypto keys. All 
> > > > > I/O operations are then stopped and userspace processes which 
> > > > > want to do some those I/O operations are stopped too (until you 
> > > > > call cryptsetup luksResume and enter correct key).
> > > > > 
> > > > > Now if you want to suspend/hiberate your machine (when some of dm 
> > > > > devices are suspeneded and some processes are stopped due to 
> > > > > pending I/O) it is not possible. Kernel freeze_processes function 
> > > > > will fail because userspace processes are still stopped inside 
> > > > > some I/O syscall (read/write, etc,...).
> > > > > 
> > > > > My patches fixes this problem and do those operations (suspend dm 
> > > > > device, wipe crypto keys, enter suspend/hiberate) in correct 
> > > > > order and without race condition.
> > > > > 
> > > > > dm device is suspended *after* userspace processes are freezed 
> > > > > and after that are crypto keys wiped. And then computer/laptop 
> > > > > enters into suspend/hibernate state.
> > > > 
> > > > Wouldn't it be better to fix freeze_processes() to be tolerant of
> > > > processes that are hung as a side-effect of their backing storage being
> > > > suspended?  A hibernate shouldn't fail simply because a user chose to
> > > > suspend a DM device.
> > > > 
> > > > Then this entire problem goes away and the key can be wiped from
> > > > userspace (like you said above).
> > > 
> > > Still there will be race condition. Before hibernation (and device
> > > poweroff) we should have synced disks and filesystems to prevent data
> > > lose (or other damage) as more as we can. And if there will be some
> > > application which using lot of I/O (e.g normal firefox) then there
> > > always will be race condtion.
> > 
> > The DM suspend will take care of flushing any pending I/O.  So I don't
> > see where the supposed race is...
> > 
> 
> Any I/O operation after DM suspend is race condition and could cause
> data lost.
> 
> > Anything else that is trapped in userspace memory will be there when the
> > machine resumes.
> > 
> 
> You are expecting that machine resumes always at 100% and correctly. But
> this is not truth in real world. There are planty of users who reported
> lot of random problems with suspend or hibernate...

But the system was left in a crash consistent state.  Properly written
apps will wait for I/O to ensure data loss (in the event of a failed
resume) isn't a problem.
 
> > > So proper way is to wipe luks crypto keys *after* userspace processes
> > > are freezed.
> > 
> > I know you believe that I'm just not accepting that at face value.
> 
> If disks are synced before any DM suspend operation then we have higher
> chance of preventing data corruption.

disks are already synced as part of the DM suspend operation!

But you're saying that all user processes are frozen (and associated
I/O flushed) before the DM suspend, that is different:

> I still think that correct order is only:
> 
> * freeze processes (which doing continous I/O)
> * fs & disk sync
> * DM suspend
> * wipe crypto keys
> * enter hibernate

I just don't think that extreme is _required_ to have a hibernate/resume
that incorporates dm-crypt key wiping.

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

On Thursday 09 April 2015 10:26:58 Mike Snitzer wrote:
> On Thu, Apr 09 2015 at 10:16am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
> 
> > On Thursday 09 April 2015 10:08:43 Mike Snitzer wrote:
> > > On Thu, Apr 09 2015 at  9:28am -0400,
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > 
> > > > On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> > > > > On Mon, Apr 06 2015 at  9:29am -0400,
> > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > 
> > > > > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > > > > > On Sun, Apr 05 2015 at  1:20pm -0400,
> > > > > > > 
> > > > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > > > This patch series increase security of suspend and hibernate
> > > > > > > > actions. It allows user to safely wipe crypto keys before
> > > > > > > > suspend and hibernate actions starts without race
> > > > > > > > conditions on userspace process with heavy I/O.
> > > > > > > > 
> > > > > > > > To automatically wipe cryto key for <device> before
> > > > > > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > > > > > wipe_on_hibernation 1
> > > > > > > > 
> > > > > > > > To automatically wipe cryto key for <device> before suspend
> > > > > > > > action call: $ dmsetup message <device> 0 key
> > > > > > > > wipe_on_suspend 1
> > > > > > > > 
> > > > > > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > > > > > not wipe key)
> > > > > > > 
> > > > > > > Can you elaborate on the attack vector your changes are meant
> > > > > > > to protect against?  The user already authorized access, why
> > > > > > > is it inherently dangerous to _not_ wipe the associated key
> > > > > > > across these events?
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > yes, I will try to explain current problems with cryptsetup 
> > > > > > luksSuspend command and hibernation.
> > > > > > 
> > > > > > First, sometimes it is needed to put machine into other hands. 
> > > > > > You can still watch other person what is doing with machine, but 
> > > > > > once if you let machine unlocked (e.g opened luks disk), she/he 
> > > > > > can access encrypted data.
> > > > > > 
> > > > > > If you turn off machine, it could be safe, because luks disk 
> > > > > > devices are locked. But if you enter machine into suspend or 
> > > > > > hibernate state luks devices are still open. And my patches try 
> > > > > > to achieve similar security as when machine is off (= no crypto 
> > > > > > keys in RAM or on swap).
> > > > > > 
> > > > > > When doing hibernate on unencrypted swap it is to prevent leaking 
> > > > > > crypto keys to hibernate image (which is stored in swap).
> > > > > > 
> > > > > > When doing suspend action it is again to prevent leaking crypto 
> > > > > > keys. E.g when you suspend laptop and put it off (somebody can 
> > > > > > remove RAMs and do some cold boot attack).
> > > > > > 
> > > > > > The most common situation is:
> > > > > > You have mounted partition from dm-crypt device (e.g. /home/), 
> > > > > > some userspace processes access it (e.g opened firefox which 
> > > > > > still reads/writes to cache ~/.firefox/) and you want to drop 
> > > > > > crypto keys from kernel for some time.
> > > > > > 
> > > > > > For that operation there is command cryptsetup luksSuspend, which 
> > > > > > suspend dm device and then tell kernel to wipe crypto keys. All 
> > > > > > I/O operations are then stopped and userspace processes which 
> > > > > > want to do some those I/O operations are stopped too (until you 
> > > > > > call cryptsetup luksResume and enter correct key).
> > > > > > 
> > > > > > Now if you want to suspend/hiberate your machine (when some of dm 
> > > > > > devices are suspeneded and some processes are stopped due to 
> > > > > > pending I/O) it is not possible. Kernel freeze_processes function 
> > > > > > will fail because userspace processes are still stopped inside 
> > > > > > some I/O syscall (read/write, etc,...).
> > > > > > 
> > > > > > My patches fixes this problem and do those operations (suspend dm 
> > > > > > device, wipe crypto keys, enter suspend/hiberate) in correct 
> > > > > > order and without race condition.
> > > > > > 
> > > > > > dm device is suspended *after* userspace processes are freezed 
> > > > > > and after that are crypto keys wiped. And then computer/laptop 
> > > > > > enters into suspend/hibernate state.
> > > > > 
> > > > > Wouldn't it be better to fix freeze_processes() to be tolerant of
> > > > > processes that are hung as a side-effect of their backing storage being
> > > > > suspended?  A hibernate shouldn't fail simply because a user chose to
> > > > > suspend a DM device.
> > > > > 
> > > > > Then this entire problem goes away and the key can be wiped from
> > > > > userspace (like you said above).
> > > > 
> > > > Still there will be race condition. Before hibernation (and device
> > > > poweroff) we should have synced disks and filesystems to prevent data
> > > > lose (or other damage) as more as we can. And if there will be some
> > > > application which using lot of I/O (e.g normal firefox) then there
> > > > always will be race condtion.
> > > 
> > > The DM suspend will take care of flushing any pending I/O.  So I don't
> > > see where the supposed race is...
> > > 
> > 
> > Any I/O operation after DM suspend is race condition and could cause
> > data lost.
> > 
> > > Anything else that is trapped in userspace memory will be there when the
> > > machine resumes.
> > > 
> > 
> > You are expecting that machine resumes always at 100% and correctly. But
> > this is not truth in real world. There are planty of users who reported
> > lot of random problems with suspend or hibernate...
> 
> But the system was left in a crash consistent state.  Properly written
> apps will wait for I/O to ensure data loss (in the event of a failed
> resume) isn't a problem.
>  

I think you are too optimistic about ideal world...
"Properly written apps" "ensure data loss"

> > > > So proper way is to wipe luks crypto keys *after* userspace processes
> > > > are freezed.
> > > 
> > > I know you believe that I'm just not accepting that at face value.
> > 
> > If disks are synced before any DM suspend operation then we have higher
> > chance of preventing data corruption.
> 
> disks are already synced as part of the DM suspend operation!
> 

Yes, but part of hibernate operation is also sync call.

> But you're saying that all user processes are frozen (and associated
> I/O flushed) before the DM suspend, that is different:
> 

Yes, I want to ensure that. So processes wont be able to do any other
I/O.

> > I still think that correct order is only:
> > 
> > * freeze processes (which doing continous I/O)
> > * fs & disk sync
> > * DM suspend
> > * wipe crypto keys
> > * enter hibernate
> 
> I just don't think that extreme is _required_ to have a hibernate/resume
> that incorporates dm-crypt key wiping.

Ok, and what other developers think?

I'm saying that if I want to wipe luks keys before suspend/hibernate and
have system in consistant state as much as possible, keys must be wiped
*after* userspace processes are freezed. Or do you have relevant or
functional argument why not? Or is there any problem in my thinking?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pavel Machek]

Hi!

> > > > > So proper way is to wipe luks crypto keys *after* userspace processes
> > > > > are freezed.
> > > > 
> > > > I know you believe that I'm just not accepting that at face value.
> > > 
> > > If disks are synced before any DM suspend operation then we have higher
> > > chance of preventing data corruption.
> > 
> > disks are already synced as part of the DM suspend operation!
> > 
> 
> Yes, but part of hibernate operation is also sync call.

Yes. Maybe that was a mistake.

> > > I still think that correct order is only:
> > > 
> > > * freeze processes (which doing continous I/O)
> > > * fs & disk sync
> > > * DM suspend
> > > * wipe crypto keys
> > > * enter hibernate
> > 
> > I just don't think that extreme is _required_ to have a hibernate/resume
> > that incorporates dm-crypt key wiping.
> 
> Ok, and what other developers think?

If someone can fix freezer to work with LUKS stopped, that would be a 
good thing. Can you do it, Mike? Then we can look if it works well
enough for Pali.

But that might be too hard / impossible. And at that point, I think
Pali's patch is right thing to do.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

On Tuesday 14 April 2015 08:50:16 Pavel Machek wrote:
> Hi!
> 
> > > > > > So proper way is to wipe luks crypto keys *after* userspace processes
> > > > > > are freezed.
> > > > > 
> > > > > I know you believe that I'm just not accepting that at face value.
> > > > 
> > > > If disks are synced before any DM suspend operation then we have higher
> > > > chance of preventing data corruption.
> > > 
> > > disks are already synced as part of the DM suspend operation!
> > > 
> > 
> > Yes, but part of hibernate operation is also sync call.
> 
> Yes. Maybe that was a mistake.
> 

I think not. I do not see any reason why system should not sync disks...

> > > > I still think that correct order is only:
> > > > 
> > > > * freeze processes (which doing continous I/O)
> > > > * fs & disk sync
> > > > * DM suspend
> > > > * wipe crypto keys
> > > > * enter hibernate
> > > 
> > > I just don't think that extreme is _required_ to have a hibernate/resume
> > > that incorporates dm-crypt key wiping.
> > 
> > Ok, and what other developers think?
> 
> If someone can fix freezer to work with LUKS stopped, that would be a 
> good thing. Can you do it, Mike? Then we can look if it works well
> enough for Pali.
> 

Its not only LUKS devices. Its for all dm devices when are suspended.

> But that might be too hard / impossible. And at that point, I think
> Pali's patch is right thing to do.
> 									Pavel

Yes I think it is hard too...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Mike Snitzer]

On Thu, Apr 16 2015 at  5:23am -0400,
Alex Elsayed <eternaleye@gmail.com> wrote:

> Mike Snitzer wrote:
> 
> > On Thu, Apr 09 2015 at  9:28am -0400,
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > 
> >> On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> >> > On Mon, Apr 06 2015 at  9:29am -0400,
> >> > Pali Rohár <pali.rohar@gmail.com> wrote:
> >> > 
> >> > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> >> > > > On Sun, Apr 05 2015 at  1:20pm -0400,
> >> > > > 
> >> > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> >> > > > > This patch series increase security of suspend and hibernate
> >> > > > > actions. It allows user to safely wipe crypto keys before
> >> > > > > suspend and hibernate actions starts without race
> >> > > > > conditions on userspace process with heavy I/O.
> >> > > > > 
> >> > > > > To automatically wipe cryto key for <device> before
> >> > > > > hibernate action call: $ dmsetup message <device> 0 key
> >> > > > > wipe_on_hibernation 1
> >> > > > > 
> >> > > > > To automatically wipe cryto key for <device> before suspend
> >> > > > > action call: $ dmsetup message <device> 0 key
> >> > > > > wipe_on_suspend 1
> >> > > > > 
> >> > > > > (Value 0 after wipe_* string reverts original behaviour - to
> >> > > > > not wipe key)
> >> > > > 
> >> > > > Can you elaborate on the attack vector your changes are meant
> >> > > > to protect against?  The user already authorized access, why
> >> > > > is it inherently dangerous to _not_ wipe the associated key
> >> > > > across these events?
> >> > > 
> >> > > Hi,
> >> > > 
> >> > > yes, I will try to explain current problems with cryptsetup
> >> > > luksSuspend command and hibernation.
> >> > > 
> >> > > First, sometimes it is needed to put machine into other hands.
> >> > > You can still watch other person what is doing with machine, but
> >> > > once if you let machine unlocked (e.g opened luks disk), she/he
> >> > > can access encrypted data.
> >> > > 
> >> > > If you turn off machine, it could be safe, because luks disk
> >> > > devices are locked. But if you enter machine into suspend or
> >> > > hibernate state luks devices are still open. And my patches try
> >> > > to achieve similar security as when machine is off (= no crypto
> >> > > keys in RAM or on swap).
> >> > > 
> >> > > When doing hibernate on unencrypted swap it is to prevent leaking
> >> > > crypto keys to hibernate image (which is stored in swap).
> >> > > 
> >> > > When doing suspend action it is again to prevent leaking crypto
> >> > > keys. E.g when you suspend laptop and put it off (somebody can
> >> > > remove RAMs and do some cold boot attack).
> >> > > 
> >> > > The most common situation is:
> >> > > You have mounted partition from dm-crypt device (e.g. /home/),
> >> > > some userspace processes access it (e.g opened firefox which
> >> > > still reads/writes to cache ~/.firefox/) and you want to drop
> >> > > crypto keys from kernel for some time.
> >> > > 
> >> > > For that operation there is command cryptsetup luksSuspend, which
> >> > > suspend dm device and then tell kernel to wipe crypto keys. All
> >> > > I/O operations are then stopped and userspace processes which
> >> > > want to do some those I/O operations are stopped too (until you
> >> > > call cryptsetup luksResume and enter correct key).
> >> > > 
> >> > > Now if you want to suspend/hiberate your machine (when some of dm
> >> > > devices are suspeneded and some processes are stopped due to
> >> > > pending I/O) it is not possible. Kernel freeze_processes function
> >> > > will fail because userspace processes are still stopped inside
> >> > > some I/O syscall (read/write, etc,...).
> >> > > 
> >> > > My patches fixes this problem and do those operations (suspend dm
> >> > > device, wipe crypto keys, enter suspend/hiberate) in correct
> >> > > order and without race condition.
> >> > > 
> >> > > dm device is suspended *after* userspace processes are freezed
> >> > > and after that are crypto keys wiped. And then computer/laptop
> >> > > enters into suspend/hibernate state.
> >> > 
> >> > Wouldn't it be better to fix freeze_processes() to be tolerant of
> >> > processes that are hung as a side-effect of their backing storage being
> >> > suspended?  A hibernate shouldn't fail simply because a user chose to
> >> > suspend a DM device.
> >> > 
> >> > Then this entire problem goes away and the key can be wiped from
> >> > userspace (like you said above).
> >> 
> >> Still there will be race condition. Before hibernation (and device
> >> poweroff) we should have synced disks and filesystems to prevent data
> >> lose (or other damage) as more as we can. And if there will be some
> >> application which using lot of I/O (e.g normal firefox) then there
> >> always will be race condtion.
> > 
> > The DM suspend will take care of flushing any pending I/O.  So I don't
> > see where the supposed race is...
> > 
> > Anything else that is trapped in userspace memory will be there when the
> > machine resumes.
> > 
> >> So proper way is to wipe luks crypto keys *after* userspace processes
> >> are freezed.
> > 
> > I know you believe that I'm just not accepting that at face value.
> 
> Um, pardon me if I'm being naive, but what about the case of hibernation 
> where the swapdev and the root device are both LVs on the same dm_crypt 
> device?
> 
> The kernel is writing to swap _after_ userspace processes are all frozen; 
> that seems to me like an ordering dependency entirely incompatible with 
> userspace dropping the key...

Good point, definitely not compatible with the Pali's approach.

(but is swap really configured ontop of the same dm-crypt device like
this in practice?  I've not heard of that being a common pattern but I
could just be sheltered)

Re: [dm-devel] [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Ondrej Kozina]

On 04/17/2015 09:52 AM, Mike Snitzer wrote:
> On Thu, Apr 16 2015 at  5:23am -0400,
> Alex Elsayed <eternaleye@gmail.com> wrote:
>
>> Mike Snitzer wrote:
>>
>>> On Thu, Apr 09 2015 at  9:28am -0400,
>>> Pali Rohár <pali.rohar@gmail.com> wrote:
>>>
>>>> On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
>>>>> On Mon, Apr 06 2015 at  9:29am -0400,
>>>>> Pali Rohár <pali.rohar@gmail.com> wrote:
>>>>>
>>>>>> On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
>>>>>>> On Sun, Apr 05 2015 at  1:20pm -0400,
>>>>>>>
>>>>>>> Pali Rohár <pali.rohar@gmail.com> wrote:
>>>>>>>> This patch series increase security of suspend and hibernate
>>>>>>>> actions. It allows user to safely wipe crypto keys before
>>>>>>>> suspend and hibernate actions starts without race
>>>>>>>> conditions on userspace process with heavy I/O.
>>>>>>>>
>>>>>>>> To automatically wipe cryto key for <device> before
>>>>>>>> hibernate action call: $ dmsetup message <device> 0 key
>>>>>>>> wipe_on_hibernation 1
>>>>>>>>
>>>>>>>> To automatically wipe cryto key for <device> before suspend
>>>>>>>> action call: $ dmsetup message <device> 0 key
>>>>>>>> wipe_on_suspend 1
>>>>>>>>
>>>>>>>> (Value 0 after wipe_* string reverts original behaviour - to
>>>>>>>> not wipe key)
>>>>>>>
>>>>>>> Can you elaborate on the attack vector your changes are meant
>>>>>>> to protect against?  The user already authorized access, why
>>>>>>> is it inherently dangerous to _not_ wipe the associated key
>>>>>>> across these events?
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> yes, I will try to explain current problems with cryptsetup
>>>>>> luksSuspend command and hibernation.
>>>>>>
>>>>>> First, sometimes it is needed to put machine into other hands.
>>>>>> You can still watch other person what is doing with machine, but
>>>>>> once if you let machine unlocked (e.g opened luks disk), she/he
>>>>>> can access encrypted data.
>>>>>>
>>>>>> If you turn off machine, it could be safe, because luks disk
>>>>>> devices are locked. But if you enter machine into suspend or
>>>>>> hibernate state luks devices are still open. And my patches try
>>>>>> to achieve similar security as when machine is off (= no crypto
>>>>>> keys in RAM or on swap).
>>>>>>
>>>>>> When doing hibernate on unencrypted swap it is to prevent leaking
>>>>>> crypto keys to hibernate image (which is stored in swap).
>>>>>>
>>>>>> When doing suspend action it is again to prevent leaking crypto
>>>>>> keys. E.g when you suspend laptop and put it off (somebody can
>>>>>> remove RAMs and do some cold boot attack).
>>>>>>
>>>>>> The most common situation is:
>>>>>> You have mounted partition from dm-crypt device (e.g. /home/),
>>>>>> some userspace processes access it (e.g opened firefox which
>>>>>> still reads/writes to cache ~/.firefox/) and you want to drop
>>>>>> crypto keys from kernel for some time.
>>>>>>
>>>>>> For that operation there is command cryptsetup luksSuspend, which
>>>>>> suspend dm device and then tell kernel to wipe crypto keys. All
>>>>>> I/O operations are then stopped and userspace processes which
>>>>>> want to do some those I/O operations are stopped too (until you
>>>>>> call cryptsetup luksResume and enter correct key).
>>>>>>
>>>>>> Now if you want to suspend/hiberate your machine (when some of dm
>>>>>> devices are suspeneded and some processes are stopped due to
>>>>>> pending I/O) it is not possible. Kernel freeze_processes function
>>>>>> will fail because userspace processes are still stopped inside
>>>>>> some I/O syscall (read/write, etc,...).
>>>>>>
>>>>>> My patches fixes this problem and do those operations (suspend dm
>>>>>> device, wipe crypto keys, enter suspend/hiberate) in correct
>>>>>> order and without race condition.
>>>>>>
>>>>>> dm device is suspended *after* userspace processes are freezed
>>>>>> and after that are crypto keys wiped. And then computer/laptop
>>>>>> enters into suspend/hibernate state.
>>>>>
>>>>> Wouldn't it be better to fix freeze_processes() to be tolerant of
>>>>> processes that are hung as a side-effect of their backing storage being
>>>>> suspended?  A hibernate shouldn't fail simply because a user chose to
>>>>> suspend a DM device.
>>>>>
>>>>> Then this entire problem goes away and the key can be wiped from
>>>>> userspace (like you said above).
>>>>
>>>> Still there will be race condition. Before hibernation (and device
>>>> poweroff) we should have synced disks and filesystems to prevent data
>>>> lose (or other damage) as more as we can. And if there will be some
>>>> application which using lot of I/O (e.g normal firefox) then there
>>>> always will be race condtion.
>>>
>>> The DM suspend will take care of flushing any pending I/O.  So I don't
>>> see where the supposed race is...
>>>
>>> Anything else that is trapped in userspace memory will be there when the
>>> machine resumes.
>>>
>>>> So proper way is to wipe luks crypto keys *after* userspace processes
>>>> are freezed.
>>>
>>> I know you believe that I'm just not accepting that at face value.
>>
>> Um, pardon me if I'm being naive, but what about the case of hibernation
>> where the swapdev and the root device are both LVs on the same dm_crypt
>> device?
>>
>> The kernel is writing to swap _after_ userspace processes are all frozen;
>> that seems to me like an ordering dependency entirely incompatible with
>> userspace dropping the key...
>
> Good point, definitely not compatible with the Pali's approach.

Ouch! I'm afraid this effectively killed one of my experiments with 
dm-crypt suspend. Good to get reminded sooner than later!

> (but is swap really configured ontop of the same dm-crypt device like
> this in practice?  I've not heard of that being a common pattern but I
> could just be sheltered)

yes. It's one among many perfectly valid setups.

(For some the goal here would be to have whole disk encrypted including 
boot partition unlocked during boot)

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Alex Elsayed]

Mike Snitzer wrote:

> On Thu, Apr 16 2015 at  5:23am -0400,
> Alex Elsayed <eternaleye@gmail.com> wrote:
> 
>> Mike Snitzer wrote:
>> 
>> > On Thu, Apr 09 2015 at  9:28am -0400,
>> > Pali Rohár <pali.rohar@gmail.com> wrote:
>> > 
>> >> On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
>> >> > On Mon, Apr 06 2015 at  9:29am -0400,
>> >> > Pali Rohár <pali.rohar@gmail.com> wrote:
>> >> > 
>> >> > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
>> >> > > > On Sun, Apr 05 2015 at  1:20pm -0400,
>> >> > > > 
>> >> > > > Pali Rohár <pali.rohar@gmail.com> wrote:
>> >> > > > > This patch series increase security of suspend and hibernate
>> >> > > > > actions. It allows user to safely wipe crypto keys before
>> >> > > > > suspend and hibernate actions starts without race
>> >> > > > > conditions on userspace process with heavy I/O.
>> >> > > > > 
>> >> > > > > To automatically wipe cryto key for <device> before
>> >> > > > > hibernate action call: $ dmsetup message <device> 0 key
>> >> > > > > wipe_on_hibernation 1
>> >> > > > > 
>> >> > > > > To automatically wipe cryto key for <device> before suspend
>> >> > > > > action call: $ dmsetup message <device> 0 key
>> >> > > > > wipe_on_suspend 1
>> >> > > > > 
>> >> > > > > (Value 0 after wipe_* string reverts original behaviour - to
>> >> > > > > not wipe key)
>> >> > > > 
>> >> > > > Can you elaborate on the attack vector your changes are meant
>> >> > > > to protect against?  The user already authorized access, why
>> >> > > > is it inherently dangerous to _not_ wipe the associated key
>> >> > > > across these events?
>> >> > > 
>> >> > > Hi,
>> >> > > 
>> >> > > yes, I will try to explain current problems with cryptsetup
>> >> > > luksSuspend command and hibernation.
>> >> > > 
>> >> > > First, sometimes it is needed to put machine into other hands.
>> >> > > You can still watch other person what is doing with machine, but
>> >> > > once if you let machine unlocked (e.g opened luks disk), she/he
>> >> > > can access encrypted data.
>> >> > > 
>> >> > > If you turn off machine, it could be safe, because luks disk
>> >> > > devices are locked. But if you enter machine into suspend or
>> >> > > hibernate state luks devices are still open. And my patches try
>> >> > > to achieve similar security as when machine is off (= no crypto
>> >> > > keys in RAM or on swap).
>> >> > > 
>> >> > > When doing hibernate on unencrypted swap it is to prevent leaking
>> >> > > crypto keys to hibernate image (which is stored in swap).
>> >> > > 
>> >> > > When doing suspend action it is again to prevent leaking crypto
>> >> > > keys. E.g when you suspend laptop and put it off (somebody can
>> >> > > remove RAMs and do some cold boot attack).
>> >> > > 
>> >> > > The most common situation is:
>> >> > > You have mounted partition from dm-crypt device (e.g. /home/),
>> >> > > some userspace processes access it (e.g opened firefox which
>> >> > > still reads/writes to cache ~/.firefox/) and you want to drop
>> >> > > crypto keys from kernel for some time.
>> >> > > 
>> >> > > For that operation there is command cryptsetup luksSuspend, which
>> >> > > suspend dm device and then tell kernel to wipe crypto keys. All
>> >> > > I/O operations are then stopped and userspace processes which
>> >> > > want to do some those I/O operations are stopped too (until you
>> >> > > call cryptsetup luksResume and enter correct key).
>> >> > > 
>> >> > > Now if you want to suspend/hiberate your machine (when some of dm
>> >> > > devices are suspeneded and some processes are stopped due to
>> >> > > pending I/O) it is not possible. Kernel freeze_processes function
>> >> > > will fail because userspace processes are still stopped inside
>> >> > > some I/O syscall (read/write, etc,...).
>> >> > > 
>> >> > > My patches fixes this problem and do those operations (suspend dm
>> >> > > device, wipe crypto keys, enter suspend/hiberate) in correct
>> >> > > order and without race condition.
>> >> > > 
>> >> > > dm device is suspended *after* userspace processes are freezed
>> >> > > and after that are crypto keys wiped. And then computer/laptop
>> >> > > enters into suspend/hibernate state.
>> >> > 
>> >> > Wouldn't it be better to fix freeze_processes() to be tolerant of
>> >> > processes that are hung as a side-effect of their backing storage
>> >> > being
>> >> > suspended?  A hibernate shouldn't fail simply because a user chose
>> >> > to suspend a DM device.
>> >> > 
>> >> > Then this entire problem goes away and the key can be wiped from
>> >> > userspace (like you said above).
>> >> 
>> >> Still there will be race condition. Before hibernation (and device
>> >> poweroff) we should have synced disks and filesystems to prevent data
>> >> lose (or other damage) as more as we can. And if there will be some
>> >> application which using lot of I/O (e.g normal firefox) then there
>> >> always will be race condtion.
>> > 
>> > The DM suspend will take care of flushing any pending I/O.  So I don't
>> > see where the supposed race is...
>> > 
>> > Anything else that is trapped in userspace memory will be there when
>> > the machine resumes.
>> > 
>> >> So proper way is to wipe luks crypto keys *after* userspace processes
>> >> are freezed.
>> > 
>> > I know you believe that I'm just not accepting that at face value.
>> 
>> Um, pardon me if I'm being naive, but what about the case of hibernation
>> where the swapdev and the root device are both LVs on the same dm_crypt
>> device?
>> 
>> The kernel is writing to swap _after_ userspace processes are all frozen;
>> that seems to me like an ordering dependency entirely incompatible with
>> userspace dropping the key...
> 
> Good point, definitely not compatible with the Pali's approach.
> 
> (but is swap really configured ontop of the same dm-crypt device like
> this in practice?  I've not heard of that being a common pattern but I
> could just be sheltered)

Every laptop I've owned in the past five years has been set up as follows:

- GPT partition table (mostly for the redundant table at the end in case of 
fuckups)
- 1GB ESP as /boot (first with grub2, then gummiboot) - It's there _anyway_
- 32MB BIOS Boot Partition (for a traditional BIOS bootloader, so I can pop
  the drive in a non-efi machine if the laptop dies - this has happened)
- The rest of the drive is a single dm-crypt volume, with LVM on top. What
  goes on top of LVM has varied, but these days it's just swap and btrfs.

The main reason for this is that I find dealing with crypttab / multiple
LUKS devices on boot (or resume from hibernate) to be an incredible hassle;
and it's vastly simpler to  just have a single dm-crypt device and let
Dracut unlock it from a single boot prompt.

I haven't set up custom-key secure boot yet, so the evil maid attack is
still on the table, but I do this more out of "Eh, why not" (and originally, 
"I should at least know _how_ to set it up") than actually having stuff I 
need the security for anyway.

Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pavel Machek]

On Thu 2015-04-09 09:12:08, Mike Snitzer wrote:
> On Mon, Apr 06 2015 at  9:29am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
> 
> > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > On Sun, Apr 05 2015 at  1:20pm -0400,
> > > 
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > This patch series increase security of suspend and hibernate
> > > > actions. It allows user to safely wipe crypto keys before
> > > > suspend and hibernate actions starts without race
> > > > conditions on userspace process with heavy I/O.
> > > > 
> > > > To automatically wipe cryto key for <device> before
> > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > wipe_on_hibernation 1
> > > > 
> > > > To automatically wipe cryto key for <device> before suspend
> > > > action call: $ dmsetup message <device> 0 key
> > > > wipe_on_suspend 1
> > > > 
> > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > not wipe key)
> > > 
> > > Can you elaborate on the attack vector your changes are meant
> > > to protect against?  The user already authorized access, why
> > > is it inherently dangerous to _not_ wipe the associated key
> > > across these events?
> > 
> > Hi,
> > 
> > yes, I will try to explain current problems with cryptsetup 
> > luksSuspend command and hibernation.
> > 
> > First, sometimes it is needed to put machine into other hands. 
> > You can still watch other person what is doing with machine, but 
> > once if you let machine unlocked (e.g opened luks disk), she/he 
> > can access encrypted data.
> > 
> > If you turn off machine, it could be safe, because luks disk 
> > devices are locked. But if you enter machine into suspend or 
> > hibernate state luks devices are still open. And my patches try 
> > to achieve similar security as when machine is off (= no crypto 
> > keys in RAM or on swap).
> > 
> > When doing hibernate on unencrypted swap it is to prevent leaking 
> > crypto keys to hibernate image (which is stored in swap).
> > 
> > When doing suspend action it is again to prevent leaking crypto 
> > keys. E.g when you suspend laptop and put it off (somebody can 
> > remove RAMs and do some cold boot attack).
> > 
> > The most common situation is:
> > You have mounted partition from dm-crypt device (e.g. /home/), 
> > some userspace processes access it (e.g opened firefox which 
> > still reads/writes to cache ~/.firefox/) and you want to drop 
> > crypto keys from kernel for some time.
> > 
> > For that operation there is command cryptsetup luksSuspend, which 
> > suspend dm device and then tell kernel to wipe crypto keys. All 
> > I/O operations are then stopped and userspace processes which 
> > want to do some those I/O operations are stopped too (until you 
> > call cryptsetup luksResume and enter correct key).
> > 
> > Now if you want to suspend/hiberate your machine (when some of dm 
> > devices are suspeneded and some processes are stopped due to 
> > pending I/O) it is not possible. Kernel freeze_processes function 
> > will fail because userspace processes are still stopped inside 
> > some I/O syscall (read/write, etc,...).
> > 
> > My patches fixes this problem and do those operations (suspend dm 
> > device, wipe crypto keys, enter suspend/hiberate) in correct 
> > order and without race condition.
> > 
> > dm device is suspended *after* userspace processes are freezed 
> > and after that are crypto keys wiped. And then computer/laptop 
> > enters into suspend/hibernate state.
> 
> Wouldn't it be better to fix freeze_processes() to be tolerant of
> processes that are hung as a side-effect of their backing storage being
> suspended?  A hibernate shouldn't fail simply because a user chose to
> suspend a DM device.

That would be nice, I agree. But that's non-trivial ammount of work
and might be (close to) impossible.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v2 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

This patch series increase security of suspend and hibernate actions. It allows
user to safely wipe crypto keys before suspend and hibernate actions starts
without race conditions on userspace process with heavy I/O.

To automatically wipe cryto key for <device> before hibernate action call:
$ dmsetup message <device> 0 key wipe_on_hibernation

To automatically wipe cryto key for <device> before suspend action call:
$ dmsetup message <device> 0 key wipe_on_suspend

To disable automatic wipe call retain_on_suspend/retain_on_hibernation.

Pali Rohár (3):
  PM suspend/hibernate: Call notifier after freezing processes
  dm: Export function dm_suspend_md()
  dm-crypt: Adds support for wiping key when doing suspend/hibernation

 drivers/md/dm-crypt.c    |  126 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/md/dm.c          |    6 +++
 drivers/md/dm.h          |    5 ++
 include/linux/suspend.h  |    2 +
 kernel/power/hibernate.c |    2 +
 kernel/power/suspend.c   |    4 +-
 6 files changed, 136 insertions(+), 9 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Pali Rohár]

To prevent race conditions on userspace processes with I/O some taks must be
called after processes are freezed. This patch adds new events which are
delivered by pm_notifier_call_chain() after freezing processes when doing
suspend or hibernate action.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 include/linux/suspend.h  |    2 ++
 kernel/power/hibernate.c |    2 ++
 kernel/power/suspend.c   |    4 +++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 5efe743..bc743c8 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; }
 #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
 #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
 #define PM_POST_RESTORE		0x0006 /* Restore failed */
+#define PM_HIBERNATION_AFTER_FREEZE	0x0007 /* After hibernation freeze */
+#define PM_SUSPEND_AFTER_FREEZE		0x0008 /* After suspend freeze */
 
 extern struct mutex pm_mutex;
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 2329daa..184f7ee 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -671,6 +671,8 @@ int hibernate(void)
 	if (error)
 		goto Exit;
 
+	pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);
+
 	lock_device_hotplug();
 	/* Allocate memory management structures */
 	error = create_basic_memory_bitmaps();
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 8d7a1ef..ba2a945 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -277,8 +277,10 @@ static int suspend_prepare(suspend_state_t state)
 	trace_suspend_resume(TPS("freeze_processes"), 0, true);
 	error = suspend_freeze_processes();
 	trace_suspend_resume(TPS("freeze_processes"), 0, false);
-	if (!error)
+	if (!error) {
+		pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
 		return 0;
+	}
 
 	suspend_stats.failed_freeze++;
 	dpm_save_failed_step(SUSPEND_FREEZE);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Rafael J. Wysocki]

On Sunday, June 21, 2015 01:20:32 PM Pali Rohár wrote:
> To prevent race conditions on userspace processes with I/O some taks must be
> called after processes are freezed. This patch adds new events which are
> delivered by pm_notifier_call_chain() after freezing processes when doing
> suspend or hibernate action.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  include/linux/suspend.h  |    2 ++
>  kernel/power/hibernate.c |    2 ++
>  kernel/power/suspend.c   |    4 +++-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 5efe743..bc743c8 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; }
>  #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
>  #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
>  #define PM_POST_RESTORE		0x0006 /* Restore failed */
> +#define PM_HIBERNATION_AFTER_FREEZE	0x0007 /* After hibernation freeze */
> +#define PM_SUSPEND_AFTER_FREEZE		0x0008 /* After suspend freeze */
>  
>  extern struct mutex pm_mutex;
>  
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 2329daa..184f7ee 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -671,6 +671,8 @@ int hibernate(void)
>  	if (error)
>  		goto Exit;
>  
> +	pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);

Don't we need to check errors from these?

Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too
for symmetry.

> +
>  	lock_device_hotplug();
>  	/* Allocate memory management structures */
>  	error = create_basic_memory_bitmaps();
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8d7a1ef..ba2a945 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -277,8 +277,10 @@ static int suspend_prepare(suspend_state_t state)
>  	trace_suspend_resume(TPS("freeze_processes"), 0, true);
>  	error = suspend_freeze_processes();
>  	trace_suspend_resume(TPS("freeze_processes"), 0, false);
> -	if (!error)
> +	if (!error) {
> +		pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
>  		return 0;
> +	}
>  
>  	suspend_stats.failed_freeze++;
>  	dpm_save_failed_step(SUSPEND_FREEZE);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Pali Rohár]

On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote:
> On Sunday, June 21, 2015 01:20:32 PM Pali Rohár wrote:
> > To prevent race conditions on userspace processes with I/O some taks must be
> > called after processes are freezed. This patch adds new events which are
> > delivered by pm_notifier_call_chain() after freezing processes when doing
> > suspend or hibernate action.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >  include/linux/suspend.h  |    2 ++
> >  kernel/power/hibernate.c |    2 ++
> >  kernel/power/suspend.c   |    4 +++-
> >  3 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 5efe743..bc743c8 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; }
> >  #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
> >  #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
> >  #define PM_POST_RESTORE		0x0006 /* Restore failed */
> > +#define PM_HIBERNATION_AFTER_FREEZE	0x0007 /* After hibernation freeze */
> > +#define PM_SUSPEND_AFTER_FREEZE		0x0008 /* After suspend freeze */
> >  
> >  extern struct mutex pm_mutex;
> >  
> > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > index 2329daa..184f7ee 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -671,6 +671,8 @@ int hibernate(void)
> >  	if (error)
> >  		goto Exit;
> >  
> > +	pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);
> 
> Don't we need to check errors from these?
> 

If yes, what to do in this case? Fail hibernation and goto Exit?

> Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too
> for symmetry.
> 

But there is no use case for BEFORE_THAW. At least it is not needed for now.

> > +
> >  	lock_device_hotplug();
> >  	/* Allocate memory management structures */
> >  	error = create_basic_memory_bitmaps();
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 8d7a1ef..ba2a945 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -277,8 +277,10 @@ static int suspend_prepare(suspend_state_t state)
> >  	trace_suspend_resume(TPS("freeze_processes"), 0, true);
> >  	error = suspend_freeze_processes();
> >  	trace_suspend_resume(TPS("freeze_processes"), 0, false);
> > -	if (!error)
> > +	if (!error) {
> > +		pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
> >  		return 0;
> > +	}
> >  
> >  	suspend_stats.failed_freeze++;
> >  	dpm_save_failed_step(SUSPEND_FREEZE);
> > 
> 

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Rafael J. Wysocki]

On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote:
> On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote:
> > On Sunday, June 21, 2015 01:20:32 PM Pali Rohár wrote:
> > > To prevent race conditions on userspace processes with I/O some taks must be
> > > called after processes are freezed. This patch adds new events which are
> > > delivered by pm_notifier_call_chain() after freezing processes when doing
> > > suspend or hibernate action.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > >  include/linux/suspend.h  |    2 ++
> > >  kernel/power/hibernate.c |    2 ++
> > >  kernel/power/suspend.c   |    4 +++-
> > >  3 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > index 5efe743..bc743c8 100644
> > > --- a/include/linux/suspend.h
> > > +++ b/include/linux/suspend.h
> > > @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; }
> > >  #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
> > >  #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
> > >  #define PM_POST_RESTORE		0x0006 /* Restore failed */
> > > +#define PM_HIBERNATION_AFTER_FREEZE	0x0007 /* After hibernation freeze */
> > > +#define PM_SUSPEND_AFTER_FREEZE		0x0008 /* After suspend freeze */
> > >  
> > >  extern struct mutex pm_mutex;
> > >  
> > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > index 2329daa..184f7ee 100644
> > > --- a/kernel/power/hibernate.c
> > > +++ b/kernel/power/hibernate.c
> > > @@ -671,6 +671,8 @@ int hibernate(void)
> > >  	if (error)
> > >  		goto Exit;
> > >  
> > > +	pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);
> > 
> > Don't we need to check errors from these?
> > 
> 
> If yes, what to do in this case? Fail hibernation and goto Exit?

Yes, fail the transition in progress.


> > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too
> > for symmetry.
> > 
> 
> But there is no use case for BEFORE_THAW. At least it is not needed for now.

For your use case, a single function pointer would be sufficient too.


> > > +
> > >  	lock_device_hotplug();
> > >  	/* Allocate memory management structures */
> > >  	error = create_basic_memory_bitmaps();
> > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > > index 8d7a1ef..ba2a945 100644
> > > --- a/kernel/power/suspend.c
> > > +++ b/kernel/power/suspend.c
> > > @@ -277,8 +277,10 @@ static int suspend_prepare(suspend_state_t state)
> > >  	trace_suspend_resume(TPS("freeze_processes"), 0, true);
> > >  	error = suspend_freeze_processes();
> > >  	trace_suspend_resume(TPS("freeze_processes"), 0, false);
> > > -	if (!error)
> > > +	if (!error) {
> > > +		pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
> > >  		return 0;
> > > +	}
> > >  
> > >  	suspend_stats.failed_freeze++;
> > >  	dpm_save_failed_step(SUSPEND_FREEZE);
> > > 
> > 
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Pali Rohár]

On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote:
> On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote:
> > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote:
> > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too
> > > for symmetry.
> > > 
> > 
> > But there is no use case for BEFORE_THAW. At least it is not needed for now.
> 
> For your use case, a single function pointer would be sufficient too.
> 

What do you mean by single function pointer? kernel/power is part of
kernel image and dm-crypt is external kernel module.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Rafael J. Wysocki]

On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote:
> On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote:
> > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote:
> > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote:
> > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too
> > > > for symmetry.
> > > > 
> > > 
> > > But there is no use case for BEFORE_THAW. At least it is not needed for now.
> > 
> > For your use case, a single function pointer would be sufficient too.
> > 
> 
> What do you mean by single function pointer? kernel/power is part of
> kernel image and dm-crypt is external kernel module.

Well, if there is a function pointer in the core suspend code initially set to
NULL and exported to modules such that the dm-crypt code can set it to
something else, that should be sufficient, shouldn't it?

So if you're adding new PM notifier events, that's already more than *you* need.

Anyway, I guess the "post freeze" new one should be enough for now, but please
change the name to POST_FREEZE.

Also I think we don't need separate "post freeze" events for suspend and
hibernation.

Thanks,
Rafael

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[NeilBrown]

On Mon, 20 Jul 2015 23:46:32 +0200 "Rafael J. Wysocki"
<rjw@rjwysocki.net> wrote:

> On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote:
> > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote:
> > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote:
> > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote:
> > > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too
> > > > > for symmetry.
> > > > > 
> > > > 
> > > > But there is no use case for BEFORE_THAW. At least it is not needed for now.
> > > 
> > > For your use case, a single function pointer would be sufficient too.
> > > 
> > 
> > What do you mean by single function pointer? kernel/power is part of
> > kernel image and dm-crypt is external kernel module.
> 
> Well, if there is a function pointer in the core suspend code initially set to
> NULL and exported to modules such that the dm-crypt code can set it to
> something else, that should be sufficient, shouldn't it?

As long as the dm-crypt module is never unloaded.  And as long as no
other module could very possible want functionality like this. Ever.

If a module wants to be notified - the providing a notifier chain
really seems like the right thing to do...

NeilBrown


> 
> So if you're adding new PM notifier events, that's already more than *you* need.
> 
> Anyway, I guess the "post freeze" new one should be enough for now, but please
> change the name to POST_FREEZE.
> 
> Also I think we don't need separate "post freeze" events for suspend and
> hibernation.
> 
> Thanks,
> Rafael

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Rafael J. Wysocki]

Hi Neil,

On Wed, Jul 22, 2015 at 12:08 AM, NeilBrown <neilb@suse.com> wrote:
> On Mon, 20 Jul 2015 23:46:32 +0200 "Rafael J. Wysocki"
> <rjw@rjwysocki.net> wrote:
>
>> On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote:
>> > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote:
>> > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote:
>> > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote:
>> > > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too
>> > > > > for symmetry.
>> > > > >
>> > > >
>> > > > But there is no use case for BEFORE_THAW. At least it is not needed for now.
>> > >
>> > > For your use case, a single function pointer would be sufficient too.
>> > >
>> >
>> > What do you mean by single function pointer? kernel/power is part of
>> > kernel image and dm-crypt is external kernel module.
>>
>> Well, if there is a function pointer in the core suspend code initially set to
>> NULL and exported to modules such that the dm-crypt code can set it to
>> something else, that should be sufficient, shouldn't it?
>
> As long as the dm-crypt module is never unloaded.

OK, there is a race related to that.

> And as long as no other module could very possible want functionality like this. Ever.

The point was that there were no other users currently, so dm-crypt is
going to be the only user for the time being.

> If a module wants to be notified - the providing a notifier chain
> really seems like the right thing to do...

Well, so please see my last response in this thread. :-)

Thanks,
Rafael

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Rafael J. Wysocki]

On Wed, Jul 22, 2015 at 1:00 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> Hi Neil,
>
> On Wed, Jul 22, 2015 at 12:08 AM, NeilBrown <neilb@suse.com> wrote:
>> On Mon, 20 Jul 2015 23:46:32 +0200 "Rafael J. Wysocki"
>> <rjw@rjwysocki.net> wrote:
>>
>>> On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote:
>>> > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote:
>>> > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote:
>>> > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote:
>>> > > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too
>>> > > > > for symmetry.
>>> > > > >
>>> > > >
>>> > > > But there is no use case for BEFORE_THAW. At least it is not needed for now.
>>> > >
>>> > > For your use case, a single function pointer would be sufficient too.
>>> > >
>>> >
>>> > What do you mean by single function pointer? kernel/power is part of
>>> > kernel image and dm-crypt is external kernel module.
>>>
>>> Well, if there is a function pointer in the core suspend code initially set to
>>> NULL and exported to modules such that the dm-crypt code can set it to
>>> something else, that should be sufficient, shouldn't it?
>>
>> As long as the dm-crypt module is never unloaded.
>
> OK, there is a race related to that.
>
>> And as long as no other module could very possible want functionality like this. Ever.
>
> The point was that there were no other users currently, so dm-crypt is
> going to be the only user for the time being.
>
>> If a module wants to be notified - the providing a notifier chain
>> really seems like the right thing to do...
>
> Well, so please see my last response in this thread. :-)

So it was below: "Anyway, I guess the "post freeze" new one should be
enough for now" which doesn't mean I'm really against the notifier.
Or at least it is not supposed to mean so.  If there's any confusion
related to that, I'm sorry.

Thanks,
Rafael

Re: [PATCH v2 1/3] PM suspend/hibernate: Call notifier after freezing processes

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2736 bytes --]

On Wednesday 22 July 2015 01:03:23 Rafael J. Wysocki wrote:
> On Wed, Jul 22, 2015 at 1:00 AM, Rafael J. Wysocki <rafael@kernel.org>
> wrote:
> > Hi Neil,
> > 
> > On Wed, Jul 22, 2015 at 12:08 AM, NeilBrown <neilb@suse.com> wrote:
> >> On Mon, 20 Jul 2015 23:46:32 +0200 "Rafael J. Wysocki"
> >> 
> >> <rjw@rjwysocki.net> wrote:
> >>> On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote:
> >>> > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote:
> >>> > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote:
> >>> > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote:
> >>> > > > > Also, if you're adding AFTER_FREEZE, it would be good to
> >>> > > > > add BEFORE_THAW too for symmetry.
> >>> > > > 
> >>> > > > But there is no use case for BEFORE_THAW. At least it is
> >>> > > > not needed for now.
> >>> > > 
> >>> > > For your use case, a single function pointer would be
> >>> > > sufficient too.
> >>> > 
> >>> > What do you mean by single function pointer? kernel/power is
> >>> > part of kernel image and dm-crypt is external kernel module.
> >>> 
> >>> Well, if there is a function pointer in the core suspend code
> >>> initially set to NULL and exported to modules such that the
> >>> dm-crypt code can set it to something else, that should be
> >>> sufficient, shouldn't it?
> >> 
> >> As long as the dm-crypt module is never unloaded.
> > 
> > OK, there is a race related to that.
> > 
> >> And as long as no other module could very possible want
> >> functionality like this. Ever.
> > 
> > The point was that there were no other users currently, so dm-crypt
> > is going to be the only user for the time being.
> > 
> >> If a module wants to be notified - the providing a notifier chain
> >> really seems like the right thing to do...
> > 
> > Well, so please see my last response in this thread. :-)
> 
> So it was below: "Anyway, I guess the "post freeze" new one should be
> enough for now" which doesn't mean I'm really against the notifier.
> Or at least it is not supposed to mean so.  If there's any confusion
> related to that, I'm sorry.
> 
> Thanks,
> Rafael

In that case we are not able to distinguish if computer is going to be 
hibernated or just suspended to RAM.

If we have both notifier (one for suspend and for hibernate) then 
different actions can be configured for suspend and hibernate.

And it makes sense to configure different behaviour for suspend and for 
hibernate. E.g. when you have encrypted partition where is stored 
hibernation image then you do not have to wipe keys before going to do 
hibernation. But for suspend to RAM you may want to wipe them.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2 2/3] dm: Export function dm_suspend_md()

[Pali Rohár]

This patch exports function dm_suspend_md() which suspend mapped device so other
kernel drivers can use it and could suspend mapped device when needed.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/md/dm.c |    6 ++++++
 drivers/md/dm.h |    5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 2caf492..03298ff 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -3343,6 +3343,12 @@ out:
 	return r;
 }
 
+int dm_suspend_md(struct mapped_device *md)
+{
+	return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
+}
+EXPORT_SYMBOL_GPL(dm_suspend_md);
+
 /*
  * Internal suspend/resume works like userspace-driven suspend. It waits
  * until all bios finish and prevents issuing new bios to the target drivers.
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 6123c2b..528e5e0 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -151,6 +151,11 @@ int dm_test_deferred_remove_flag(struct mapped_device *md);
 void dm_deferred_remove(void);
 
 /*
+ * Suspend mapped_device
+ */
+int dm_suspend_md(struct mapped_device *md);
+
+/*
  * The device-mapper can be driven through one of two interfaces;
  * ioctl or filesystem, depending which patch you have applied.
  */
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] dm: Export function dm_suspend_md()

[Mike Snitzer]

On Sun, Jun 21 2015 at  7:20am -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:

> This patch exports function dm_suspend_md() which suspend mapped device so other
> kernel drivers can use it and could suspend mapped device when needed.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/md/dm.c |    6 ++++++
>  drivers/md/dm.h |    5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 2caf492..03298ff 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -3343,6 +3343,12 @@ out:
>  	return r;
>  }
>  
> +int dm_suspend_md(struct mapped_device *md)
> +{
> +	return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> +}
> +EXPORT_SYMBOL_GPL(dm_suspend_md);
> +
>  /*
>   * Internal suspend/resume works like userspace-driven suspend. It waits
>   * until all bios finish and prevents issuing new bios to the target drivers.

To do this properly you should be introducing a variant of
dm_internal_suspend.  We currently have two variants:
dm_internal_suspend_fast
dm_internal_suspend_noflush

The reason to use a dm_internal_suspend variant is this suspend was
_not_ initiated by an upper level ioctl (from userspace).  It was
done internally from within the target.

You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning you're
interested in flushing all pending IO (in the FS layered on dm-crupt, if
one exists).

But see the comment in __dm_internal_suspend() about TASK_UNINTERRUPTIBLE.
If you're OK with the dm-crypt initiated suspend being
TASK_UNINTERRUPTIBLE then you could just introduce:

void dm_internal_suspend_uninterruptible_flush(struct mapped_device *md)
{
        mutex_lock(&md->suspend_lock);
        __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
        mutex_unlock(&md->suspend_lock);
}
EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush);

Otherwise, there is much more extensive DM core changes needed to
__dm_internal_suspend() and .presuspend to properly support
TASK_INTERRUPTIBLE.

Re: [PATCH v2 2/3] dm: Export function dm_suspend_md()

[Pali Rohár]

On Friday 17 July 2015 10:04:39 Mike Snitzer wrote:
> On Sun, Jun 21 2015 at  7:20am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
> 
> > This patch exports function dm_suspend_md() which suspend mapped device so other
> > kernel drivers can use it and could suspend mapped device when needed.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >  drivers/md/dm.c |    6 ++++++
> >  drivers/md/dm.h |    5 +++++
> >  2 files changed, 11 insertions(+)
> > 
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 2caf492..03298ff 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -3343,6 +3343,12 @@ out:
> >  	return r;
> >  }
> >  
> > +int dm_suspend_md(struct mapped_device *md)
> > +{
> > +	return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> > +}
> > +EXPORT_SYMBOL_GPL(dm_suspend_md);
> > +
> >  /*
> >   * Internal suspend/resume works like userspace-driven suspend. It waits
> >   * until all bios finish and prevents issuing new bios to the target drivers.
> 
> To do this properly you should be introducing a variant of
> dm_internal_suspend.  We currently have two variants:
> dm_internal_suspend_fast
> dm_internal_suspend_noflush
> 
> The reason to use a dm_internal_suspend variant is this suspend was
> _not_ initiated by an upper level ioctl (from userspace).  It was
> done internally from within the target.
> 
> You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning you're
> interested in flushing all pending IO (in the FS layered on dm-crupt, if
> one exists).
> 
> But see the comment in __dm_internal_suspend() about TASK_UNINTERRUPTIBLE.
> If you're OK with the dm-crypt initiated suspend being
> TASK_UNINTERRUPTIBLE then you could just introduce:
> 
> void dm_internal_suspend_uninterruptible_flush(struct mapped_device *md)
> {
>         mutex_lock(&md->suspend_lock);
>         __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
>         mutex_unlock(&md->suspend_lock);
> }
> EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush);
> 
> Otherwise, there is much more extensive DM core changes needed to
> __dm_internal_suspend() and .presuspend to properly support
> TASK_INTERRUPTIBLE.

Hi! I will look at dm_internal_suspend. Anyway use case for suspend is
from dm-crypt to do both operations: suspend + key wipe. It means that
without entering key again from userspace, resume is not possible. So my
question is: It is possible to do internal suspend and then using resume
from userspace via ioctl?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2 2/3] dm: Export function dm_suspend_md()

[Mike Snitzer]

On Fri, Jul 17 2015 at 10:22am -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:

> On Friday 17 July 2015 10:04:39 Mike Snitzer wrote:
> > On Sun, Jun 21 2015 at  7:20am -0400,
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > 
> > > This patch exports function dm_suspend_md() which suspend mapped device so other
> > > kernel drivers can use it and could suspend mapped device when needed.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > >  drivers/md/dm.c |    6 ++++++
> > >  drivers/md/dm.h |    5 +++++
> > >  2 files changed, 11 insertions(+)
> > > 
> > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > > index 2caf492..03298ff 100644
> > > --- a/drivers/md/dm.c
> > > +++ b/drivers/md/dm.c
> > > @@ -3343,6 +3343,12 @@ out:
> > >  	return r;
> > >  }
> > >  
> > > +int dm_suspend_md(struct mapped_device *md)
> > > +{
> > > +	return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> > > +}
> > > +EXPORT_SYMBOL_GPL(dm_suspend_md);
> > > +
> > >  /*
> > >   * Internal suspend/resume works like userspace-driven suspend. It waits
> > >   * until all bios finish and prevents issuing new bios to the target drivers.
> > 
> > To do this properly you should be introducing a variant of
> > dm_internal_suspend.  We currently have two variants:
> > dm_internal_suspend_fast
> > dm_internal_suspend_noflush
> > 
> > The reason to use a dm_internal_suspend variant is this suspend was
> > _not_ initiated by an upper level ioctl (from userspace).  It was
> > done internally from within the target.
> > 
> > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning you're
> > interested in flushing all pending IO (in the FS layered on dm-crupt, if
> > one exists).
> > 
> > But see the comment in __dm_internal_suspend() about TASK_UNINTERRUPTIBLE.
> > If you're OK with the dm-crypt initiated suspend being
> > TASK_UNINTERRUPTIBLE then you could just introduce:
> > 
> > void dm_internal_suspend_uninterruptible_flush(struct mapped_device *md)
> > {
> >         mutex_lock(&md->suspend_lock);
> >         __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> >         mutex_unlock(&md->suspend_lock);
> > }
> > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush);
> > 
> > Otherwise, there is much more extensive DM core changes needed to
> > __dm_internal_suspend() and .presuspend to properly support
> > TASK_INTERRUPTIBLE.
> 
> Hi! I will look at dm_internal_suspend. Anyway use case for suspend is
> from dm-crypt to do both operations: suspend + key wipe. It means that
> without entering key again from userspace, resume is not possible. So my
> question is: It is possible to do internal suspend and then using resume
> from userspace via ioctl?

Good question: no, userspace resume would block waiting for internal
resume.

Soooo... I'll have to look at your patch 3 to understand why dm-crypt is
needing to initiate the suspend internally but then userspace is
initiating the resume... this imbalance is concerning.

Re: [PATCH v2 2/3] dm: Export function dm_suspend_md()

[Mike Snitzer]

On Fri, Jul 17 2015 at 11:22am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Fri, Jul 17 2015 at 10:22am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
> 
> > On Friday 17 July 2015 10:04:39 Mike Snitzer wrote:
> > > On Sun, Jun 21 2015 at  7:20am -0400,
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > 
> > > > This patch exports function dm_suspend_md() which suspend mapped device so other
> > > > kernel drivers can use it and could suspend mapped device when needed.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > >  drivers/md/dm.c |    6 ++++++
> > > >  drivers/md/dm.h |    5 +++++
> > > >  2 files changed, 11 insertions(+)
> > > > 
> > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > > > index 2caf492..03298ff 100644
> > > > --- a/drivers/md/dm.c
> > > > +++ b/drivers/md/dm.c
> > > > @@ -3343,6 +3343,12 @@ out:
> > > >  	return r;
> > > >  }
> > > >  
> > > > +int dm_suspend_md(struct mapped_device *md)
> > > > +{
> > > > +	return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dm_suspend_md);
> > > > +
> > > >  /*
> > > >   * Internal suspend/resume works like userspace-driven suspend. It waits
> > > >   * until all bios finish and prevents issuing new bios to the target drivers.
> > > 
> > > To do this properly you should be introducing a variant of
> > > dm_internal_suspend.  We currently have two variants:
> > > dm_internal_suspend_fast
> > > dm_internal_suspend_noflush
> > > 
> > > The reason to use a dm_internal_suspend variant is this suspend was
> > > _not_ initiated by an upper level ioctl (from userspace).  It was
> > > done internally from within the target.
> > > 
> > > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning you're
> > > interested in flushing all pending IO (in the FS layered on dm-crupt, if
> > > one exists).
> > > 
> > > But see the comment in __dm_internal_suspend() about TASK_UNINTERRUPTIBLE.
> > > If you're OK with the dm-crypt initiated suspend being
> > > TASK_UNINTERRUPTIBLE then you could just introduce:
> > > 
> > > void dm_internal_suspend_uninterruptible_flush(struct mapped_device *md)
> > > {
> > >         mutex_lock(&md->suspend_lock);
> > >         __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> > >         mutex_unlock(&md->suspend_lock);
> > > }
> > > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush);
> > > 
> > > Otherwise, there is much more extensive DM core changes needed to
> > > __dm_internal_suspend() and .presuspend to properly support
> > > TASK_INTERRUPTIBLE.
> > 
> > Hi! I will look at dm_internal_suspend. Anyway use case for suspend is
> > from dm-crypt to do both operations: suspend + key wipe. It means that
> > without entering key again from userspace, resume is not possible. So my
> > question is: It is possible to do internal suspend and then using resume
> > from userspace via ioctl?
> 
> Good question: no, userspace resume would block waiting for internal
> resume.
> 
> Soooo... I'll have to look at your patch 3 to understand why dm-crypt is
> needing to initiate the suspend internally but then userspace is
> initiating the resume... this imbalance is concerning.

Why not introduce a new message that allows you to wipe the key after
suspend?  Both initiated from userspace.

Re: [PATCH v2 2/3] dm: Export function dm_suspend_md()

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 4036 bytes --]

On Friday 17 July 2015 17:30:45 Mike Snitzer wrote:
> On Fri, Jul 17 2015 at 11:22am -0400,
> 
> Mike Snitzer <snitzer@redhat.com> wrote:
> > On Fri, Jul 17 2015 at 10:22am -0400,
> > 
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > On Friday 17 July 2015 10:04:39 Mike Snitzer wrote:
> > > > On Sun, Jun 21 2015 at  7:20am -0400,
> > > > 
> > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > This patch exports function dm_suspend_md() which suspend
> > > > > mapped device so other kernel drivers can use it and could
> > > > > suspend mapped device when needed.
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/md/dm.c |    6 ++++++
> > > > >  drivers/md/dm.h |    5 +++++
> > > > >  2 files changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > > > > index 2caf492..03298ff 100644
> > > > > --- a/drivers/md/dm.c
> > > > > +++ b/drivers/md/dm.c
> > > > > 
> > > > > @@ -3343,6 +3343,12 @@ out:
> > > > >  	return r;
> > > > >  
> > > > >  }
> > > > > 
> > > > > +int dm_suspend_md(struct mapped_device *md)
> > > > > +{
> > > > > +	return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(dm_suspend_md);
> > > > > +
> > > > > 
> > > > >  /*
> > > > >  
> > > > >   * Internal suspend/resume works like userspace-driven
> > > > >   suspend. It waits * until all bios finish and prevents
> > > > >   issuing new bios to the target drivers.
> > > > 
> > > > To do this properly you should be introducing a variant of
> > > > dm_internal_suspend.  We currently have two variants:
> > > > dm_internal_suspend_fast
> > > > dm_internal_suspend_noflush
> > > > 
> > > > The reason to use a dm_internal_suspend variant is this suspend
> > > > was _not_ initiated by an upper level ioctl (from userspace). 
> > > > It was done internally from within the target.
> > > > 
> > > > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning
> > > > you're interested in flushing all pending IO (in the FS
> > > > layered on dm-crupt, if one exists).
> > > > 
> > > > But see the comment in __dm_internal_suspend() about
> > > > TASK_UNINTERRUPTIBLE. If you're OK with the dm-crypt initiated
> > > > suspend being TASK_UNINTERRUPTIBLE then you could just
> > > > introduce:
> > > > 
> > > > void dm_internal_suspend_uninterruptible_flush(struct
> > > > mapped_device *md) {
> > > > 
> > > >         mutex_lock(&md->suspend_lock);
> > > >         __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> > > >         mutex_unlock(&md->suspend_lock);
> > > > 
> > > > }
> > > > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush);
> > > > 
> > > > Otherwise, there is much more extensive DM core changes needed
> > > > to __dm_internal_suspend() and .presuspend to properly support
> > > > TASK_INTERRUPTIBLE.
> > > 
> > > Hi! I will look at dm_internal_suspend. Anyway use case for
> > > suspend is from dm-crypt to do both operations: suspend + key
> > > wipe. It means that without entering key again from userspace,
> > > resume is not possible. So my question is: It is possible to do
> > > internal suspend and then using resume from userspace via ioctl?
> > 
> > Good question: no, userspace resume would block waiting for
> > internal resume.
> > 
> > Soooo... I'll have to look at your patch 3 to understand why
> > dm-crypt is needing to initiate the suspend internally but then
> > userspace is initiating the resume... this imbalance is
> > concerning.
> 
> Why not introduce a new message that allows you to wipe the key after
> suspend?  Both initiated from userspace.

There is already message for wiping key and it will success only if dm 
is suspended.

But this patch series is fixing another problem: wipe key before 
suspend/hibernation action happend and to have it race free it must be 
done after userspace is freezed!

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 2/3] dm: Export function dm_suspend_md()

[Mike Snitzer]

On Fri, Jul 17 2015 at  1:13pm -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:

> On Friday 17 July 2015 17:30:45 Mike Snitzer wrote:
> > On Fri, Jul 17 2015 at 11:22am -0400,
> > 
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > > On Fri, Jul 17 2015 at 10:22am -0400,
> > > 
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > On Friday 17 July 2015 10:04:39 Mike Snitzer wrote:
> > > > > On Sun, Jun 21 2015 at  7:20am -0400,
> > > > > 
> > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > This patch exports function dm_suspend_md() which suspend
> > > > > > mapped device so other kernel drivers can use it and could
> > > > > > suspend mapped device when needed.
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/md/dm.c |    6 ++++++
> > > > > >  drivers/md/dm.h |    5 +++++
> > > > > >  2 files changed, 11 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > > > > > index 2caf492..03298ff 100644
> > > > > > --- a/drivers/md/dm.c
> > > > > > +++ b/drivers/md/dm.c
> > > > > > 
> > > > > > @@ -3343,6 +3343,12 @@ out:
> > > > > >  	return r;
> > > > > >  
> > > > > >  }
> > > > > > 
> > > > > > +int dm_suspend_md(struct mapped_device *md)
> > > > > > +{
> > > > > > +	return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(dm_suspend_md);
> > > > > > +
> > > > > > 
> > > > > >  /*
> > > > > >  
> > > > > >   * Internal suspend/resume works like userspace-driven
> > > > > >   suspend. It waits * until all bios finish and prevents
> > > > > >   issuing new bios to the target drivers.
> > > > > 
> > > > > To do this properly you should be introducing a variant of
> > > > > dm_internal_suspend.  We currently have two variants:
> > > > > dm_internal_suspend_fast
> > > > > dm_internal_suspend_noflush
> > > > > 
> > > > > The reason to use a dm_internal_suspend variant is this suspend
> > > > > was _not_ initiated by an upper level ioctl (from userspace). 
> > > > > It was done internally from within the target.
> > > > > 
> > > > > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning
> > > > > you're interested in flushing all pending IO (in the FS
> > > > > layered on dm-crupt, if one exists).
> > > > > 
> > > > > But see the comment in __dm_internal_suspend() about
> > > > > TASK_UNINTERRUPTIBLE. If you're OK with the dm-crypt initiated
> > > > > suspend being TASK_UNINTERRUPTIBLE then you could just
> > > > > introduce:
> > > > > 
> > > > > void dm_internal_suspend_uninterruptible_flush(struct
> > > > > mapped_device *md) {
> > > > > 
> > > > >         mutex_lock(&md->suspend_lock);
> > > > >         __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG);
> > > > >         mutex_unlock(&md->suspend_lock);
> > > > > 
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush);
> > > > > 
> > > > > Otherwise, there is much more extensive DM core changes needed
> > > > > to __dm_internal_suspend() and .presuspend to properly support
> > > > > TASK_INTERRUPTIBLE.
> > > > 
> > > > Hi! I will look at dm_internal_suspend. Anyway use case for
> > > > suspend is from dm-crypt to do both operations: suspend + key
> > > > wipe. It means that without entering key again from userspace,
> > > > resume is not possible. So my question is: It is possible to do
> > > > internal suspend and then using resume from userspace via ioctl?
> > > 
> > > Good question: no, userspace resume would block waiting for
> > > internal resume.
> > > 
> > > Soooo... I'll have to look at your patch 3 to understand why
> > > dm-crypt is needing to initiate the suspend internally but then
> > > userspace is initiating the resume... this imbalance is
> > > concerning.
> > 
> > Why not introduce a new message that allows you to wipe the key after
> > suspend?  Both initiated from userspace.
> 
> There is already message for wiping key and it will success only if dm 
> is suspended.
> 
> But this patch series is fixing another problem: wipe key before 
> suspend/hibernation action happend and to have it race free it must be 
> done after userspace is freezed!

Yes, I remember now.  So it isn't even userspace initiating the
suspend_and_wipe, it is the PM chain notifier code you're adding.

I'll think more about your use of dm_suspend()

[PATCH v2 3/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

This patch adds dm message commands and option strings to optionally wipe key
from dm-crypt device before entering suspend or hibernate state.

Before key is wiped dm device must be suspended. To prevent race conditions with
I/O and userspace processes, wiping action must be called after processes are
freezed. Otherwise userspace processes could start reading/writing to disk after
dm device is suspened and freezing processes before suspend/hibernate action
will fail.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
Changes since v1:
* Use "retain_on_hibernation" message instead "wipe_on_hibernation 0"
* In crypt_suspend_and_wipe_key() check for errors and return status
* In crypt_status() show also key_wipe_on_hibernation/key_wipe_on_suspend
---
 drivers/md/dm-crypt.c |  126 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 118 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 5503e43..f8536fb 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -23,6 +23,7 @@
 #include <linux/atomic.h>
 #include <linux/scatterlist.h>
 #include <linux/rbtree.h>
+#include <linux/suspend.h>
 #include <asm/page.h>
 #include <asm/unaligned.h>
 #include <crypto/hash.h>
@@ -31,6 +32,8 @@
 
 #include <linux/device-mapper.h>
 
+#include "dm.h"
+
 #define DM_MSG_PREFIX "crypt"
 
 /*
@@ -112,13 +115,18 @@ struct iv_tcw_private {
  * and encrypts / decrypts at the same time.
  */
 enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
-	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD };
+	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
+	     DM_CRYPT_KEY_WIPE_ON_HIBERNATION,
+	     DM_CRYPT_KEY_WIPE_ON_SUSPEND,
+};
 
 /*
  * The fields in here must be read only after initialization.
  */
 struct crypt_config {
 	struct dm_dev *dev;
+	struct dm_target *ti;
+	struct list_head entry;
 	sector_t start;
 
 	/*
@@ -181,6 +189,9 @@ struct crypt_config {
 
 #define MIN_IOS        16
 
+static LIST_HEAD(crypt_list);
+static DEFINE_MUTEX(crypt_list_mtx);
+
 static void clone_init(struct dm_crypt_io *, struct bio *);
 static void kcryptd_queue_crypt(struct dm_crypt_io *io);
 static u8 *iv_of_dmreq(struct crypt_config *cc, struct dm_crypt_request *dmreq);
@@ -1497,12 +1508,33 @@ out:
 
 static int crypt_wipe_key(struct crypt_config *cc)
 {
+	int ret;
+
+	if (cc->iv_gen_ops && cc->iv_gen_ops->wipe) {
+		ret = cc->iv_gen_ops->wipe(cc);
+		if (ret)
+			return ret;
+	}
+
 	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 	memset(&cc->key, 0, cc->key_size * sizeof(u8));
 
 	return crypt_setkey_allcpus(cc);
 }
 
+static int crypt_suspend_and_wipe_key(struct crypt_config *cc)
+{
+	int ret;
+
+	if (!dm_suspended(cc->ti)) {
+		ret = dm_suspend_md(dm_table_get_md(cc->ti->table));
+		if (ret)
+			return ret;
+	}
+
+	return crypt_wipe_key(cc);
+}
+
 static void crypt_dtr(struct dm_target *ti)
 {
 	struct crypt_config *cc = ti->private;
@@ -1512,6 +1544,10 @@ static void crypt_dtr(struct dm_target *ti)
 	if (!cc)
 		return;
 
+	mutex_lock(&crypt_list_mtx);
+	list_del(&cc->entry);
+	mutex_unlock(&crypt_list_mtx);
+
 	if (cc->write_thread)
 		kthread_stop(cc->write_thread);
 
@@ -1738,6 +1774,7 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	cc->key_size = key_size;
 
 	ti->private = cc;
+	cc->ti = ti;
 	ret = crypt_ctr_cipher(ti, argv[0], argv[1]);
 	if (ret < 0)
 		goto bad;
@@ -1833,7 +1870,14 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 			else if (!strcasecmp(opt_string, "submit_from_crypt_cpus"))
 				set_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
 
+			else if (!strcasecmp(opt_string, "key_wipe_on_hibernation"))
+				set_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags);
+
+			else if (!strcasecmp(opt_string, "key_wipe_on_suspend"))
+				set_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags);
+
 			else {
+				ret = -EINVAL;
 				ti->error = "Invalid feature arguments";
 				goto bad;
 			}
@@ -1872,6 +1916,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->num_flush_bios = 1;
 	ti->discard_zeroes_data_unsupported = true;
 
+	mutex_lock(&crypt_list_mtx);
+	list_add(&cc->entry, &crypt_list);
+	mutex_unlock(&crypt_list_mtx);
+
 	return 0;
 
 bad:
@@ -1937,6 +1985,10 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
 		num_feature_args += !!ti->num_discard_bios;
 		num_feature_args += test_bit(DM_CRYPT_SAME_CPU, &cc->flags);
 		num_feature_args += test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
+		num_feature_args += test_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION,
+					     &cc->flags);
+		num_feature_args += test_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND,
+					     &cc->flags);
 		if (num_feature_args) {
 			DMEMIT(" %d", num_feature_args);
 			if (ti->num_discard_bios)
@@ -1945,6 +1997,10 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
 				DMEMIT(" same_cpu_crypt");
 			if (test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags))
 				DMEMIT(" submit_from_crypt_cpus");
+			if (test_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags))
+				DMEMIT(" key_wipe_on_hibernation");
+			if (test_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags))
+				DMEMIT(" key_wipe_on_suspend");
 		}
 
 		break;
@@ -1980,6 +2036,10 @@ static void crypt_resume(struct dm_target *ti)
 /* Message interface
  *	key set <key>
  *	key wipe
+ *	key wipe_on_hibernation
+ *	key retain_on_hibernation
+ *	key wipe_on_suspend
+ *	key retain_on_suspend
  */
 static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 {
@@ -1990,6 +2050,22 @@ static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 		goto error;
 
 	if (!strcasecmp(argv[0], "key")) {
+		if (argc == 2 && !strcasecmp(argv[1], "wipe_on_hibernation")) {
+			set_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags);
+			return 0;
+		}
+		if (argc == 2 && !strcasecmp(argv[1], "retain_on_hibernation")) {
+			clear_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags);
+			return 0;
+		}
+		if (argc == 2 && !strcasecmp(argv[1], "wipe_on_suspend")) {
+			set_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags);
+			return 0;
+		}
+		if (argc == 2 && !strcasecmp(argv[1], "retain_on_suspend")) {
+			clear_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags);
+			return 0;
+		}
 		if (!test_bit(DM_CRYPT_SUSPENDED, &cc->flags)) {
 			DMWARN("not suspended during key manipulation.");
 			return -EINVAL;
@@ -2003,11 +2079,6 @@ static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 			return ret;
 		}
 		if (argc == 2 && !strcasecmp(argv[1], "wipe")) {
-			if (cc->iv_gen_ops && cc->iv_gen_ops->wipe) {
-				ret = cc->iv_gen_ops->wipe(cc);
-				if (ret)
-					return ret;
-			}
 			return crypt_wipe_key(cc);
 		}
 	}
@@ -2056,19 +2127,58 @@ static struct target_type crypt_target = {
 	.iterate_devices = crypt_iterate_devices,
 };
 
+static int dm_crypt_pm_notifier_call(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	struct crypt_config *cc;
+	int r;
+
+	mutex_lock(&crypt_list_mtx);
+
+	list_for_each_entry(cc, &crypt_list, entry) {
+		if ((action == PM_HIBERNATION_AFTER_FREEZE &&
+		     test_bit(DM_CRYPT_KEY_WIPE_ON_HIBERNATION, &cc->flags)) ||
+		    (action == PM_SUSPEND_AFTER_FREEZE &&
+		     test_bit(DM_CRYPT_KEY_WIPE_ON_SUSPEND, &cc->flags))) {
+			r = crypt_suspend_and_wipe_key(cc);
+			if (r) {
+				DMWARN("wiping key failed %d", r);
+			}
+		}
+	}
+
+	mutex_unlock(&crypt_list_mtx);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block dm_crypt_pm_notifier_block = {
+	.notifier_call = dm_crypt_pm_notifier_call,
+};
+
 static int __init dm_crypt_init(void)
 {
 	int r;
 
 	r = dm_register_target(&crypt_target);
-	if (r < 0)
+	if (r < 0) {
 		DMERR("register failed %d", r);
+		return r;
+	}
 
-	return r;
+	r = register_pm_notifier(&dm_crypt_pm_notifier_block);
+	if (r) {
+		DMERR("register_pm_notifier failed %d", r);
+		dm_unregister_target(&crypt_target);
+		return r;
+	}
+
+	return 0;
 }
 
 static void __exit dm_crypt_exit(void)
 {
+	unregister_pm_notifier(&dm_crypt_pm_notifier_block);
 	dm_unregister_target(&crypt_target);
 }
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pavel Machek]

On Sun 2015-06-21 13:20:34, Pali Rohár wrote:
> This patch adds dm message commands and option strings to optionally wipe key
> from dm-crypt device before entering suspend or hibernate state.
> 
> Before key is wiped dm device must be suspended. To prevent race conditions with
> I/O and userspace processes, wiping action must be called after processes are
> freezed. Otherwise userspace processes could start reading/writing to disk after
> dm device is suspened and freezing processes before suspend/hibernate action
> will fail.

Are you sure this is enough?

We still may need to allocate memory after userspace is frozen, and
that could mean writing dirty buffers out to make some memory free...

								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 3/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

On Tuesday 28 July 2015 16:44:19 Pavel Machek wrote:
> On Sun 2015-06-21 13:20:34, Pali Rohár wrote:
> > This patch adds dm message commands and option strings to optionally wipe key
> > from dm-crypt device before entering suspend or hibernate state.
> > 
> > Before key is wiped dm device must be suspended. To prevent race conditions with
> > I/O and userspace processes, wiping action must be called after processes are
> > freezed. Otherwise userspace processes could start reading/writing to disk after
> > dm device is suspened and freezing processes before suspend/hibernate action
> > will fail.
> 
> Are you sure this is enough?
> 
> We still may need to allocate memory after userspace is frozen, and
> that could mean writing dirty buffers out to make some memory free...
> 
> 								Pavel
> 								

Hm... good question. Maybe it is needed to also flush all buffers?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation

[Pali Rohár]

On Sunday 21 June 2015 13:20:31 Pali Rohár wrote:
> This patch series increase security of suspend and hibernate actions. It allows
> user to safely wipe crypto keys before suspend and hibernate actions starts
> without race conditions on userspace process with heavy I/O.
> 
> To automatically wipe cryto key for <device> before hibernate action call:
> $ dmsetup message <device> 0 key wipe_on_hibernation
> 
> To automatically wipe cryto key for <device> before suspend action call:
> $ dmsetup message <device> 0 key wipe_on_suspend
> 
> To disable automatic wipe call retain_on_suspend/retain_on_hibernation.
> 
> Pali Rohár (3):
>   PM suspend/hibernate: Call notifier after freezing processes
>   dm: Export function dm_suspend_md()
>   dm-crypt: Adds support for wiping key when doing suspend/hibernation
> 
>  drivers/md/dm-crypt.c    |  126 +++++++++++++++++++++++++++++++++++++++++++---
>  drivers/md/dm.c          |    6 +++
>  drivers/md/dm.h          |    5 ++
>  include/linux/suspend.h  |    2 +
>  kernel/power/hibernate.c |    2 +
>  kernel/power/suspend.c   |    4 +-
>  6 files changed, 136 insertions(+), 9 deletions(-)
> 

Hello, can somebody look and review this (v2) patch series?

-- 
Pali Rohár
pali.rohar@gmail.com