Introduce the in-kernel hibernation encryption

Use the helper functions introduced previously to encrypt the page data before they are submitted to the block device. Besides, for the case of hibernation compression, the data are firstly compressed and then encrypted, and vice versa for the resume process.

Suggested-by Rafael J. Wysocki <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "Lee, Chun-Yi" <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/power.h |   1 +
 kernel/power/swap.c  | 215 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 205 insertions(+), 11 deletions(-)

kernel/power/power.h +1 -0

index 660aac3..637695c 100644
@@ -207,6 +207,7 @@ extern int swsusp_swap_in_use(void);
 #define SF_PLATFORM_MODE	1
 #define SF_NOCOMPRESS_MODE	2
 #define SF_CRC32_MODE	        4
+#define SF_ENCRYPT_MODE		8
 
 /* kernel/power/hibernate.c */
 extern int swsusp_check(void);

kernel/power/swap.c +204 -11

index c2bcf97..2b6b3d0 100644
@@ -102,14 +102,16 @@ struct swap_map_handle {
 	unsigned int k;
 	unsigned long reqd_free_pages;
 	u32 crc32;
+	bool crypto;
 };
 
 struct swsusp_header {
 	char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) -
-	              sizeof(u32)];
+			sizeof(u32) - HIBERNATE_SALT_BYTES];
 	u32	crc32;
 	sector_t image;
 	unsigned int flags;	/* Flags to pass to the "boot" kernel */
+	char salt[HIBERNATE_SALT_BYTES];
 	char	orig_sig[10];
 	char	sig[10];
 } __packed;

@@ -127,6 +129,53 @@ struct swsusp_extent {
 	unsigned long end;
 };
 
+/* For encryption/decryption. */
+static struct hibernation_crypto *hibernation_crypto_ops;
+
+void set_hibernation_ops(struct hibernation_crypto *ops)
+{
+	hibernation_crypto_ops = ops;
+}
+EXPORT_SYMBOL_GPL(set_hibernation_ops);
+
+static int crypto_data(const char *inbuf,
+			    int inlen,
+			    char *outbuf,
+			    int outlen,
+			    bool encrypt,
+			    int page_idx)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->crypto_data)
+		return hibernation_crypto_ops->crypto_data(inbuf,
+			inlen, outbuf, outlen, encrypt, page_idx);
+	else
+		return -EINVAL;
+}
+
+static void crypto_save(void *outbuf)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->save)
+		hibernation_crypto_ops->save(outbuf);
+}
+
+static void crypto_restore(void *inbuf)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->restore)
+		hibernation_crypto_ops->restore(inbuf);
+}
+
+static int crypto_init(bool suspend)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->init)
+		return hibernation_crypto_ops->init(suspend);
+	else
+		return -EINVAL;
+}
+
 static struct rb_root swsusp_extents = RB_ROOT;
 
 static int swsusp_extents_insert(unsigned long swap_offset)

@@ -318,6 +367,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 		swsusp_header->flags = flags;
 		if (flags & SF_CRC32_MODE)
 			swsusp_header->crc32 = handle->crc32;
+		if (handle->crypto) {
+			swsusp_header->flags |= SF_ENCRYPT_MODE;
+			crypto_save((void *)swsusp_header->salt);
+		}
 		error = hib_submit_io(REQ_OP_WRITE, REQ_SYNC,
 				      swsusp_resume_block, swsusp_header, NULL);
 	} else {

@@ -535,11 +588,12 @@ static int save_image(struct swap_map_handle *handle,
 {
 	unsigned int m;
 	int ret;
-	int nr_pages;
+	int nr_pages, crypto_page_idx;
 	int err2;
 	struct hib_bio_batch hb;
 	ktime_t start;
 	ktime_t stop;
+	void *tmp = NULL, *crypt_buf = NULL;
 
 	hib_init_batch(&hb);
 

@@ -549,12 +603,33 @@ static int save_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
+
 	start = ktime_get();
 	while (1) {
 		ret = snapshot_read_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_write_page(handle, data_of(*snapshot), &hb);
+		tmp = data_of(*snapshot);
+		if (handle->crypto) {
+			/* Encryption before submit_io.*/
+			ret = crypto_data(data_of(*snapshot),
+					  PAGE_SIZE,
+					  crypt_buf,
+					  PAGE_SIZE,
+					  true,
+					  crypto_page_idx);
+			if (ret)
+				goto out;
+			crypto_page_idx++;
+			tmp = crypt_buf;
+		}
+		ret = swap_write_page(handle, tmp, &hb);
 		if (ret)
 			break;
 		if (!(nr_pages % m))

@@ -569,6 +644,9 @@ static int save_image(struct swap_map_handle *handle,
 	if (!ret)
 		pr_info("Image saving done\n");
 	swsusp_show_speed(start, stop, nr_to_write, "Wrote");
+ out:
+	if (crypt_buf)
+		free_page((unsigned long)crypt_buf);
 	return ret;
 }
 

@@ -671,7 +749,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 {
 	unsigned int m;
 	int ret = 0;
-	int nr_pages;
+	int nr_pages, crypto_page_idx;
 	int err2;
 	struct hib_bio_batch hb;
 	ktime_t start;

@@ -767,6 +845,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
 	start = ktime_get();
 	for (;;) {
 		for (thr = 0; thr < nr_threads; thr++) {

@@ -835,7 +914,25 @@ static int save_image_lzo(struct swap_map_handle *handle,
 			for (off = 0;
 			     off < LZO_HEADER + data[thr].cmp_len;
 			     off += PAGE_SIZE) {
-				memcpy(page, data[thr].cmp + off, PAGE_SIZE);
+				if (handle->crypto) {
+					/*
+					 * Encrypt the compressed data
+					 * before we write them to the
+					 * block device.
+					 */
+					ret = crypto_data(data[thr].cmp + off,
+							  PAGE_SIZE,
+							  page,
+							  PAGE_SIZE,
+							  true,
+							  crypto_page_idx);
+					if (ret)
+						goto out_finish;
+					crypto_page_idx++;
+				} else {
+					memcpy(page, data[thr].cmp + off,
+						PAGE_SIZE);
+				}
 
 				ret = swap_write_page(handle, page, &hb);
 				if (ret)

@@ -909,6 +1006,7 @@ int swsusp_write(unsigned int flags)
 	int error;
 
 	pages = snapshot_get_image_size();
+	memset(&handle, 0, sizeof(struct swap_map_handle));
 	error = get_swap_writer(&handle);
 	if (error) {
 		pr_err("Cannot get swap writer\n");

@@ -922,6 +1020,9 @@ int swsusp_write(unsigned int flags)
 		}
 	}
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	if (!crypto_init(true))
+		/* The image needs to be encrypted. */
+		handle.crypto = true;
 	error = snapshot_read_next(&snapshot);
 	if (error < PAGE_SIZE) {
 		if (error >= 0)

@@ -1059,7 +1160,8 @@ static int load_image(struct swap_map_handle *handle,
 	ktime_t stop;
 	struct hib_bio_batch hb;
 	int err2;
-	unsigned nr_pages;
+	unsigned nr_pages, crypto_page_idx;
+	void *crypt_buf = NULL;
 
 	hib_init_batch(&hb);
 

@@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
 	start = ktime_get();
 	for ( ; ; ) {
 		ret = snapshot_write_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_read_page(handle, data_of(*snapshot), &hb);
+		if (handle->crypto)
+			ret = swap_read_page(handle, crypt_buf, &hb);
+		else
+			ret = swap_read_page(handle, data_of(*snapshot), &hb);
 		if (ret)
 			break;
 		if (snapshot->sync_read)
 			ret = hib_wait_io(&hb);
 		if (ret)
 			break;
+		if (handle->crypto) {
+			/*
+			 * Need a decryption for the
+			 * data read from the block
+			 * device.
+			 */
+			ret = crypto_data(crypt_buf, PAGE_SIZE,
+					  data_of(*snapshot),
+					  PAGE_SIZE,
+					  false,
+					  crypto_page_idx);
+			if (ret)
+				break;
+			crypto_page_idx++;
+		}
 		if (!(nr_pages % m))
 			pr_info("Image loading progress: %3d%%\n",
 				nr_pages / m * 10);

@@ -1097,6 +1223,8 @@ static int load_image(struct swap_map_handle *handle,
 			ret = -ENODATA;
 	}
 	swsusp_show_speed(start, stop, nr_to_read, "Read");
+	if (crypt_buf)
+		free_page((unsigned long)crypt_buf);
 	return ret;
 }
 

@@ -1164,7 +1292,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	struct hib_bio_batch hb;
 	ktime_t start;
 	ktime_t stop;
-	unsigned nr_pages;
+	unsigned nr_pages, crypto_page_idx;
 	size_t off;
 	unsigned i, thr, run_threads, nr_threads;
 	unsigned ring = 0, pg = 0, ring_size = 0,

@@ -1173,6 +1301,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	unsigned char **page = NULL;
 	struct dec_data *data = NULL;
 	struct crc_data *crc = NULL;
+	void *first_page = NULL;
 
 	hib_init_batch(&hb);
 

@@ -1278,6 +1407,18 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	}
 	want = ring_size = i;
 
+	/*
+	 * The first page of data[thr] contains the length of
+	 * compressed data, this page should not mess up the
+	 * read buffer, so we allocate a separate page for it.
+	 */
+	if (handle->crypto) {
+		first_page = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!first_page) {
+			ret = -ENOMEM;
+			goto out_clean;
+		}
+	}
 	pr_info("Using %u thread(s) for decompression\n", nr_threads);
 	pr_info("Loading and decompressing image data (%u pages)...\n",
 		nr_to_read);

@@ -1285,6 +1426,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
 	start = ktime_get();
 
 	ret = snapshot_write_next(snapshot);

@@ -1336,7 +1478,24 @@ static int load_image_lzo(struct swap_map_handle *handle,
 		}
 
 		for (thr = 0; have && thr < nr_threads; thr++) {
-			data[thr].cmp_len = *(size_t *)page[pg];
+			if (handle->crypto) {
+				/*
+				 * Need to decrypt the first page
+				 * of each data[thr], which contains
+				 * the compressed data length.
+				 */
+				ret = crypto_data(page[pg],
+						  PAGE_SIZE,
+						  first_page,
+						  PAGE_SIZE,
+						  false,
+						  crypto_page_idx);
+				if (ret)
+					goto out_finish;
+				data[thr].cmp_len = *(size_t *)first_page;
+			} else {
+				data[thr].cmp_len = *(size_t *)page[pg];
+			}
 			if (unlikely(!data[thr].cmp_len ||
 			             data[thr].cmp_len >
 			             lzo1x_worst_compress(LZO_UNC_SIZE))) {

@@ -1358,8 +1517,26 @@ static int load_image_lzo(struct swap_map_handle *handle,
 			for (off = 0;
 			     off < LZO_HEADER + data[thr].cmp_len;
 			     off += PAGE_SIZE) {
-				memcpy(data[thr].cmp + off,
-				       page[pg], PAGE_SIZE);
+				if (handle->crypto) {
+					/*
+					 * Decrypt the compressed data
+					 * and leverage the decompression
+					 * threads to get it done.
+					 */
+					ret = crypto_data(page[pg],
+							  PAGE_SIZE,
+							  data[thr].cmp + off,
+							  PAGE_SIZE,
+							  false,
+							  crypto_page_idx);
+					if (ret)
+						goto out_finish;
+					crypto_page_idx++;
+				} else {
+					memcpy(data[thr].cmp + off,
+						page[pg], PAGE_SIZE);
+
+				}
 				have--;
 				want++;
 				if (++pg >= ring_size)

@@ -1452,6 +1629,8 @@ static int load_image_lzo(struct swap_map_handle *handle,
 out_clean:
 	for (i = 0; i < ring_size; i++)
 		free_page((unsigned long)page[i]);
+	if (first_page)
+		free_page((unsigned long)first_page);
 	if (crc) {
 		if (crc->thr)
 			kthread_stop(crc->thr);

@@ -1482,6 +1661,7 @@ int swsusp_read(unsigned int *flags_p)
 	struct swsusp_info *header;
 
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	memset(&handle, 0, sizeof(struct swap_map_handle));
 	error = snapshot_write_next(&snapshot);
 	if (error < PAGE_SIZE)
 		return error < 0 ? error : -EFAULT;

@@ -1489,6 +1669,16 @@ int swsusp_read(unsigned int *flags_p)
 	error = get_swap_reader(&handle, flags_p);
 	if (error)
 		goto end;
+	if (*flags_p & SF_ENCRYPT_MODE) {
+		error = crypto_init(false);
+		if (!error) {
+			/* The image has been encrypted. */
+			handle.crypto = true;
+		} else {
+			pr_err("Failed to init cipher during resume.\n");
+			goto end;
+		}
+	}
 	if (!error)
 		error = swap_read_page(&handle, header, NULL);
 	if (!error) {

@@ -1526,6 +1716,9 @@ int swsusp_check(void)
 
 		if (!memcmp(HIBERNATE_SIG, swsusp_header->sig, 10)) {
 			memcpy(swsusp_header->sig, swsusp_header->orig_sig, 10);
+			/* Read salt passed from previous kernel. */
+			if (swsusp_header->flags & SF_ENCRYPT_MODE)
+				crypto_restore((void *)&swsusp_header->salt);
 			/* Reset swap signature now */
 			error = hib_submit_io(REQ_OP_WRITE, REQ_SYNC,
 						swsusp_resume_block,

Signature · 1 line

2.7.4

Hi Chen Yu,

Quoted 6 lines · Chen Yu

Use the helper functions introduced previously to encrypt the page data before they are submitted to the block device. Besides, for the case of hibernation compression, the data are firstly compressed and then encrypted, and vice versa for the resume process.

I want to suggest my solution that it direct signs/encrypts the memory snapshot image. This solution is already shipped with SLE12 a couple of years:

https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3

The above patches still need to clean up. I am working on some other bugs, but I can clean up and send out it ASAP.

The advantage of this solution is that it produces a signed and encrypted image. Not just for writing to block device by kernel, it also can provide a signed/encrypted image to user space. User space can store the encrypted image to anywhere.

I am OK for your user space key generator because I didn't have similar solution yet. I am working on the EFI master key and also want to adapt hibernation to keyring. I will continue the works.

Thanks a lot!
Joey Lee

Quoted 443 lines

Suggested-by Rafael J. Wysocki <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "Lee, Chun-Yi" <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/power.h |   1 +
 kernel/power/swap.c  | 215 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 205 insertions(+), 11 deletions(-)

kernel/power/power.h +1 -0

index 660aac3..637695c 100644
@@ -207,6 +207,7 @@ extern int swsusp_swap_in_use(void);
 #define SF_PLATFORM_MODE	1
 #define SF_NOCOMPRESS_MODE	2
 #define SF_CRC32_MODE	        4
+#define SF_ENCRYPT_MODE		8
 
 /* kernel/power/hibernate.c */
 extern int swsusp_check(void);

kernel/power/swap.c +204 -11

index c2bcf97..2b6b3d0 100644
@@ -102,14 +102,16 @@ struct swap_map_handle {
 	unsigned int k;
 	unsigned long reqd_free_pages;
 	u32 crc32;
+	bool crypto;
 };
 
 struct swsusp_header {
 	char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) -
-	              sizeof(u32)];
+			sizeof(u32) - HIBERNATE_SALT_BYTES];
 	u32	crc32;
 	sector_t image;
 	unsigned int flags;	/* Flags to pass to the "boot" kernel */
+	char salt[HIBERNATE_SALT_BYTES];
 	char	orig_sig[10];
 	char	sig[10];
 } __packed;

@@ -127,6 +129,53 @@ struct swsusp_extent {
 	unsigned long end;
 };
 
+/* For encryption/decryption. */
+static struct hibernation_crypto *hibernation_crypto_ops;
+
+void set_hibernation_ops(struct hibernation_crypto *ops)
+{
+	hibernation_crypto_ops = ops;
+}
+EXPORT_SYMBOL_GPL(set_hibernation_ops);
+
+static int crypto_data(const char *inbuf,
+			    int inlen,
+			    char *outbuf,
+			    int outlen,
+			    bool encrypt,
+			    int page_idx)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->crypto_data)
+		return hibernation_crypto_ops->crypto_data(inbuf,
+			inlen, outbuf, outlen, encrypt, page_idx);
+	else
+		return -EINVAL;
+}
+
+static void crypto_save(void *outbuf)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->save)
+		hibernation_crypto_ops->save(outbuf);
+}
+
+static void crypto_restore(void *inbuf)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->restore)
+		hibernation_crypto_ops->restore(inbuf);
+}
+
+static int crypto_init(bool suspend)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->init)
+		return hibernation_crypto_ops->init(suspend);
+	else
+		return -EINVAL;
+}
+
 static struct rb_root swsusp_extents = RB_ROOT;
 
 static int swsusp_extents_insert(unsigned long swap_offset)

@@ -318,6 +367,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 		swsusp_header->flags = flags;
 		if (flags & SF_CRC32_MODE)
 			swsusp_header->crc32 = handle->crc32;
+		if (handle->crypto) {
+			swsusp_header->flags |= SF_ENCRYPT_MODE;
+			crypto_save((void *)swsusp_header->salt);
+		}
 		error = hib_submit_io(REQ_OP_WRITE, REQ_SYNC,
 				      swsusp_resume_block, swsusp_header, NULL);
 	} else {

@@ -535,11 +588,12 @@ static int save_image(struct swap_map_handle *handle,
 {
 	unsigned int m;
 	int ret;
-	int nr_pages;
+	int nr_pages, crypto_page_idx;
 	int err2;
 	struct hib_bio_batch hb;
 	ktime_t start;
 	ktime_t stop;
+	void *tmp = NULL, *crypt_buf = NULL;
 
 	hib_init_batch(&hb);
 

@@ -549,12 +603,33 @@ static int save_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
+
 	start = ktime_get();
 	while (1) {
 		ret = snapshot_read_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_write_page(handle, data_of(*snapshot), &hb);
+		tmp = data_of(*snapshot);
+		if (handle->crypto) {
+			/* Encryption before submit_io.*/
+			ret = crypto_data(data_of(*snapshot),
+					  PAGE_SIZE,
+					  crypt_buf,
+					  PAGE_SIZE,
+					  true,
+					  crypto_page_idx);
+			if (ret)
+				goto out;
+			crypto_page_idx++;
+			tmp = crypt_buf;
+		}
+		ret = swap_write_page(handle, tmp, &hb);
 		if (ret)
 			break;
 		if (!(nr_pages % m))

@@ -569,6 +644,9 @@ static int save_image(struct swap_map_handle *handle,
 	if (!ret)
 		pr_info("Image saving done\n");
 	swsusp_show_speed(start, stop, nr_to_write, "Wrote");
+ out:
+	if (crypt_buf)
+		free_page((unsigned long)crypt_buf);
 	return ret;
 }
 

@@ -671,7 +749,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 {
 	unsigned int m;
 	int ret = 0;
-	int nr_pages;
+	int nr_pages, crypto_page_idx;
 	int err2;
 	struct hib_bio_batch hb;
 	ktime_t start;

@@ -767,6 +845,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
 	start = ktime_get();
 	for (;;) {
 		for (thr = 0; thr < nr_threads; thr++) {

@@ -835,7 +914,25 @@ static int save_image_lzo(struct swap_map_handle *handle,
 			for (off = 0;
 			     off < LZO_HEADER + data[thr].cmp_len;
 			     off += PAGE_SIZE) {
-				memcpy(page, data[thr].cmp + off, PAGE_SIZE);
+				if (handle->crypto) {
+					/*
+					 * Encrypt the compressed data
+					 * before we write them to the
+					 * block device.
+					 */
+					ret = crypto_data(data[thr].cmp + off,
+							  PAGE_SIZE,
+							  page,
+							  PAGE_SIZE,
+							  true,
+							  crypto_page_idx);
+					if (ret)
+						goto out_finish;
+					crypto_page_idx++;
+				} else {
+					memcpy(page, data[thr].cmp + off,
+						PAGE_SIZE);
+				}
 
 				ret = swap_write_page(handle, page, &hb);
 				if (ret)

@@ -909,6 +1006,7 @@ int swsusp_write(unsigned int flags)
 	int error;
 
 	pages = snapshot_get_image_size();
+	memset(&handle, 0, sizeof(struct swap_map_handle));
 	error = get_swap_writer(&handle);
 	if (error) {
 		pr_err("Cannot get swap writer\n");

@@ -922,6 +1020,9 @@ int swsusp_write(unsigned int flags)
 		}
 	}
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	if (!crypto_init(true))
+		/* The image needs to be encrypted. */
+		handle.crypto = true;
 	error = snapshot_read_next(&snapshot);
 	if (error < PAGE_SIZE) {
 		if (error >= 0)

@@ -1059,7 +1160,8 @@ static int load_image(struct swap_map_handle *handle,
 	ktime_t stop;
 	struct hib_bio_batch hb;
 	int err2;
-	unsigned nr_pages;
+	unsigned nr_pages, crypto_page_idx;
+	void *crypt_buf = NULL;
 
 	hib_init_batch(&hb);
 

@@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
 	start = ktime_get();
 	for ( ; ; ) {
 		ret = snapshot_write_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_read_page(handle, data_of(*snapshot), &hb);
+		if (handle->crypto)
+			ret = swap_read_page(handle, crypt_buf, &hb);
+		else
+			ret = swap_read_page(handle, data_of(*snapshot), &hb);
 		if (ret)
 			break;
 		if (snapshot->sync_read)
 			ret = hib_wait_io(&hb);
 		if (ret)
 			break;
+		if (handle->crypto) {
+			/*
+			 * Need a decryption for the
+			 * data read from the block
+			 * device.
+			 */
+			ret = crypto_data(crypt_buf, PAGE_SIZE,
+					  data_of(*snapshot),
+					  PAGE_SIZE,
+					  false,
+					  crypto_page_idx);
+			if (ret)
+				break;
+			crypto_page_idx++;
+		}
 		if (!(nr_pages % m))
 			pr_info("Image loading progress: %3d%%\n",
 				nr_pages / m * 10);

@@ -1097,6 +1223,8 @@ static int load_image(struct swap_map_handle *handle,
 			ret = -ENODATA;
 	}
 	swsusp_show_speed(start, stop, nr_to_read, "Read");
+	if (crypt_buf)
+		free_page((unsigned long)crypt_buf);
 	return ret;
 }
 

@@ -1164,7 +1292,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	struct hib_bio_batch hb;
 	ktime_t start;
 	ktime_t stop;
-	unsigned nr_pages;
+	unsigned nr_pages, crypto_page_idx;
 	size_t off;
 	unsigned i, thr, run_threads, nr_threads;
 	unsigned ring = 0, pg = 0, ring_size = 0,

@@ -1173,6 +1301,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	unsigned char **page = NULL;
 	struct dec_data *data = NULL;
 	struct crc_data *crc = NULL;
+	void *first_page = NULL;
 
 	hib_init_batch(&hb);
 

@@ -1278,6 +1407,18 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	}
 	want = ring_size = i;
 
+	/*
+	 * The first page of data[thr] contains the length of
+	 * compressed data, this page should not mess up the
+	 * read buffer, so we allocate a separate page for it.
+	 */
+	if (handle->crypto) {
+		first_page = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!first_page) {
+			ret = -ENOMEM;
+			goto out_clean;
+		}
+	}
 	pr_info("Using %u thread(s) for decompression\n", nr_threads);
 	pr_info("Loading and decompressing image data (%u pages)...\n",
 		nr_to_read);

@@ -1285,6 +1426,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
 	start = ktime_get();
 
 	ret = snapshot_write_next(snapshot);

@@ -1336,7 +1478,24 @@ static int load_image_lzo(struct swap_map_handle *handle,
 		}
 
 		for (thr = 0; have && thr < nr_threads; thr++) {
-			data[thr].cmp_len = *(size_t *)page[pg];
+			if (handle->crypto) {
+				/*
+				 * Need to decrypt the first page
+				 * of each data[thr], which contains
+				 * the compressed data length.
+				 */
+				ret = crypto_data(page[pg],
+						  PAGE_SIZE,
+						  first_page,
+						  PAGE_SIZE,
+						  false,
+						  crypto_page_idx);
+				if (ret)
+					goto out_finish;
+				data[thr].cmp_len = *(size_t *)first_page;
+			} else {
+				data[thr].cmp_len = *(size_t *)page[pg];
+			}
 			if (unlikely(!data[thr].cmp_len ||
 			             data[thr].cmp_len >
 			             lzo1x_worst_compress(LZO_UNC_SIZE))) {

@@ -1358,8 +1517,26 @@ static int load_image_lzo(struct swap_map_handle *handle,
 			for (off = 0;
 			     off < LZO_HEADER + data[thr].cmp_len;
 			     off += PAGE_SIZE) {
-				memcpy(data[thr].cmp + off,
-				       page[pg], PAGE_SIZE);
+				if (handle->crypto) {
+					/*
+					 * Decrypt the compressed data
+					 * and leverage the decompression
+					 * threads to get it done.
+					 */
+					ret = crypto_data(page[pg],
+							  PAGE_SIZE,
+							  data[thr].cmp + off,
+							  PAGE_SIZE,
+							  false,
+							  crypto_page_idx);
+					if (ret)
+						goto out_finish;
+					crypto_page_idx++;
+				} else {
+					memcpy(data[thr].cmp + off,
+						page[pg], PAGE_SIZE);
+
+				}
 				have--;
 				want++;
 				if (++pg >= ring_size)

@@ -1452,6 +1629,8 @@ static int load_image_lzo(struct swap_map_handle *handle,
 out_clean:
 	for (i = 0; i < ring_size; i++)
 		free_page((unsigned long)page[i]);
+	if (first_page)
+		free_page((unsigned long)first_page);
 	if (crc) {
 		if (crc->thr)
 			kthread_stop(crc->thr);

@@ -1482,6 +1661,7 @@ int swsusp_read(unsigned int *flags_p)
 	struct swsusp_info *header;
 
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	memset(&handle, 0, sizeof(struct swap_map_handle));
 	error = snapshot_write_next(&snapshot);
 	if (error < PAGE_SIZE)
 		return error < 0 ? error : -EFAULT;

@@ -1489,6 +1669,16 @@ int swsusp_read(unsigned int *flags_p)
 	error = get_swap_reader(&handle, flags_p);
 	if (error)
 		goto end;
+	if (*flags_p & SF_ENCRYPT_MODE) {
+		error = crypto_init(false);
+		if (!error) {
+			/* The image has been encrypted. */
+			handle.crypto = true;
+		} else {
+			pr_err("Failed to init cipher during resume.\n");
+			goto end;
+		}
+	}
 	if (!error)
 		error = swap_read_page(&handle, header, NULL);
 	if (!error) {

@@ -1526,6 +1716,9 @@ int swsusp_check(void)
 
 		if (!memcmp(HIBERNATE_SIG, swsusp_header->sig, 10)) {
 			memcpy(swsusp_header->sig, swsusp_header->orig_sig, 10);
+			/* Read salt passed from previous kernel. */
+			if (swsusp_header->flags & SF_ENCRYPT_MODE)
+				crypto_restore((void *)&swsusp_header->salt);
 			/* Reset swap signature now */
 			error = hib_submit_io(REQ_OP_WRITE, REQ_SYNC,
 						swsusp_resume_block,

Signature · 1 line

2.7.4

Hi,

Quoted 10 lines · joeyli

Hi Chen Yu,

Quoted 6 lines · Chen Yu

Use the helper functions introduced previously to encrypt the page data before they are submitted to the block device. Besides, for the case of hibernation compression, the data are firstly compressed and then encrypted, and vice versa for the resume process.

Quoted 6 lines

I want to suggest my solution that it direct signs/encrypts the memory snapshot image. This solution is already shipped with SLE12 a couple of years:

https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3

I did not see image page encryption in above link, if I understand

correctly, your code focus on signation and encrypt the hidden data, but not target for the whole snapshot data.

Quoted 8 lines

The above patches still need to clean up. I am working on some other bugs, but I can clean up and send out it ASAP.

The advantage of this solution is that it produces a signed and encrypted image. Not just for writing to block device by kernel, it also can provide a signed/encrypted image to user space. User space can store the encrypted image to anywhere.

Quoted 4 lines

I am OK for your user space key generator because I didn't have similar solution yet. I am working on the EFI master key and also want to adapt hibernation to keyring. I will continue the works.

The user space tool can easily add the keyring support besides ioctl if needed.

Best,
Yu

Quoted 446 lines

Thanks a lot!
Joey Lee

Quoted 443 lines

Suggested-by Rafael J. Wysocki <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "Lee, Chun-Yi" <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/power.h |   1 +
 kernel/power/swap.c  | 215 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 205 insertions(+), 11 deletions(-)

kernel/power/power.h +1 -0

index 660aac3..637695c 100644
@@ -207,6 +207,7 @@ extern int swsusp_swap_in_use(void);
 #define SF_PLATFORM_MODE	1
 #define SF_NOCOMPRESS_MODE	2
 #define SF_CRC32_MODE	        4
+#define SF_ENCRYPT_MODE		8
 
 /* kernel/power/hibernate.c */
 extern int swsusp_check(void);

kernel/power/swap.c +204 -11

index c2bcf97..2b6b3d0 100644
@@ -102,14 +102,16 @@ struct swap_map_handle {
 	unsigned int k;
 	unsigned long reqd_free_pages;
 	u32 crc32;
+	bool crypto;
 };
 
 struct swsusp_header {
 	char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) -
-	              sizeof(u32)];
+			sizeof(u32) - HIBERNATE_SALT_BYTES];
 	u32	crc32;
 	sector_t image;
 	unsigned int flags;	/* Flags to pass to the "boot" kernel */
+	char salt[HIBERNATE_SALT_BYTES];
 	char	orig_sig[10];
 	char	sig[10];
 } __packed;

@@ -127,6 +129,53 @@ struct swsusp_extent {
 	unsigned long end;
 };
 
+/* For encryption/decryption. */
+static struct hibernation_crypto *hibernation_crypto_ops;
+
+void set_hibernation_ops(struct hibernation_crypto *ops)
+{
+	hibernation_crypto_ops = ops;
+}
+EXPORT_SYMBOL_GPL(set_hibernation_ops);
+
+static int crypto_data(const char *inbuf,
+			    int inlen,
+			    char *outbuf,
+			    int outlen,
+			    bool encrypt,
+			    int page_idx)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->crypto_data)
+		return hibernation_crypto_ops->crypto_data(inbuf,
+			inlen, outbuf, outlen, encrypt, page_idx);
+	else
+		return -EINVAL;
+}
+
+static void crypto_save(void *outbuf)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->save)
+		hibernation_crypto_ops->save(outbuf);
+}
+
+static void crypto_restore(void *inbuf)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->restore)
+		hibernation_crypto_ops->restore(inbuf);
+}
+
+static int crypto_init(bool suspend)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->init)
+		return hibernation_crypto_ops->init(suspend);
+	else
+		return -EINVAL;
+}
+
 static struct rb_root swsusp_extents = RB_ROOT;
 
 static int swsusp_extents_insert(unsigned long swap_offset)

@@ -318,6 +367,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 		swsusp_header->flags = flags;
 		if (flags & SF_CRC32_MODE)
 			swsusp_header->crc32 = handle->crc32;
+		if (handle->crypto) {
+			swsusp_header->flags |= SF_ENCRYPT_MODE;
+			crypto_save((void *)swsusp_header->salt);
+		}
 		error = hib_submit_io(REQ_OP_WRITE, REQ_SYNC,
 				      swsusp_resume_block, swsusp_header, NULL);
 	} else {

@@ -535,11 +588,12 @@ static int save_image(struct swap_map_handle *handle,
 {
 	unsigned int m;
 	int ret;
-	int nr_pages;
+	int nr_pages, crypto_page_idx;
 	int err2;
 	struct hib_bio_batch hb;
 	ktime_t start;
 	ktime_t stop;
+	void *tmp = NULL, *crypt_buf = NULL;
 
 	hib_init_batch(&hb);
 

@@ -549,12 +603,33 @@ static int save_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
+
 	start = ktime_get();
 	while (1) {
 		ret = snapshot_read_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_write_page(handle, data_of(*snapshot), &hb);
+		tmp = data_of(*snapshot);
+		if (handle->crypto) {
+			/* Encryption before submit_io.*/
+			ret = crypto_data(data_of(*snapshot),
+					  PAGE_SIZE,
+					  crypt_buf,
+					  PAGE_SIZE,
+					  true,
+					  crypto_page_idx);
+			if (ret)
+				goto out;
+			crypto_page_idx++;
+			tmp = crypt_buf;
+		}
+		ret = swap_write_page(handle, tmp, &hb);
 		if (ret)
 			break;
 		if (!(nr_pages % m))

@@ -569,6 +644,9 @@ static int save_image(struct swap_map_handle *handle,
 	if (!ret)
 		pr_info("Image saving done\n");
 	swsusp_show_speed(start, stop, nr_to_write, "Wrote");
+ out:
+	if (crypt_buf)
+		free_page((unsigned long)crypt_buf);
 	return ret;
 }
 

@@ -671,7 +749,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 {
 	unsigned int m;
 	int ret = 0;
-	int nr_pages;
+	int nr_pages, crypto_page_idx;
 	int err2;
 	struct hib_bio_batch hb;
 	ktime_t start;

@@ -767,6 +845,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
 	start = ktime_get();
 	for (;;) {
 		for (thr = 0; thr < nr_threads; thr++) {

@@ -835,7 +914,25 @@ static int save_image_lzo(struct swap_map_handle *handle,
 			for (off = 0;
 			     off < LZO_HEADER + data[thr].cmp_len;
 			     off += PAGE_SIZE) {
-				memcpy(page, data[thr].cmp + off, PAGE_SIZE);
+				if (handle->crypto) {
+					/*
+					 * Encrypt the compressed data
+					 * before we write them to the
+					 * block device.
+					 */
+					ret = crypto_data(data[thr].cmp + off,
+							  PAGE_SIZE,
+							  page,
+							  PAGE_SIZE,
+							  true,
+							  crypto_page_idx);
+					if (ret)
+						goto out_finish;
+					crypto_page_idx++;
+				} else {
+					memcpy(page, data[thr].cmp + off,
+						PAGE_SIZE);
+				}
 
 				ret = swap_write_page(handle, page, &hb);
 				if (ret)

@@ -909,6 +1006,7 @@ int swsusp_write(unsigned int flags)
 	int error;
 
 	pages = snapshot_get_image_size();
+	memset(&handle, 0, sizeof(struct swap_map_handle));
 	error = get_swap_writer(&handle);
 	if (error) {
 		pr_err("Cannot get swap writer\n");

@@ -922,6 +1020,9 @@ int swsusp_write(unsigned int flags)
 		}
 	}
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	if (!crypto_init(true))
+		/* The image needs to be encrypted. */
+		handle.crypto = true;
 	error = snapshot_read_next(&snapshot);
 	if (error < PAGE_SIZE) {
 		if (error >= 0)

@@ -1059,7 +1160,8 @@ static int load_image(struct swap_map_handle *handle,
 	ktime_t stop;
 	struct hib_bio_batch hb;
 	int err2;
-	unsigned nr_pages;
+	unsigned nr_pages, crypto_page_idx;
+	void *crypt_buf = NULL;
 
 	hib_init_batch(&hb);
 

@@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
 	start = ktime_get();
 	for ( ; ; ) {
 		ret = snapshot_write_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_read_page(handle, data_of(*snapshot), &hb);
+		if (handle->crypto)
+			ret = swap_read_page(handle, crypt_buf, &hb);
+		else
+			ret = swap_read_page(handle, data_of(*snapshot), &hb);
 		if (ret)
 			break;
 		if (snapshot->sync_read)
 			ret = hib_wait_io(&hb);
 		if (ret)
 			break;
+		if (handle->crypto) {
+			/*
+			 * Need a decryption for the
+			 * data read from the block
+			 * device.
+			 */
+			ret = crypto_data(crypt_buf, PAGE_SIZE,
+					  data_of(*snapshot),
+					  PAGE_SIZE,
+					  false,
+					  crypto_page_idx);
+			if (ret)
+				break;
+			crypto_page_idx++;
+		}
 		if (!(nr_pages % m))
 			pr_info("Image loading progress: %3d%%\n",
 				nr_pages / m * 10);

@@ -1097,6 +1223,8 @@ static int load_image(struct swap_map_handle *handle,
 			ret = -ENODATA;
 	}
 	swsusp_show_speed(start, stop, nr_to_read, "Read");
+	if (crypt_buf)
+		free_page((unsigned long)crypt_buf);
 	return ret;
 }
 

@@ -1164,7 +1292,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	struct hib_bio_batch hb;
 	ktime_t start;
 	ktime_t stop;
-	unsigned nr_pages;
+	unsigned nr_pages, crypto_page_idx;
 	size_t off;
 	unsigned i, thr, run_threads, nr_threads;
 	unsigned ring = 0, pg = 0, ring_size = 0,

@@ -1173,6 +1301,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	unsigned char **page = NULL;
 	struct dec_data *data = NULL;
 	struct crc_data *crc = NULL;
+	void *first_page = NULL;
 
 	hib_init_batch(&hb);
 

@@ -1278,6 +1407,18 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	}
 	want = ring_size = i;
 
+	/*
+	 * The first page of data[thr] contains the length of
+	 * compressed data, this page should not mess up the
+	 * read buffer, so we allocate a separate page for it.
+	 */
+	if (handle->crypto) {
+		first_page = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!first_page) {
+			ret = -ENOMEM;
+			goto out_clean;
+		}
+	}
 	pr_info("Using %u thread(s) for decompression\n", nr_threads);
 	pr_info("Loading and decompressing image data (%u pages)...\n",
 		nr_to_read);

@@ -1285,6 +1426,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
 	start = ktime_get();
 
 	ret = snapshot_write_next(snapshot);

@@ -1336,7 +1478,24 @@ static int load_image_lzo(struct swap_map_handle *handle,
 		}
 
 		for (thr = 0; have && thr < nr_threads; thr++) {
-			data[thr].cmp_len = *(size_t *)page[pg];
+			if (handle->crypto) {
+				/*
+				 * Need to decrypt the first page
+				 * of each data[thr], which contains
+				 * the compressed data length.
+				 */
+				ret = crypto_data(page[pg],
+						  PAGE_SIZE,
+						  first_page,
+						  PAGE_SIZE,
+						  false,
+						  crypto_page_idx);
+				if (ret)
+					goto out_finish;
+				data[thr].cmp_len = *(size_t *)first_page;
+			} else {
+				data[thr].cmp_len = *(size_t *)page[pg];
+			}
 			if (unlikely(!data[thr].cmp_len ||
 			             data[thr].cmp_len >
 			             lzo1x_worst_compress(LZO_UNC_SIZE))) {

@@ -1358,8 +1517,26 @@ static int load_image_lzo(struct swap_map_handle *handle,
 			for (off = 0;
 			     off < LZO_HEADER + data[thr].cmp_len;
 			     off += PAGE_SIZE) {
-				memcpy(data[thr].cmp + off,
-				       page[pg], PAGE_SIZE);
+				if (handle->crypto) {
+					/*
+					 * Decrypt the compressed data
+					 * and leverage the decompression
+					 * threads to get it done.
+					 */
+					ret = crypto_data(page[pg],
+							  PAGE_SIZE,
+							  data[thr].cmp + off,
+							  PAGE_SIZE,
+							  false,
+							  crypto_page_idx);
+					if (ret)
+						goto out_finish;
+					crypto_page_idx++;
+				} else {
+					memcpy(data[thr].cmp + off,
+						page[pg], PAGE_SIZE);
+
+				}
 				have--;
 				want++;
 				if (++pg >= ring_size)

@@ -1452,6 +1629,8 @@ static int load_image_lzo(struct swap_map_handle *handle,
 out_clean:
 	for (i = 0; i < ring_size; i++)
 		free_page((unsigned long)page[i]);
+	if (first_page)
+		free_page((unsigned long)first_page);
 	if (crc) {
 		if (crc->thr)
 			kthread_stop(crc->thr);

@@ -1482,6 +1661,7 @@ int swsusp_read(unsigned int *flags_p)
 	struct swsusp_info *header;
 
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	memset(&handle, 0, sizeof(struct swap_map_handle));
 	error = snapshot_write_next(&snapshot);
 	if (error < PAGE_SIZE)
 		return error < 0 ? error : -EFAULT;

@@ -1489,6 +1669,16 @@ int swsusp_read(unsigned int *flags_p)
 	error = get_swap_reader(&handle, flags_p);
 	if (error)
 		goto end;
+	if (*flags_p & SF_ENCRYPT_MODE) {
+		error = crypto_init(false);
+		if (!error) {
+			/* The image has been encrypted. */
+			handle.crypto = true;
+		} else {
+			pr_err("Failed to init cipher during resume.\n");
+			goto end;
+		}
+	}
 	if (!error)
 		error = swap_read_page(&handle, header, NULL);
 	if (!error) {

@@ -1526,6 +1716,9 @@ int swsusp_check(void)
 
 		if (!memcmp(HIBERNATE_SIG, swsusp_header->sig, 10)) {
 			memcpy(swsusp_header->sig, swsusp_header->orig_sig, 10);
+			/* Read salt passed from previous kernel. */
+			if (swsusp_header->flags & SF_ENCRYPT_MODE)
+				crypto_restore((void *)&swsusp_header->salt);
 			/* Reset swap signature now */
 			error = hib_submit_io(REQ_OP_WRITE, REQ_SYNC,
 						swsusp_resume_block,

Signature · 1 line

2.7.4

Quoted 12 lines · Yu Chen

Hi,

Quoted 10 lines · joeyli

Hi Chen Yu,

Quoted 6 lines · Chen Yu

Use the helper functions introduced previously to encrypt the page data before they are submitted to the block device. Besides, for the case of hibernation compression, the data are firstly compressed and then encrypted, and vice versa for the resume process.

Quoted 7 lines

Quoted 6 lines

I want to suggest my solution that it direct signs/encrypts the memory snapshot image. This solution is already shipped with SLE12 a couple of years:

https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3

I did not see image page encryption in above link, if I understand

PM / hibernate: Generate and verify signature for snapshot image
https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f

PM / hibernate: snapshot image encryption
https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929

The above patches sign and encrypt the data pages in snapshot image. It puts the signature to header.

Quoted 2 lines

correctly, your code focus on signation and encrypt the hidden data, but not target for the whole snapshot data.

Please ignore the hidden area because we already encrypted snapshot image. Those data doesn't need to erase from snapshot anymore. I will remove the hidden area patches in next version.

Quoted 12 lines

Quoted 12 lines

The above patches still need to clean up. I am working on some other bugs, but I can clean up and send out it ASAP.

The advantage of this solution is that it produces a signed and encrypted image. Not just for writing to block device by kernel, it also can provide a signed/encrypted image to user space. User space can store the encrypted image to anywhere.

I am OK for your user space key generator because I didn't have similar solution yet. I am working on the EFI master key and also want to adapt hibernation to keyring. I will continue the works.

Quoted 3 lines

The user space tool can easily add the keyring support besides ioctl if needed.

Understood.

Thanks
Joey Lee

Quoted 448 lines

Best,
Yu

Quoted 446 lines

Thanks a lot!
Joey Lee

Quoted 443 lines

Suggested-by Rafael J. Wysocki <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "Lee, Chun-Yi" <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/power.h |   1 +
 kernel/power/swap.c  | 215 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 205 insertions(+), 11 deletions(-)

kernel/power/power.h +1 -0

index 660aac3..637695c 100644
@@ -207,6 +207,7 @@ extern int swsusp_swap_in_use(void);
 #define SF_PLATFORM_MODE	1
 #define SF_NOCOMPRESS_MODE	2
 #define SF_CRC32_MODE	        4
+#define SF_ENCRYPT_MODE		8
 
 /* kernel/power/hibernate.c */
 extern int swsusp_check(void);

kernel/power/swap.c +204 -11

index c2bcf97..2b6b3d0 100644
@@ -102,14 +102,16 @@ struct swap_map_handle {
 	unsigned int k;
 	unsigned long reqd_free_pages;
 	u32 crc32;
+	bool crypto;
 };
 
 struct swsusp_header {
 	char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) -
-	              sizeof(u32)];
+			sizeof(u32) - HIBERNATE_SALT_BYTES];
 	u32	crc32;
 	sector_t image;
 	unsigned int flags;	/* Flags to pass to the "boot" kernel */
+	char salt[HIBERNATE_SALT_BYTES];
 	char	orig_sig[10];
 	char	sig[10];
 } __packed;

@@ -127,6 +129,53 @@ struct swsusp_extent {
 	unsigned long end;
 };
 
+/* For encryption/decryption. */
+static struct hibernation_crypto *hibernation_crypto_ops;
+
+void set_hibernation_ops(struct hibernation_crypto *ops)
+{
+	hibernation_crypto_ops = ops;
+}
+EXPORT_SYMBOL_GPL(set_hibernation_ops);
+
+static int crypto_data(const char *inbuf,
+			    int inlen,
+			    char *outbuf,
+			    int outlen,
+			    bool encrypt,
+			    int page_idx)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->crypto_data)
+		return hibernation_crypto_ops->crypto_data(inbuf,
+			inlen, outbuf, outlen, encrypt, page_idx);
+	else
+		return -EINVAL;
+}
+
+static void crypto_save(void *outbuf)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->save)
+		hibernation_crypto_ops->save(outbuf);
+}
+
+static void crypto_restore(void *inbuf)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->restore)
+		hibernation_crypto_ops->restore(inbuf);
+}
+
+static int crypto_init(bool suspend)
+{
+	if (hibernation_crypto_ops &&
+	    hibernation_crypto_ops->init)
+		return hibernation_crypto_ops->init(suspend);
+	else
+		return -EINVAL;
+}
+
 static struct rb_root swsusp_extents = RB_ROOT;
 
 static int swsusp_extents_insert(unsigned long swap_offset)

@@ -318,6 +367,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 		swsusp_header->flags = flags;
 		if (flags & SF_CRC32_MODE)
 			swsusp_header->crc32 = handle->crc32;
+		if (handle->crypto) {
+			swsusp_header->flags |= SF_ENCRYPT_MODE;
+			crypto_save((void *)swsusp_header->salt);
+		}
 		error = hib_submit_io(REQ_OP_WRITE, REQ_SYNC,
 				      swsusp_resume_block, swsusp_header, NULL);
 	} else {

@@ -535,11 +588,12 @@ static int save_image(struct swap_map_handle *handle,
 {
 	unsigned int m;
 	int ret;
-	int nr_pages;
+	int nr_pages, crypto_page_idx;
 	int err2;
 	struct hib_bio_batch hb;
 	ktime_t start;
 	ktime_t stop;
+	void *tmp = NULL, *crypt_buf = NULL;
 
 	hib_init_batch(&hb);
 

@@ -549,12 +603,33 @@ static int save_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
+
 	start = ktime_get();
 	while (1) {
 		ret = snapshot_read_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_write_page(handle, data_of(*snapshot), &hb);
+		tmp = data_of(*snapshot);
+		if (handle->crypto) {
+			/* Encryption before submit_io.*/
+			ret = crypto_data(data_of(*snapshot),
+					  PAGE_SIZE,
+					  crypt_buf,
+					  PAGE_SIZE,
+					  true,
+					  crypto_page_idx);
+			if (ret)
+				goto out;
+			crypto_page_idx++;
+			tmp = crypt_buf;
+		}
+		ret = swap_write_page(handle, tmp, &hb);
 		if (ret)
 			break;
 		if (!(nr_pages % m))

@@ -569,6 +644,9 @@ static int save_image(struct swap_map_handle *handle,
 	if (!ret)
 		pr_info("Image saving done\n");
 	swsusp_show_speed(start, stop, nr_to_write, "Wrote");
+ out:
+	if (crypt_buf)
+		free_page((unsigned long)crypt_buf);
 	return ret;
 }
 

@@ -671,7 +749,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 {
 	unsigned int m;
 	int ret = 0;
-	int nr_pages;
+	int nr_pages, crypto_page_idx;
 	int err2;
 	struct hib_bio_batch hb;
 	ktime_t start;

@@ -767,6 +845,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
 	start = ktime_get();
 	for (;;) {
 		for (thr = 0; thr < nr_threads; thr++) {

@@ -835,7 +914,25 @@ static int save_image_lzo(struct swap_map_handle *handle,
 			for (off = 0;
 			     off < LZO_HEADER + data[thr].cmp_len;
 			     off += PAGE_SIZE) {
-				memcpy(page, data[thr].cmp + off, PAGE_SIZE);
+				if (handle->crypto) {
+					/*
+					 * Encrypt the compressed data
+					 * before we write them to the
+					 * block device.
+					 */
+					ret = crypto_data(data[thr].cmp + off,
+							  PAGE_SIZE,
+							  page,
+							  PAGE_SIZE,
+							  true,
+							  crypto_page_idx);
+					if (ret)
+						goto out_finish;
+					crypto_page_idx++;
+				} else {
+					memcpy(page, data[thr].cmp + off,
+						PAGE_SIZE);
+				}
 
 				ret = swap_write_page(handle, page, &hb);
 				if (ret)

@@ -909,6 +1006,7 @@ int swsusp_write(unsigned int flags)
 	int error;
 
 	pages = snapshot_get_image_size();
+	memset(&handle, 0, sizeof(struct swap_map_handle));
 	error = get_swap_writer(&handle);
 	if (error) {
 		pr_err("Cannot get swap writer\n");

@@ -922,6 +1020,9 @@ int swsusp_write(unsigned int flags)
 		}
 	}
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	if (!crypto_init(true))
+		/* The image needs to be encrypted. */
+		handle.crypto = true;
 	error = snapshot_read_next(&snapshot);
 	if (error < PAGE_SIZE) {
 		if (error >= 0)

@@ -1059,7 +1160,8 @@ static int load_image(struct swap_map_handle *handle,
 	ktime_t stop;
 	struct hib_bio_batch hb;
 	int err2;
-	unsigned nr_pages;
+	unsigned nr_pages, crypto_page_idx;
+	void *crypt_buf = NULL;
 
 	hib_init_batch(&hb);
 

@@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
 	start = ktime_get();
 	for ( ; ; ) {
 		ret = snapshot_write_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_read_page(handle, data_of(*snapshot), &hb);
+		if (handle->crypto)
+			ret = swap_read_page(handle, crypt_buf, &hb);
+		else
+			ret = swap_read_page(handle, data_of(*snapshot), &hb);
 		if (ret)
 			break;
 		if (snapshot->sync_read)
 			ret = hib_wait_io(&hb);
 		if (ret)
 			break;
+		if (handle->crypto) {
+			/*
+			 * Need a decryption for the
+			 * data read from the block
+			 * device.
+			 */
+			ret = crypto_data(crypt_buf, PAGE_SIZE,
+					  data_of(*snapshot),
+					  PAGE_SIZE,
+					  false,
+					  crypto_page_idx);
+			if (ret)
+				break;
+			crypto_page_idx++;
+		}
 		if (!(nr_pages % m))
 			pr_info("Image loading progress: %3d%%\n",
 				nr_pages / m * 10);

@@ -1097,6 +1223,8 @@ static int load_image(struct swap_map_handle *handle,
 			ret = -ENODATA;
 	}
 	swsusp_show_speed(start, stop, nr_to_read, "Read");
+	if (crypt_buf)
+		free_page((unsigned long)crypt_buf);
 	return ret;
 }
 

@@ -1164,7 +1292,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	struct hib_bio_batch hb;
 	ktime_t start;
 	ktime_t stop;
-	unsigned nr_pages;
+	unsigned nr_pages, crypto_page_idx;
 	size_t off;
 	unsigned i, thr, run_threads, nr_threads;
 	unsigned ring = 0, pg = 0, ring_size = 0,

@@ -1173,6 +1301,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	unsigned char **page = NULL;
 	struct dec_data *data = NULL;
 	struct crc_data *crc = NULL;
+	void *first_page = NULL;
 
 	hib_init_batch(&hb);
 

@@ -1278,6 +1407,18 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	}
 	want = ring_size = i;
 
+	/*
+	 * The first page of data[thr] contains the length of
+	 * compressed data, this page should not mess up the
+	 * read buffer, so we allocate a separate page for it.
+	 */
+	if (handle->crypto) {
+		first_page = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!first_page) {
+			ret = -ENOMEM;
+			goto out_clean;
+		}
+	}
 	pr_info("Using %u thread(s) for decompression\n", nr_threads);
 	pr_info("Loading and decompressing image data (%u pages)...\n",
 		nr_to_read);

@@ -1285,6 +1426,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
 	start = ktime_get();
 
 	ret = snapshot_write_next(snapshot);

@@ -1336,7 +1478,24 @@ static int load_image_lzo(struct swap_map_handle *handle,
 		}
 
 		for (thr = 0; have && thr < nr_threads; thr++) {
-			data[thr].cmp_len = *(size_t *)page[pg];
+			if (handle->crypto) {
+				/*
+				 * Need to decrypt the first page
+				 * of each data[thr], which contains
+				 * the compressed data length.
+				 */
+				ret = crypto_data(page[pg],
+						  PAGE_SIZE,
+						  first_page,
+						  PAGE_SIZE,
+						  false,
+						  crypto_page_idx);
+				if (ret)
+					goto out_finish;
+				data[thr].cmp_len = *(size_t *)first_page;
+			} else {
+				data[thr].cmp_len = *(size_t *)page[pg];
+			}
 			if (unlikely(!data[thr].cmp_len ||
 			             data[thr].cmp_len >
 			             lzo1x_worst_compress(LZO_UNC_SIZE))) {

@@ -1358,8 +1517,26 @@ static int load_image_lzo(struct swap_map_handle *handle,
 			for (off = 0;
 			     off < LZO_HEADER + data[thr].cmp_len;
 			     off += PAGE_SIZE) {
-				memcpy(data[thr].cmp + off,
-				       page[pg], PAGE_SIZE);
+				if (handle->crypto) {
+					/*
+					 * Decrypt the compressed data
+					 * and leverage the decompression
+					 * threads to get it done.
+					 */
+					ret = crypto_data(page[pg],
+							  PAGE_SIZE,
+							  data[thr].cmp + off,
+							  PAGE_SIZE,
+							  false,
+							  crypto_page_idx);
+					if (ret)
+						goto out_finish;
+					crypto_page_idx++;
+				} else {
+					memcpy(data[thr].cmp + off,
+						page[pg], PAGE_SIZE);
+
+				}
 				have--;
 				want++;
 				if (++pg >= ring_size)

@@ -1452,6 +1629,8 @@ static int load_image_lzo(struct swap_map_handle *handle,
 out_clean:
 	for (i = 0; i < ring_size; i++)
 		free_page((unsigned long)page[i]);
+	if (first_page)
+		free_page((unsigned long)first_page);
 	if (crc) {
 		if (crc->thr)
 			kthread_stop(crc->thr);

@@ -1482,6 +1661,7 @@ int swsusp_read(unsigned int *flags_p)
 	struct swsusp_info *header;
 
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	memset(&handle, 0, sizeof(struct swap_map_handle));
 	error = snapshot_write_next(&snapshot);
 	if (error < PAGE_SIZE)
 		return error < 0 ? error : -EFAULT;

@@ -1489,6 +1669,16 @@ int swsusp_read(unsigned int *flags_p)
 	error = get_swap_reader(&handle, flags_p);
 	if (error)
 		goto end;
+	if (*flags_p & SF_ENCRYPT_MODE) {
+		error = crypto_init(false);
+		if (!error) {
+			/* The image has been encrypted. */
+			handle.crypto = true;
+		} else {
+			pr_err("Failed to init cipher during resume.\n");
+			goto end;
+		}
+	}
 	if (!error)
 		error = swap_read_page(&handle, header, NULL);
 	if (!error) {

@@ -1526,6 +1716,9 @@ int swsusp_check(void)
 
 		if (!memcmp(HIBERNATE_SIG, swsusp_header->sig, 10)) {
 			memcpy(swsusp_header->sig, swsusp_header->orig_sig, 10);
+			/* Read salt passed from previous kernel. */
+			if (swsusp_header->flags & SF_ENCRYPT_MODE)
+				crypto_restore((void *)&swsusp_header->salt);
 			/* Reset swap signature now */
 			error = hib_submit_io(REQ_OP_WRITE, REQ_SYNC,
 						swsusp_resume_block,

Signature · 1 line

2.7.4

Hi,

Quoted 24 lines · joeyli

Quoted 12 lines · Yu Chen

Hi,

Quoted 10 lines · joeyli

Hi Chen Yu,

Quoted 6 lines · Chen Yu

Use the helper functions introduced previously to encrypt the page data before they are submitted to the block device. Besides, for the case of hibernation compression, the data are firstly compressed and then encrypted, and vice versa for the resume process.

Quoted 7 lines

Quoted 6 lines

I want to suggest my solution that it direct signs/encrypts the memory snapshot image. This solution is already shipped with SLE12 a couple of years:

https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3

I did not see image page encryption in above link, if I understand

PM / hibernate: Generate and verify signature for snapshot image
https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f

Quoted 6 lines

PM / hibernate: snapshot image encryption
https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929

The above patches sign and encrypt the data pages in snapshot image. It puts the signature to header.

It looks like your signature code can be simplyly added on top of the solution we proposed here, how about we collaborating on this task?

just my 2 cents,
1. The cryption code should be indepent of the snapshot code, and this is why we implement it as a kernel module for that in PATCH[1/3].

2. There's no need to traverse the snapshot image twice, if the image is large(there's requirement on servers now) we can simplyly do the encryption before the disk IO, and this is why PATCH[2/3] looks like this.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Suggested-by Rafael J. Wysocki <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "Lee, Chun-Yi" <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/power.h |   1 +
 kernel/power/swap.c  | 215 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 205 insertions(+), 11 deletions(-)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

/* kernel/power/hibernate.c */
extern int swsusp_check(void);

kernel/power/swap.c +0 -0

index c2bcf97..2b6b3d0 100644

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

kernel/power/swap.c +6 -1

@@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
 	start = ktime_get();
 	for ( ; ; ) {
 		ret = snapshot_write_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_read_page(handle, data_of(*snapshot), &hb);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1192

Quoted 8 lines

kernel/power/swap.c +4 -0

+		if (handle->crypto)
+			ret = swap_read_page(handle, crypt_buf, &hb);
+		else
+			ret = swap_read_page(handle, data_of(*snapshot), &hb);
 		if (ret)
 			break;
 		if (snapshot->sync_read)
 			ret = hib_wait_io(&hb);

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

kernel/power/swap.c +7 -0

 		if (ret)
 			break;
+		if (handle->crypto) {
+			/*
+			 * Need a decryption for the
+			 * data read from the block
+			 * device.
+			 */
+			ret = crypto_data(crypt_buf, PAGE_SIZE,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1209

Quoted 8 lines

kernel/power/swap.c +8 -0

+					  data_of(*snapshot),
+					  PAGE_SIZE,
+					  false,
+					  crypto_page_idx);
+			if (ret)
+				break;
+			crypto_page_idx++;
+		}

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1212

Quoted 3 lines

kernel/power/swap.c +0 -0

 		if (!(nr_pages % m))
 			pr_info("Image loading progress: %3d%%\n",
 				nr_pages / m * 10);

Quoted 26 lines · Yu Chen

Hi,

Quoted 24 lines · joeyli

Quoted 12 lines · Yu Chen

Hi,

Quoted 10 lines · joeyli

Hi Chen Yu,

Quoted 6 lines · Chen Yu

Use the helper functions introduced previously to encrypt the page data before they are submitted to the block device. Besides, for the case of hibernation compression, the data are firstly compressed and then encrypted, and vice versa for the resume process.

Quoted 7 lines

Quoted 6 lines

I want to suggest my solution that it direct signs/encrypts the memory snapshot image. This solution is already shipped with SLE12 a couple of years:

https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3

I did not see image page encryption in above link, if I understand

PM / hibernate: Generate and verify signature for snapshot image
https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f

Quoted 8 lines

Quoted 6 lines

PM / hibernate: snapshot image encryption
https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929

The above patches sign and encrypt the data pages in snapshot image. It puts the signature to header.

It looks like your signature code can be simplyly added on top of the solution we proposed here, how about we collaborating on this task?

OK, I will base on your user key solution to respin my signature patches.

Quoted 3 lines

just my 2 cents,
1. The cryption code should be indepent of the snapshot code, and this is why we implement it as a kernel module for that in PATCH[1/3].

Why the cryption code must be indepent of snapshot code?

Quoted 4 lines

2. There's no need to traverse the snapshot image twice, if the image is large(there's requirement on servers now) we can simplyly do the encryption before the disk IO, and this is why PATCH[2/3] looks like this.

If the encryption solution is only for block device, then the uswsusp interface must be locked-down when kernel is in locked mode:

uswsusp: Disable when the kernel is locked down
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=lockdown-20180410&id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612

I still suggest to keep the solution to direct encript the snapshot image for uswsusp because the snapshot can be encrypted by kernel before user space get it.

I mean that if the uswsusp be used, then kernel direct encrypts the snapshot image, otherwise kernel encrypts pages before block io.

On the other hand, I have a question about asynchronous block io. Please see below...

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Suggested-by Rafael J. Wysocki <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "Lee, Chun-Yi" <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/power.h |   1 +
 kernel/power/swap.c  | 215 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 205 insertions(+), 11 deletions(-)

[...snip]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

/* kernel/power/hibernate.c */
extern int swsusp_check(void);

kernel/power/swap.c +0 -0

index c2bcf97..2b6b3d0 100644

[...snip]

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

kernel/power/swap.c +10 -1

@@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
 	start = ktime_get();
 	for ( ; ; ) {
 		ret = snapshot_write_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_read_page(handle, data_of(*snapshot), &hb);
+		if (handle->crypto)
+			ret = swap_read_page(handle, crypt_buf, &hb);
+		else
+			ret = swap_read_page(handle, data_of(*snapshot), &hb);
 		if (ret)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1192

Quoted 3 lines

kernel/power/swap.c +0 -0

 			break;
 		if (snapshot->sync_read)
 			ret = hib_wait_io(&hb);

In snapshot_write_next(), the logic will clean the snapshot->sync_read when the buffer page doesn't equal to the original page. Which means that the page can be read by asynchronous block io. Otherwise, kernel calls hib_wait_io() to wait until the block io was done.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

kernel/power/swap.c +11 -0

 		if (ret)
 			break;
+		if (handle->crypto) {
+			/*
+			 * Need a decryption for the
+			 * data read from the block
+			 * device.
+			 */
+			ret = crypto_data(crypt_buf, PAGE_SIZE,
+					  data_of(*snapshot),
+					  PAGE_SIZE,
+					  false,
+					  crypto_page_idx);

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1209

Quoted 4 lines

kernel/power/swap.c +4 -0

+			if (ret)
+				break;
+			crypto_page_idx++;
+		}

The decryption is here in the for-loop. But maybe the page is still in the block io queue for waiting the batch read? The page content is not really read to memory when the crypto_data be run?

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1212

Quoted 3 lines

kernel/power/swap.c +0 -0

 		if (!(nr_pages % m))
 			pr_info("Image loading progress: %3d%%\n",
 				nr_pages / m * 10);

                nr_pages++;
        }
        err2 = hib_wait_io(&hb);
        stop = ktime_get();

When the for-loop is break, the above hib_wait_io(&hb) guarantees that all asynchronous block io are done. Then all pages are read to memory.

I think that the decryption code must be moved after for-loop be break. Or there have any callback hook in the asynchronous block io that we can put the encryption code after the block io read the page.

Thanks a lot!
Joey Lee

Hi Joey Lee,

Quoted 38 lines · joeyli

Quoted 26 lines · Yu Chen

Hi,

Quoted 24 lines · joeyli

Quoted 12 lines · Yu Chen

Hi,

Quoted 10 lines · joeyli

Hi Chen Yu,

Quoted 6 lines · Chen Yu

Use the helper functions introduced previously to encrypt the page data before they are submitted to the block device. Besides, for the case of hibernation compression, the data are firstly compressed and then encrypted, and vice versa for the resume process.

Quoted 7 lines

Quoted 6 lines

I want to suggest my solution that it direct signs/encrypts the memory snapshot image. This solution is already shipped with SLE12 a couple of years:

https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3

I did not see image page encryption in above link, if I understand

PM / hibernate: Generate and verify signature for snapshot image
https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f

Quoted 8 lines

Quoted 6 lines

PM / hibernate: snapshot image encryption
https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929

The above patches sign and encrypt the data pages in snapshot image. It puts the signature to header.

It looks like your signature code can be simplyly added on top of the solution we proposed here, how about we collaborating on this task?

OK, I will base on your user key solution to respin my signature patches.

Quoted 6 lines

Quoted 3 lines

just my 2 cents,
1. The cryption code should be indepent of the snapshot code, and this is why we implement it as a kernel module for that in PATCH[1/3].

Why the cryption code must be indepent of snapshot code?

Modules can be easier to be maintained and customized/improved in the future IMO..

Quoted 15 lines

Quoted 4 lines

2. There's no need to traverse the snapshot image twice, if the image is large(there's requirement on servers now) we can simplyly do the encryption before the disk IO, and this is why PATCH[2/3] looks like this.

If the encryption solution is only for block device, then the uswsusp interface must be locked-down when kernel is in locked mode:

uswsusp: Disable when the kernel is locked down
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=lockdown-20180410&id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612

I still suggest to keep the solution to direct encript the snapshot image for uswsusp because the snapshot can be encrypted by kernel before user space get it.

Quoted 3 lines

I mean that if the uswsusp be used, then kernel direct encrypts the snapshot image, otherwise kernel encrypts pages before block io.

I did not quite get the point, if the kernel has been locked down, then the uswsusp is disabled, why the kernel encrypts the snapshot for uswsusp?

Quoted 3 lines

On the other hand, I have a question about asynchronous block io. Please see below...

Okay.

Quoted 42 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Suggested-by Rafael J. Wysocki <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "Lee, Chun-Yi" <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/power.h |   1 +
 kernel/power/swap.c  | 215 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 205 insertions(+), 11 deletions(-)

[...snip]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

/* kernel/power/hibernate.c */
extern int swsusp_check(void);

kernel/power/swap.c +0 -0

index c2bcf97..2b6b3d0 100644

[...snip]

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

kernel/power/swap.c +10 -1

@@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
 	start = ktime_get();
 	for ( ; ; ) {
 		ret = snapshot_write_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_read_page(handle, data_of(*snapshot), &hb);
+		if (handle->crypto)
+			ret = swap_read_page(handle, crypt_buf, &hb);
+		else
+			ret = swap_read_page(handle, data_of(*snapshot), &hb);
 		if (ret)

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1192

Quoted 3 lines

kernel/power/swap.c +0 -0

 			break;
 		if (snapshot->sync_read)
 			ret = hib_wait_io(&hb);

In snapshot_write_next(), the logic will clean the snapshot->sync_read when the buffer page doesn't equal to the original page. Which means that the page can be read by asynchronous block io. Otherwise, kernel calls hib_wait_io() to wait until the block io was done.

Yes, you are right, I missed the asynchronous block io in non-compression case.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

kernel/power/swap.c +11 -0

 		if (ret)
 			break;
+		if (handle->crypto) {
+			/*
+			 * Need a decryption for the
+			 * data read from the block
+			 * device.
+			 */
+			ret = crypto_data(crypt_buf, PAGE_SIZE,
+					  data_of(*snapshot),
+					  PAGE_SIZE,
+					  false,
+					  crypto_page_idx);

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1209

Quoted 4 lines

kernel/power/swap.c +4 -0

+			if (ret)
+				break;
+			crypto_page_idx++;
+		}

The decryption is here in the for-loop. But maybe the page is still in the block io queue for waiting the batch read? The page content is not really read to memory when the crypto_data be run?

Yes, it is possible.

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1212

Quoted 3 lines

kernel/power/swap.c +0 -0

 		if (!(nr_pages % m))
 			pr_info("Image loading progress: %3d%%\n",
 				nr_pages / m * 10);

                nr_pages++;
        }
        err2 = hib_wait_io(&hb);
        stop = ktime_get();

Quoted 7 lines

When the for-loop is break, the above hib_wait_io(&hb) guarantees that all asynchronous block io are done. Then all pages are read to memory.

I think that the decryption code must be moved after for-loop be break. Or there have any callback hook in the asynchronous block io that we can put the encryption code after the block io read the page.

Yes, we can move the decryptino code here. Another thought came to my mind, how about disabling the asynchronous block io in non-compression case if encryption hibernation is enabled(because encryption hibernation is not using the asynchronous block io and it has to traverse each page one by one, so asynchronous block io does not bring benefit to encryption hibernation in non-compression case)? Or do I miss something?

Hi Yu Chen,

Sorry for my delay...

Quoted 46 lines · Yu Chen

Hi Joey Lee,

Quoted 44 lines · joeyli

Quoted 26 lines · Yu Chen

Hi,

Quoted 24 lines · joeyli

Quoted 12 lines · Yu Chen

Hi,

Quoted 10 lines · joeyli

Hi Chen Yu,

Quoted 6 lines · Chen Yu

Use the helper functions introduced previously to encrypt the page data before they are submitted to the block device. Besides, for the case of hibernation compression, the data are firstly compressed and then encrypted, and vice versa for the resume process.

Quoted 7 lines

Quoted 6 lines

I want to suggest my solution that it direct signs/encrypts the memory snapshot image. This solution is already shipped with SLE12 a couple of years:

https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3

I did not see image page encryption in above link, if I understand

PM / hibernate: Generate and verify signature for snapshot image
https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f

Quoted 8 lines

Quoted 6 lines

PM / hibernate: snapshot image encryption
https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929

The above patches sign and encrypt the data pages in snapshot image. It puts the signature to header.

It looks like your signature code can be simplyly added on top of the solution we proposed here, how about we collaborating on this task?

OK, I will base on your user key solution to respin my signature patches.

Quoted 3 lines

just my 2 cents,
1. The cryption code should be indepent of the snapshot code, and this is why we implement it as a kernel module for that in PATCH[1/3].

Why the cryption code must be indepent of snapshot code?

Quoted 1 line

Modules can be easier to be maintained and customized/improved in the future IMO..

hm... currently I didn't see apparent benefit on this...

About modularization, is it possible to separate the key handler code from crypto_hibernation.c? Otherwise the snapshot signature needs to depend on encrypt function.

Quoted 15 lines

Quoted 15 lines

Quoted 4 lines

2. There's no need to traverse the snapshot image twice, if the image is large(there's requirement on servers now) we can simplyly do the encryption before the disk IO, and this is why PATCH[2/3] looks like this.

If the encryption solution is only for block device, then the uswsusp interface must be locked-down when kernel is in locked mode:

uswsusp: Disable when the kernel is locked down
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=lockdown-20180410&id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612

I still suggest to keep the solution to direct encript the snapshot image for uswsusp because the snapshot can be encrypted by kernel before user space get it.

Quoted 6 lines

Quoted 3 lines

I mean that if the uswsusp be used, then kernel direct encrypts the snapshot image, otherwise kernel encrypts pages before block io.

I did not quite get the point, if the kernel has been locked down, then the uswsusp is disabled, why the kernel encrypts the snapshot for uswsusp?

There have some functions be locked-down because there have no appropriate mechanisms to check the integrity of writing data. If the snapshot image can be encrypted and authentication, then the uswsusp interface is avaiable for userspace. We do not need to lock it down.

Quoted 96 lines

Quoted 4 lines

On the other hand, I have a question about asynchronous block io. Please see below...

Okay.

Quoted 42 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Suggested-by Rafael J. Wysocki <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "Lee, Chun-Yi" <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/power.h |   1 +
 kernel/power/swap.c  | 215 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 205 insertions(+), 11 deletions(-)

[...snip]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

/* kernel/power/hibernate.c */
extern int swsusp_check(void);

kernel/power/swap.c +0 -0

index c2bcf97..2b6b3d0 100644

[...snip]

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

kernel/power/swap.c +10 -1

@@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle *handle,
 	if (!m)
 		m = 1;
 	nr_pages = 0;
+	crypto_page_idx = 0;
+	if (handle->crypto) {
+		crypt_buf = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!crypt_buf)
+			return -ENOMEM;
+	}
 	start = ktime_get();
 	for ( ; ; ) {
 		ret = snapshot_write_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_read_page(handle, data_of(*snapshot), &hb);
+		if (handle->crypto)
+			ret = swap_read_page(handle, crypt_buf, &hb);
+		else
+			ret = swap_read_page(handle, data_of(*snapshot), &hb);
 		if (ret)

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1192

Quoted 3 lines

kernel/power/swap.c +0 -0

 			break;
 		if (snapshot->sync_read)
 			ret = hib_wait_io(&hb);

In snapshot_write_next(), the logic will clean the snapshot->sync_read when the buffer page doesn't equal to the original page. Which means that the page can be read by asynchronous block io. Otherwise, kernel calls hib_wait_io() to wait until the block io was done.

Yes, you are right, I missed the asynchronous block io in non-compression case.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

kernel/power/swap.c +11 -0

 		if (ret)
 			break;
+		if (handle->crypto) {
+			/*
+			 * Need a decryption for the
+			 * data read from the block
+			 * device.
+			 */
+			ret = crypto_data(crypt_buf, PAGE_SIZE,
+					  data_of(*snapshot),
+					  PAGE_SIZE,
+					  false,
+					  crypto_page_idx);

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1209

Quoted 4 lines

kernel/power/swap.c +4 -0

+			if (ret)
+				break;
+			crypto_page_idx++;
+		}

The decryption is here in the for-loop. But maybe the page is still in the block io queue for waiting the batch read? The page content is not really read to memory when the crypto_data be run?

Yes, it is possible.

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

kernel/power/swap.c static int load_image(struct swap_map_handle *handle, · line 1212

Quoted 3 lines

kernel/power/swap.c +0 -0

 		if (!(nr_pages % m))
 			pr_info("Image loading progress: %3d%%\n",
 				nr_pages / m * 10);

                nr_pages++;
        }
        err2 = hib_wait_io(&hb);
        stop = ktime_get();

Quoted 7 lines

When the for-loop is break, the above hib_wait_io(&hb) guarantees that all asynchronous block io are done. Then all pages are read to memory.

I think that the decryption code must be moved after for-loop be break. Or there have any callback hook in the asynchronous block io that we can put the encryption code after the block io read the page.

Quoted 7 lines

Yes, we can move the decryptino code here. Another thought came to my mind, how about disabling the asynchronous block io in non-compression case if encryption hibernation is enabled(because encryption hibernation is not using the asynchronous block io and it has to traverse each page one by one, so asynchronous block io does not bring benefit to encryption hibernation in non-compression case)? Or do I miss something?

Disabling asynchronous block will cause IO performance drops. As I remeber that it's a lot of drops. You can try it. I am not sure how many people are still using non-compression mode.

I do not know too much on block layer API. Is it possible to encrypt each page before writing to device from kernel?

Thanks
Joey Lee

Hi,

Quoted 5 lines · joeyli

Hi Yu Chen,

Sorry for my delay...

On Fri, Jul 06, 2018 at 11:28:56PM +0800, Yu Chen wrote:

Quoted 10 lines

Quoted 3 lines

Quoted 2 lines

Why the cryption code must be indepent of snapshot code?

Modules can be easier to be maintained and customized/improved in the future IMO..

hm... currently I didn't see apparent benefit on this...

About modularization, is it possible to separate the key handler code from crypto_hibernation.c? Otherwise the snapshot signature needs to depend on encrypt function.

I understand.
It seems that we can reuse crypto_data() for the signature logic.
For example, add one parameter for crypto_data(..., crypto_mode)
the crypto_mode is a combination of ENCRYPT/DECRYPT/SIGNATURE_START/SIGNATURE_END, and can be controled by sysfs. If SIGNATURE_START is enabled, the crypto_data() invokes crypto_shash_update() to get "hmac(sha512)" hash, and if SIGNATURE_END is enabled, crypto_shash_final() stores the final result in global buffer

struct hibernation_crypto.keys.digest[SNAPSHOT_DIGEST_SIZE] which can be passed to the restore kernel, the same as the salt. Besides,

the swsusp_prepare_hash() in your code could be moved into init_crypto_helper(), thus the signature key could also reuse the same key passed from user space or via get_efi_secret_key(). In this way, the change in snapshot.c is minimal and the crypto facilities could be maintained in one file.

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

Quoted 4 lines

2. There's no need to traverse the snapshot image twice, if the image is large(there's requirement on servers now) we can simplyly do the encryption before the disk IO, and this is why PATCH[2/3] looks like this.

If the encryption solution is only for block device, then the uswsusp interface must be locked-down when kernel is in locked mode:

uswsusp: Disable when the kernel is locked down
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=lockdown-20180410&id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612

I still suggest to keep the solution to direct encript the snapshot image for uswsusp because the snapshot can be encrypted by kernel before user space get it.

Quoted 6 lines

Quoted 3 lines

I mean that if the uswsusp be used, then kernel direct encrypts the snapshot image, otherwise kernel encrypts pages before block io.

I did not quite get the point, if the kernel has been locked down, then the uswsusp is disabled, why the kernel encrypts the snapshot for uswsusp?

Quoted 5 lines

There have some functions be locked-down because there have no appropriate mechanisms to check the integrity of writing data. If the snapshot image can be encrypted and authentication, then the uswsusp interface is avaiable for userspace. We do not need to lock it down.

Ok, I got your point. To be honest, I like your implementation to treat the snapshot data seperately except that it might cause small overhead when traversing large number of snapshot and make snapshot logic a little coupling with crypto logic. Let me send our v2 using the crypto-before-blockio for review and maybe change to your solution using the encapsulated APIs in crypto_hibernate.c.

Quoted 7 lines · Yu Chen

Hi,

Quoted 5 lines · joeyli

Hi Yu Chen,

Sorry for my delay...

On Fri, Jul 06, 2018 at 11:28:56PM +0800, Yu Chen wrote:

[...snip]

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

Quoted 2 lines

Why the cryption code must be indepent of snapshot code?

Modules can be easier to be maintained and customized/improved in the future IMO..

hm... currently I didn't see apparent benefit on this...

About modularization, is it possible to separate the key handler code from crypto_hibernation.c? Otherwise the snapshot signature needs to depend on encrypt function.

Quoted 7 lines

I understand.
It seems that we can reuse crypto_data() for the signature logic.
For example, add one parameter for crypto_data(..., crypto_mode)
the crypto_mode is a combination of ENCRYPT/DECRYPT/SIGNATURE_START/SIGNATURE_END, and can be controled by sysfs. If SIGNATURE_START is enabled, the crypto_data() invokes crypto_shash_update() to get "hmac(sha512)" hash, and if SIGNATURE_END is enabled, crypto_shash_final() stores the final result in global buffer

I agree that the swsusp_prepare and hmac-hash codes can be extract to crypto_hibernation.

Quoted 2 lines

struct hibernation_crypto.keys.digest[SNAPSHOT_DIGEST_SIZE] which can be passed to the restore kernel, the same as the salt. Besides,

The salt is stored in the swsusp_header and put in swap. What I want is that the signature of snapshot should be keep in the snapshot header as my original design in patch. The benefit is that the snapshot with signature can be stored to any place but not just swap. The user space can take snapshot image with signature to anywhere.

Quoted 5 lines

the swsusp_prepare_hash() in your code could be moved into init_crypto_helper(), thus the signature key could also reuse the same key passed from user space or via get_efi_secret_key(). In this way, the change in snapshot.c is minimal and the crypto facilities could be maintained in one file.

I agree. But as I mentioned in that mail, the key handler codes in hibernation_crypto() should be extract to another C file to avoid the logic is mixed with the crypto code. The benefit is that we can add more key sources like encrypted key or EFI key in the future.

Quoted 27 lines

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

Quoted 4 lines

2. There's no need to traverse the snapshot image twice, if the image is large(there's requirement on servers now) we can simplyly do the encryption before the disk IO, and this is why PATCH[2/3] looks like this.

If the encryption solution is only for block device, then the uswsusp interface must be locked-down when kernel is in locked mode:

uswsusp: Disable when the kernel is locked down
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=lockdown-20180410&id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612

I still suggest to keep the solution to direct encript the snapshot image for uswsusp because the snapshot can be encrypted by kernel before user space get it.

Quoted 6 lines

Quoted 3 lines

I mean that if the uswsusp be used, then kernel direct encrypts the snapshot image, otherwise kernel encrypts pages before block io.

I did not quite get the point, if the kernel has been locked down, then the uswsusp is disabled, why the kernel encrypts the snapshot for uswsusp?

Quoted 5 lines

There have some functions be locked-down because there have no appropriate mechanisms to check the integrity of writing data. If the snapshot image can be encrypted and authentication, then the uswsusp interface is avaiable for userspace. We do not need to lock it down.

Quoted 6 lines

Ok, I got your point. To be honest, I like your implementation to treat the snapshot data seperately except that it might cause small overhead when traversing large number of snapshot and make snapshot logic a little coupling with crypto logic. Let me send our v2 using the crypto-before-blockio for review and maybe change to your solution using the encapsulated APIs in crypto_hibernate.c.

Because sometimes I stick on other topics...

If you are urgent for pushing your encryption solution. Please do not consider too much on signature check. Just complete your patches and push to kernel mainline. I will follow your result to respin my signature work.

Thanks
Joey Lee

crypto_hibernate is a user-space utility to generate 512bits AES key and pass it to the kernel via ioctl for hibernation encryption.(We can also add the key into kernel via keyctl if necessary, but currently using ioctl seems to be more straightforward as we need both the key and salt transit).

The key derivation is based on a simplified implementation of PBKDF2[1] in e2fsprogs - both the key length and the hash bytes are the same - 512bits. crypto_hibernate will firstly probe the user for passphrase and get salt from kernel, then uses them to generate a 512bits AES key based on PBKDF2.

tools/power/crypto/crypto_hibernate.c new file +26 -0

+
+#define PBKDF2_ITERATIONS          0xFFFF
+#define SHA512_BLOCKSIZE 128
+#define SHA512_LENGTH 64
+#define SALT_BYTES	16
+#define SYM_KEY_BYTES SHA512_LENGTH
+#define TOTAL_USER_INFO_LEN	(SALT_BYTES+SYM_KEY_BYTES)
+#define MAX_PASSPHRASE_SIZE	1024
+
+struct hibernation_crypto_keys {
+	char derived_key[SYM_KEY_BYTES];
+	char salt[SALT_BYTES];
+	bool valid;
+};
+
+struct hibernation_crypto_keys hib_keys;
+
+static char *get_key_ptr(void)
+{
+	return hib_keys.derived_key;
+}
+
+static char *get_salt_ptr(void)
+{
+	return hib_keys.salt;
+}

tools/power/crypto/crypto_hibernate.c new file +24 -0

+
+static void get_passphrase(char *passphrase, int len)
+{
+	char *p;
+	struct termios current_settings;
+
+	assert(len > 0);
+	disable_echo(&current_settings);
+	p = fgets(passphrase, len, stdin);
+	tcsetattr(0, TCSANOW, &current_settings);
+	printf("\n");
+	if (!p) {
+		printf("Aborting.\n");
+		exit(1);
+	}
+	p = strrchr(passphrase, '\n');
+	if (!p)
+		p = passphrase + len - 1;
+	*p = '\0';
+}
+
+#define CRYPTO_FILE	"/dev/crypto_hibernate"
+
+static int write_keys(void)

tools/power/crypto/crypto_hibernate.c new file +4 -0

+}
+
+static int read_salt(void)
+{

tools/power/crypto/crypto_hibernate.c new file +8 -0

+int main(int argc, char *argv[])
+{
+	int opt, option_index = 0;
+	char in_passphrase[MAX_PASSPHRASE_SIZE];
+
+	while ((opt = getopt_long_only(argc, argv, "+p:s:h",
+				NULL, &option_index)) != -1) {
+		switch (opt) {

tools/power/crypto/crypto_hibernate.c new file +4 -0

+			}
+			break;
+		case 's':
+			{

Hi Chen,

Quoted 12 lines · Chen Yu

crypto_hibernate is a user-space utility to generate 512bits AES key and pass it to the kernel via ioctl for hibernation encryption.(We can also add the key into kernel via keyctl if necessary, but currently using ioctl seems to be more straightforward as we need both the key and salt transit).

The key derivation is based on a simplified implementation of PBKDF2[1] in e2fsprogs - both the key length and the hash bytes are the same - 512bits. crypto_hibernate will firstly probe the user for passphrase and get salt from kernel, then uses them to generate a 512bits AES key based on PBKDF2.

What is a "512bits AES key"? Do you mean AES-256-XTS (which takes a 512-bit key, which the XTS mode internally splits into two keys)?

Do you allow for
other algorithms, or is it hardcoded to AES-256-XTS?

What if someone wants to
use a different algorithm?

BTW, it's difficult to review this with only patch 3/3 Cc'ed to me, as there is no context about the problem you are trying to solve and what your actual proposed kernel changes are. I suggest Cc'ing linux-crypto on all 3 patches.

A few more comments below, from a very brief reading of the code:

[...]

Quoted 26 lines

tools/power/crypto/crypto_hibernate.c new file +26 -0

+
+#define PBKDF2_ITERATIONS          0xFFFF
+#define SHA512_BLOCKSIZE 128
+#define SHA512_LENGTH 64
+#define SALT_BYTES	16
+#define SYM_KEY_BYTES SHA512_LENGTH
+#define TOTAL_USER_INFO_LEN	(SALT_BYTES+SYM_KEY_BYTES)
+#define MAX_PASSPHRASE_SIZE	1024
+
+struct hibernation_crypto_keys {
+	char derived_key[SYM_KEY_BYTES];
+	char salt[SALT_BYTES];
+	bool valid;
+};
+
+struct hibernation_crypto_keys hib_keys;
+
+static char *get_key_ptr(void)
+{
+	return hib_keys.derived_key;
+}
+
+static char *get_salt_ptr(void)
+{
+	return hib_keys.salt;
+}

[...]

tools/power/crypto/crypto_hibernate.c line 305

Quoted 4 lines

tools/power/crypto/crypto_hibernate.c new file +4 -0

+
+
+#define HIBERNATE_SALT_READ      _IOW('C', 3, struct hibernation_crypto_keys)
+#define HIBERNATE_KEY_WRITE     _IOW('C', 4, struct hibernation_crypto_keys)

Why are the ioctl numbers defined based on the size of 'struct hibernation_crypto_keys'? It's not a UAPI structure, right?

Quoted 28 lines

tools/power/crypto/crypto_hibernate.c new file +28 -0

+
+static void get_passphrase(char *passphrase, int len)
+{
+	char *p;
+	struct termios current_settings;
+
+	assert(len > 0);
+	disable_echo(&current_settings);
+	p = fgets(passphrase, len, stdin);
+	tcsetattr(0, TCSANOW, &current_settings);
+	printf("\n");
+	if (!p) {
+		printf("Aborting.\n");
+		exit(1);
+	}
+	p = strrchr(passphrase, '\n');
+	if (!p)
+		p = passphrase + len - 1;
+	*p = '\0';
+}
+
+#define CRYPTO_FILE	"/dev/crypto_hibernate"
+
+static int write_keys(void)
+{
+	int fd;
+
+	fd = open(CRYPTO_FILE, O_RDWR);

tools/power/crypto/crypto_hibernate.c line 354

Quoted 6 lines

tools/power/crypto/crypto_hibernate.c new file +6 -0

+	if (fd < 0) {
+		printf("Cannot open device file...\n");
+		return -EINVAL;
+	}
+	ioctl(fd, HIBERNATE_KEY_WRITE, get_key_ptr());
+	return 0;

No error checking on the ioctl?

Also, how is the kernel supposed to know how long the key is, and which algorithm it's supposed to be used for? I assume it's hardcoded to AES-256-XTS? What if someone wants to use a different algorithm?

Quoted 8 lines

tools/power/crypto/crypto_hibernate.c new file +8 -0

+}
+
+static int read_salt(void)
+{
+	int fd;
+
+	fd = open(CRYPTO_FILE, O_RDWR);
+	if (fd < 0) {

tools/power/crypto/crypto_hibernate.c line 368

Quoted 6 lines

tools/power/crypto/crypto_hibernate.c new file +6 -0

+		printf("Cannot open device file...\n");
+		return -EINVAL;
+	}
+	ioctl(fd, HIBERNATE_SALT_READ, get_salt_ptr());
+	return 0;
+}

No error checking on the ioctl?

Quoted 14 lines

tools/power/crypto/crypto_hibernate.c new file +14 -0

+int main(int argc, char *argv[])
+{
+	int opt, option_index = 0;
+	char in_passphrase[MAX_PASSPHRASE_SIZE];
+
+	while ((opt = getopt_long_only(argc, argv, "+p:s:h",
+				NULL, &option_index)) != -1) {
+		switch (opt) {
+		case 'p':
+			{
+				char *p = optarg;
+
+				if (strlen(p) >= (MAX_PASSPHRASE_SIZE - 1)) {
+					printf("Please provide passphrase less than %d bytes.\n",

tools/power/crypto/crypto_hibernate.c line 418

Quoted 4 lines

tools/power/crypto/crypto_hibernate.c new file +4 -0

+						MAX_PASSPHRASE_SIZE);
+					exit(1);
+				}
+				strcpy(in_passphrase, p);

I haven't read this super closely, but this really looks like an off-by-one error. It seems you intended MAX_PASSPHRASE_SIZE to include a null terminator, so the correct check would be 'strlen(p) >= MAX_PASSPHRASE_SIZE'.

Quoted 9 lines

tools/power/crypto/crypto_hibernate.c new file +9 -0

+			}
+			break;
+		case 's':
+			{
+				char *p = optarg;
+
+				if (strlen(p) != (SALT_BYTES - 1)) {
+					printf("Please provide salt with len less than %d bytes.\n",
+						SALT_BYTES);

tools/power/crypto/crypto_hibernate.c line 432

Quoted 5 lines

tools/power/crypto/crypto_hibernate.c new file +5 -0

+					exit(1);
+				}
+				strcpy(get_salt_ptr(), p);
+			}
+			break;

Salts don't need to be human-readable. How about making the salt binary? So, a salt specified on the command-line would be hex.

Eric

Hi Eric,

Quoted 15 lines · Eric Biggers

Hi Chen,

Quoted 12 lines · Chen Yu

crypto_hibernate is a user-space utility to generate 512bits AES key and pass it to the kernel via ioctl for hibernation encryption.(We can also add the key into kernel via keyctl if necessary, but currently using ioctl seems to be more straightforward as we need both the key and salt transit).

The key derivation is based on a simplified implementation of PBKDF2[1] in e2fsprogs - both the key length and the hash bytes are the same - 512bits. crypto_hibernate will firstly probe the user for passphrase and get salt from kernel, then uses them to generate a 512bits AES key based on PBKDF2.

Thanks for reviewing this.

Quoted 3 lines

What is a "512bits AES key"? Do you mean AES-256-XTS (which takes a 512-bit key, which the XTS mode internally splits into two keys)?

Yes, it is AES-256-XTS.

Quoted 2 lines

Do you allow for
other algorithms, or is it hardcoded to AES-256-XTS?

Currently it is hardcoded to AES-256-XTS. It is copied from implementation of PBKDF2 in e2fsprogs, which is hardcoded to useAES-256-XTS for ext4 encryption in the kernel(pbkdf2_sha512() in e2fsprogs)

Quoted 3 lines

What if someone wants to
use a different algorithm?

If user wants to use a different algorithm, then I think we need to port the code from openssl, which is the full implementation of PBKDF2 for:
https://www.ietf.org/rfc/rfc2898.txt

Quoted 4 lines

BTW, it's difficult to review this with only patch 3/3 Cc'ed to me, as there is no context about the problem you are trying to solve and what your actual proposed kernel changes are. I suggest Cc'ing linux-crypto on all 3 patches.

Ok, I'll send a refreshed version.

Quoted 30 lines

A few more comments below, from a very brief reading of the code:

[...]

Quoted 26 lines

tools/power/crypto/crypto_hibernate.c new file +26 -0

+
+#define PBKDF2_ITERATIONS          0xFFFF
+#define SHA512_BLOCKSIZE 128
+#define SHA512_LENGTH 64
+#define SALT_BYTES	16
+#define SYM_KEY_BYTES SHA512_LENGTH
+#define TOTAL_USER_INFO_LEN	(SALT_BYTES+SYM_KEY_BYTES)
+#define MAX_PASSPHRASE_SIZE	1024
+
+struct hibernation_crypto_keys {
+	char derived_key[SYM_KEY_BYTES];
+	char salt[SALT_BYTES];
+	bool valid;
+};
+
+struct hibernation_crypto_keys hib_keys;
+
+static char *get_key_ptr(void)
+{
+	return hib_keys.derived_key;
+}
+
+static char *get_salt_ptr(void)
+{
+	return hib_keys.salt;
+}

[...]

Quoted 8 lines

tools/power/crypto/crypto_hibernate.c line 305

Quoted 4 lines

tools/power/crypto/crypto_hibernate.c new file +4 -0

+
+
+#define HIBERNATE_SALT_READ      _IOW('C', 3, struct hibernation_crypto_keys)
+#define HIBERNATE_KEY_WRITE     _IOW('C', 4, struct hibernation_crypto_keys)

Why are the ioctl numbers defined based on the size of 'struct hibernation_crypto_keys'? It's not a UAPI structure, right?

It's not a UAPI structure, and it is defined both in user space tool and in kernel. Do you mean, I should put the defination of this structure under include/uapi ?

Quoted 28 lines

Quoted 28 lines

tools/power/crypto/crypto_hibernate.c new file +28 -0

+
+static void get_passphrase(char *passphrase, int len)
+{
+	char *p;
+	struct termios current_settings;
+
+	assert(len > 0);
+	disable_echo(&current_settings);
+	p = fgets(passphrase, len, stdin);
+	tcsetattr(0, TCSANOW, &current_settings);
+	printf("\n");
+	if (!p) {
+		printf("Aborting.\n");
+		exit(1);
+	}
+	p = strrchr(passphrase, '\n');
+	if (!p)
+		p = passphrase + len - 1;
+	*p = '\0';
+}
+
+#define CRYPTO_FILE	"/dev/crypto_hibernate"
+
+static int write_keys(void)
+{
+	int fd;
+
+	fd = open(CRYPTO_FILE, O_RDWR);

Quoted 9 lines

tools/power/crypto/crypto_hibernate.c line 354

Quoted 6 lines

tools/power/crypto/crypto_hibernate.c new file +6 -0

+	if (fd < 0) {
+		printf("Cannot open device file...\n");
+		return -EINVAL;
+	}
+	ioctl(fd, HIBERNATE_KEY_WRITE, get_key_ptr());
+	return 0;

No error checking on the ioctl?

Ok, will add error checking for it.

Quoted 4 lines

Also, how is the kernel supposed to know how long the key is, and which algorithm it's supposed to be used for? I assume it's hardcoded to AES-256-XTS? What if someone wants to use a different algorithm?

Yes, the length in both user space and kernel are hardcoded to AES-256-XTS. I can add the support for other algorithm, but might need to port from openssl first.

crypto_hibernate is a user-space utility to generate 512bits AES key and pass it to the kernel via ioctl for hibernation encryption.(We can also add the key into kernel via keyctl if necessary, but currently using ioctl seems to be more straightforward as we need both the key and salt transit).

Suggested-by "Theodore Ts'o" <[email protected]>

Quoted 6 lines · Chen Yu

crypto_hibernate is a user-space utility to generate 512bits AES key and pass it to the kernel via ioctl for hibernation encryption.(We can also add the key into kernel via keyctl if necessary, but currently using ioctl seems to be more straightforward as we need both the key and salt transit).

Ah, here you go:

https://en.wikipedia.org/wiki/Uswsusp

It can already do encryption. If you see problems there, patches will be gladly accepted to fix them.

If you want to maintain uswsusp in kernel tree, I guess that would be an option, too.

If you want to just help swsusp, I have some patches that reduce duplicities between x86-32 and x86-64. It would be cool to clean them up and get them merged.

Quoted 12 lines · Pavel Machek

Quoted 6 lines · Chen Yu

crypto_hibernate is a user-space utility to generate 512bits AES key and pass it to the kernel via ioctl for hibernation encryption.(We can also add the key into kernel via keyctl if necessary, but currently using ioctl seems to be more straightforward as we need both the key and salt transit).

Ah, here you go:

Quoted 1 line

https://en.wikipedia.org/wiki/Uswsusp

No, not really.

Quoted 2 lines

It can already do encryption. If you see problems there, patches will be gladly accepted to fix them.

By whom?

Quoted 2 lines

If you want to maintain uswsusp in kernel tree, I guess that would be an option, too.

No, it isn't an option due to the dependencies on crypto libraries.

Quoted 15 lines · Rafael J. Wysocki

Quoted 13 lines · Pavel Machek

Quoted 6 lines · Chen Yu

crypto_hibernate is a user-space utility to generate 512bits AES key and pass it to the kernel via ioctl for hibernation encryption.(We can also add the key into kernel via keyctl if necessary, but currently using ioctl seems to be more straightforward as we need both the key and salt transit).

Ah, here you go:

https://en.wikipedia.org/wiki/Uswsusp

Quoted 6 lines

No, not really.

Quoted 2 lines

It can already do encryption. If you see problems there, patches will be gladly accepted to fix them.

By whom?

By me.

Quoted 4 lines

Quoted 2 lines

If you want to maintain uswsusp in kernel tree, I guess that would be an option, too.

No, it isn't an option due to the dependencies on crypto libraries.

Why not?