Skip to contentSkip to conversation

cryptoapi highmem bug

PATCH In review Christophe Saout 4 participants 28 messages Feb 24, 2004 – Feb 26, 2004 Crypto API

lore
Share & tools
lore

Christophe Saout

author
#0 lore raw

Hi,

someone noticed strange corruptions with dm-crypt and highmem. After I found out that I could force my machine to use highmem even though it only has 256MB, I finally found the problem after some debugging:

The problem is in cbc_process (well, partly):

Quoted 3 lines
     const int need_stack = (src == dst);
     u8 stack[need_stack ? crypto_tfm_alg_blocksize(tfm) : 0];
     u8 *buf = need_stack ? stack : dst;

src == dst fails if the page was in highmem because crypto_kmap will assign two different virtual addresses for the same page.

The result is data corruption.

How could this be fixed?

scapperwalk_map could check if this page was already mapped (walk_in) and reuse the virtual address if so. So a single page is only mapped once and the check in cbc_process will work.

I can really use the src == dst case because I would need to allocate unnecessary buffers (at least 512 bytes at a time and per cpu).

(I just hacked dm-crypt to allocate 512 bytes on the stack and use it temporarily and kmap around myself to copy it back and the problem is gone. Ugly.)

#1 lore raw

Christophe,

What is calling cbc_process directly? I don't see how any other function could possibly call this function directly.

cipher.c's cipher() function called cbc_process() with two different src and dst buffers, *always*.

Have any more info?

JLC

Quoted 36 lines · Christophe Saout

Hi,

someone noticed strange corruptions with dm-crypt and highmem. After I found out that I could force my machine to use highmem even though it only has 256MB, I finally found the problem after some debugging:

The problem is in cbc_process (well, partly):

Quoted 3 lines
     const int need_stack = (src == dst);
     u8 stack[need_stack ? crypto_tfm_alg_blocksize(tfm) : 0];
     u8 *buf = need_stack ? stack : dst;

src == dst fails if the page was in highmem because crypto_kmap will assign two different virtual addresses for the same page.

The result is data corruption.

How could this be fixed?

scapperwalk_map could check if this page was already mapped (walk_in) and reuse the virtual address if so. So a single page is only mapped once and the check in cbc_process will work.

I can really use the src == dst case because I would need to allocate unnecessary buffers (at least 512 bytes at a time and per cpu).

(I just hacked dm-crypt to allocate 512 bytes on the stack and use it temporarily and kmap around myself to copy it back and the problem is gone. Ugly.)

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6
author
#2 lore raw

Am Di, den 24.02.2004 schrieb Jean-Luc Cooke um 23:34:

Quoted 2 lines

What is calling cbc_process directly? I don't see how any other function could possibly call this function directly.

Nobody is calling it directly.

Quoted 2 lines

cipher.c's cipher() function called cbc_process() with two different src and dst buffers, *always*.

It you pass the same to ->encrypt_iv (like kblockd for reads) it will kmap the same page twice and call cbc_process with two different virtual addresses pointing to the same page.

#3 lore raw
Quoted 5 lines · Christophe Saout

Am Di, den 24.02.2004 schrieb Jean-Luc Cooke um 23:34:

Quoted 2 lines

What is calling cbc_process directly? I don't see how any other function could possibly call this function directly.

Quoted 8 lines

Nobody is calling it directly.

Quoted 2 lines

cipher.c's cipher() function called cbc_process() with two different src and dst buffers, *always*.

It you pass the same to ->encrypt_iv (like kblockd for reads) it will kmap the same page twice and call cbc_process with two different virtual addresses pointing to the same page.

Ah right. Wanring: this is a cryptographer hacking kernel - danger!

How do I check for equal real addresses from two virtual ones?

JLC

Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6
#4 lore raw
Quoted 2 lines · Jean-Luc Cooke

How do I check for equal real addresses from two virtual ones?

I don't think there is a practical way of doing this. It would involve comparing the virtual address with the kmap and atomic kmap regions, performing a pagetable walk, extracting the pageframe. If the page is not in a kmap area generate the pageframe directly. Make that work on all architectures. Very yuk.

If practical this API should have been defined in terms of (page/offset/len) and it should have kmapped the pages itself. I guess it's too late for that.

#5 lore raw
Quoted 10 lines · Andrew Morton
Quoted 2 lines · Jean-Luc Cooke

How do I check for equal real addresses from two virtual ones?

I don't think there is a practical way of doing this. It would involve comparing the virtual address with the kmap and atomic kmap regions, performing a pagetable walk, extracting the pageframe. If the page is not in a kmap area generate the pageframe directly. Make that work on all architectures. Very yuk.

Quoted 3 lines

If practical this API should have been defined in terms of (page/offset/len) and it should have kmapped the pages itself. I guess it's too late for that.

Do you mean that the crypt() function should do kmapping?

It's not too late to change internals.

- James

Signature · 2 lines
James Morris
<[email protected]>
#6 lore raw
Quoted 7 lines · James Morris
Quoted 3 lines

If practical this API should have been defined in terms of (page/offset/len) and it should have kmapped the pages itself. I guess it's too late for that.

Do you mean that the crypt() function should do kmapping?

It's not too late to change internals.

Can I get an example of how to do this with scatterlists?

JLC

Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6
#7 lore raw
In reply to James Morris
Quoted 13 lines · James Morris
Quoted 10 lines · Andrew Morton
Quoted 2 lines · Jean-Luc Cooke

How do I check for equal real addresses from two virtual ones?

I don't think there is a practical way of doing this. It would involve comparing the virtual address with the kmap and atomic kmap regions, performing a pagetable walk, extracting the pageframe. If the page is not in a kmap area generate the pageframe directly. Make that work on all architectures. Very yuk.

Quoted 5 lines
Quoted 3 lines

If practical this API should have been defined in terms of (page/offset/len) and it should have kmapped the pages itself. I guess it's too late for that.

Do you mean that the crypt() function should do kmapping?

Passing the page* down one more level would permit cbc_process() to get at the pageframes themselves, so it can sanely determine if src and dest overlap. (I assume this is for encryption-in-place).

Or maybe it's sufficient for crypt() to pass a simple boolean down to the prfn() callout which says "this is in-place encryption".

author
#8 lore raw
Quoted 2 lines · Andrew Morton

Or maybe it's sufficient for crypt() to pass a simple boolean down to the prfn() callout which says "this is in-place encryption".

Yes, this works. As usual I was simply thinking too complicated. ;)

It looks like this now. It's on top of Jean-Luc's patch. Jean-Luc, can update your patch and use this one instead? It's much simpler this way and it keeps the whole voodoo in cipher.c.

crypto/cipher.c +13 -13
diff -ur linux.orig/crypto/cipher.c linux/crypto/cipher.c
@@ -26,7 +26,7 @@
 
 typedef void (cryptfn_t)(void *, u8 *, const u8 *);
 typedef void (procfn_t)(struct crypto_tfm *, u8 *,
-                        u8*, cryptfn_t, int enc, void *);
+                        u8*, cryptfn_t, int enc, void *, int);
 
 static inline void xor_64(u8 *a, const u8 *b)
 {

@@ -81,7 +81,9 @@
 
 		scatterwalk_copychunks(src_p, &walk_in, bsize, 0);
 
-		prfn(tfm, dst_p, src_p, crfn, enc, info);
+		prfn(tfm, dst_p, src_p, crfn, enc, info,
+		     scatterwalk_samebuf(&walk_in, &walk_out,
+					 src_p, dst_p));
 
 		scatterwalk_done(&walk_in, 0, nbytes);
 

@@ -185,8 +187,8 @@
 }
 #endif /* CONFIG_CRYPTO_OMAC */
 
-static void cbc_process(struct crypto_tfm *tfm,
-                        u8 *dst, u8 *src, cryptfn_t fn, int enc, void *info)
+static void cbc_process(struct crypto_tfm *tfm, u8 *dst, u8 *src,
+			cryptfn_t fn, int enc, void *info, int in_place)
 {
 	u8 *iv = info;
 	

@@ -202,9 +204,8 @@
 		fn(crypto_tfm_ctx(tfm), dst, iv);
 		memcpy(iv, dst, crypto_tfm_alg_blocksize(tfm));
 	} else {
-		const int need_stack = (src == dst);
-		u8 stack[need_stack ? crypto_tfm_alg_blocksize(tfm) : 0];
-		u8 *buf = need_stack ? stack : dst;
+		u8 stack[in_place ? crypto_tfm_alg_blocksize(tfm) : 0];
+		u8 *buf = in_place ? stack : dst;
 		
 		fn(crypto_tfm_ctx(tfm), buf, src);
 		tfm->crt_u.cipher.cit_xor_block(buf, iv);

@@ -214,13 +215,12 @@
 	}
 }
 
-static void ctr_process(struct crypto_tfm *tfm,
-                        u8 *dst, u8 *src, cryptfn_t fn, int enc, void *info)
+static void ctr_process(struct crypto_tfm *tfm, u8 *dst, u8 *src,
+			cryptfn_t fn, int enc, void *info, int in_place)
 {
 	u8 *iv = info;
-	const int need_stack = (src == dst);
-	u8 stack[need_stack ? crypto_tfm_alg_blocksize(tfm) : 0];
-	u8 *buf = need_stack ? stack : dst;
+	u8 stack[in_place ? crypto_tfm_alg_blocksize(tfm) : 0];
+	u8 *buf = in_place ? stack : dst;
 	int i;
 
 	/* Null encryption */

@@ -255,7 +255,7 @@
 
 
 static void ecb_process(struct crypto_tfm *tfm, u8 *dst, u8 *src,
-                        cryptfn_t fn, int enc, void *info)
+			cryptfn_t fn, int enc, void *info, int in_place)
 {
 	/* we can use dst as scratch space since we overwrite it later */
 	omac_update(tfm, dst, src);
crypto/scatterwalk.h +8 -0
diff -ur linux.orig/crypto/scatterwalk.h linux/crypto/scatterwalk.h
@@ -38,6 +38,14 @@
 	return sg + 1;
 }
 
+static inline int scatterwalk_samebuf(struct scatter_walk *walk_in,
+				      struct scatter_walk *walk_out,
+				      void *src_p, void *dst_p)
+{
+	return walk_in->page == walk_out->page &&
+	       walk_in->data == src_p && walk_out->data == dst_p;
+}
+
 void *scatterwalk_whichbuf(struct scatter_walk *walk, unsigned int nbytes, void *scratch);
 void scatterwalk_start(struct scatter_walk *walk, struct scatterlist *sg);
 int scatterwalk_copychunks(void *buf, struct scatter_walk *walk, size_t nbytes, int out);
#9 lore raw

Read my mind Christophe!

This...
http://jlcooke.ca/lkml/crypto_omac_hmac_ctr_25feb2004.patch

...and C.Saout's latest patch in single serving form:
http://jlcooke.ca/lkml/crypto_omac_hmac_ctr_25feb2004_b.patch

JLC

Quoted 102 lines · Christophe Saout
Quoted 2 lines · Andrew Morton

Or maybe it's sufficient for crypt() to pass a simple boolean down to the prfn() callout which says "this is in-place encryption".

Yes, this works. As usual I was simply thinking too complicated. ;)

It looks like this now. It's on top of Jean-Luc's patch. Jean-Luc, can update your patch and use this one instead? It's much simpler this way and it keeps the whole voodoo in cipher.c.

diff -ur linux.orig/crypto/cipher.c linux/crypto/cipher.c

crypto/cipher.c +13 -13
@@ -26,7 +26,7 @@
 
 typedef void (cryptfn_t)(void *, u8 *, const u8 *);
 typedef void (procfn_t)(struct crypto_tfm *, u8 *,
-                        u8*, cryptfn_t, int enc, void *);
+                        u8*, cryptfn_t, int enc, void *, int);
 
 static inline void xor_64(u8 *a, const u8 *b)
 {

@@ -81,7 +81,9 @@
 
 		scatterwalk_copychunks(src_p, &walk_in, bsize, 0);
 
-		prfn(tfm, dst_p, src_p, crfn, enc, info);
+		prfn(tfm, dst_p, src_p, crfn, enc, info,
+		     scatterwalk_samebuf(&walk_in, &walk_out,
+					 src_p, dst_p));
 
 		scatterwalk_done(&walk_in, 0, nbytes);
 

@@ -185,8 +187,8 @@
 }
 #endif /* CONFIG_CRYPTO_OMAC */
 
-static void cbc_process(struct crypto_tfm *tfm,
-                        u8 *dst, u8 *src, cryptfn_t fn, int enc, void *info)
+static void cbc_process(struct crypto_tfm *tfm, u8 *dst, u8 *src,
+			cryptfn_t fn, int enc, void *info, int in_place)
 {
 	u8 *iv = info;
 

@@ -202,9 +204,8 @@
 		fn(crypto_tfm_ctx(tfm), dst, iv);
 		memcpy(iv, dst, crypto_tfm_alg_blocksize(tfm));
 	} else {
-		const int need_stack = (src == dst);
-		u8 stack[need_stack ? crypto_tfm_alg_blocksize(tfm) : 0];
-		u8 *buf = need_stack ? stack : dst;
+		u8 stack[in_place ? crypto_tfm_alg_blocksize(tfm) : 0];
+		u8 *buf = in_place ? stack : dst;
 
 		fn(crypto_tfm_ctx(tfm), buf, src);
 		tfm->crt_u.cipher.cit_xor_block(buf, iv);

@@ -214,13 +215,12 @@
 	}
 }
 
-static void ctr_process(struct crypto_tfm *tfm,
-                        u8 *dst, u8 *src, cryptfn_t fn, int enc, void *info)
+static void ctr_process(struct crypto_tfm *tfm, u8 *dst, u8 *src,
+			cryptfn_t fn, int enc, void *info, int in_place)
 {
 	u8 *iv = info;
-	const int need_stack = (src == dst);
-	u8 stack[need_stack ? crypto_tfm_alg_blocksize(tfm) : 0];
-	u8 *buf = need_stack ? stack : dst;
+	u8 stack[in_place ? crypto_tfm_alg_blocksize(tfm) : 0];
+	u8 *buf = in_place ? stack : dst;
 	int i;
 
 	/* Null encryption */

@@ -255,7 +255,7 @@
 
 
 static void ecb_process(struct crypto_tfm *tfm, u8 *dst, u8 *src,
-                        cryptfn_t fn, int enc, void *info)
+			cryptfn_t fn, int enc, void *info, int in_place)
 {
 	/* we can use dst as scratch space since we overwrite it later */
 	omac_update(tfm, dst, src);

diff -ur linux.orig/crypto/scatterwalk.h linux/crypto/scatterwalk.h

crypto/scatterwalk.h +8 -0
@@ -38,6 +38,14 @@
 	return sg + 1;
 }
 
+static inline int scatterwalk_samebuf(struct scatter_walk *walk_in,
+				      struct scatter_walk *walk_out,
+				      void *src_p, void *dst_p)
+{
+	return walk_in->page == walk_out->page &&
+	       walk_in->data == src_p && walk_out->data == dst_p;
+}
+
 void *scatterwalk_whichbuf(struct scatter_walk *walk, unsigned int nbytes, void *scratch);
 void scatterwalk_start(struct scatter_walk *walk, struct scatterlist *sg);
 int scatterwalk_copychunks(void *buf, struct scatter_walk *walk, size_t nbytes, int out);
Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6
author
#10 lore raw

PATCH 1/2 move scatterwalk functions to own file

In reply to Andrew Morton

Hi,

I've cleaned up the latest patches and adjusted the header files.

This patch moves the scatterwalk functions from cipher.c to scatterwalk.c and adds a header file.

crypto/cipher.c +8 -128
diff -Nur linux.orig/crypto/cipher.c linux/crypto/cipher.c
@@ -4,7 +4,6 @@
  * Cipher operations.
  *
  * Copyright (c) 2002 James Morris <[email protected]>
- * Generic scatterwalk code by Adam J. Richter <[email protected]>.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the Free

@@ -17,31 +16,14 @@
 #include <linux/errno.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
-#include <linux/pagemap.h>
-#include <linux/highmem.h>
 #include <asm/scatterlist.h>
 #include "internal.h"
+#include "scatterwalk.h"
 
 typedef void (cryptfn_t)(void *, u8 *, const u8 *);
 typedef void (procfn_t)(struct crypto_tfm *, u8 *,
                         u8*, cryptfn_t, int enc, void *);
 
-struct scatter_walk {
-	struct scatterlist	*sg;
-	struct page		*page;
-	void			*data;
-	unsigned int		len_this_page;
-	unsigned int		len_this_segment;
-	unsigned int		offset;
-};
-
-enum km_type crypto_km_types[] = {
-	KM_USER0,
-	KM_USER1,
-	KM_SOFTIRQ0,
-	KM_SOFTIRQ1,
-};
-
 static inline void xor_64(u8 *a, const u8 *b)
 {
 	((u32 *)a)[0] ^= ((u32 *)b)[0];

@@ -57,108 +39,6 @@
 }
 
 
-/* Define sg_next is an inline routine now in case we want to change
-   scatterlist to a linked list later. */
-static inline struct scatterlist *sg_next(struct scatterlist *sg)
-{
-	return sg + 1;
-}
-
-void *which_buf(struct scatter_walk *walk, unsigned int nbytes, void *scratch)
-{
-	if (nbytes <= walk->len_this_page &&
-	    (((unsigned long)walk->data) & (PAGE_CACHE_SIZE - 1)) + nbytes <=
-	    PAGE_CACHE_SIZE)
-		return walk->data;
-	else
-		return scratch;
-}
-
-static void memcpy_dir(void *buf, void *sgdata, size_t nbytes, int out)
-{
-	if (out)
-		memcpy(sgdata, buf, nbytes);
-	else
-		memcpy(buf, sgdata, nbytes);
-}
-
-static void scatterwalk_start(struct scatter_walk *walk, struct scatterlist *sg)
-{
-	unsigned int rest_of_page;
-
-	walk->sg = sg;
-
-	walk->page = sg->page;
-	walk->len_this_segment = sg->length;
-
-	rest_of_page = PAGE_CACHE_SIZE - (sg->offset & (PAGE_CACHE_SIZE - 1));
-	walk->len_this_page = min(sg->length, rest_of_page);
-	walk->offset = sg->offset;
-}
-
-static void scatterwalk_map(struct scatter_walk *walk, int out)
-{
-	walk->data = crypto_kmap(walk->page, out) + walk->offset;
-}
-
-static void scatter_page_done(struct scatter_walk *walk, int out,
-			      unsigned int more)
-{
-	/* walk->data may be pointing the first byte of the next page;
-	   however, we know we transfered at least one byte.  So,
-	   walk->data - 1 will be a virutual address in the mapped page. */
-
-	if (out)
-		flush_dcache_page(walk->page);
-
-	if (more) {
-		walk->len_this_segment -= walk->len_this_page;
-
-		if (walk->len_this_segment) {
-			walk->page++;
-			walk->len_this_page = min(walk->len_this_segment,
-						  (unsigned)PAGE_CACHE_SIZE);
-			walk->offset = 0;
-		}
-		else
-			scatterwalk_start(walk, sg_next(walk->sg));
-	}
-}
-
-static void scatter_done(struct scatter_walk *walk, int out, int more)
-{
-	crypto_kunmap(walk->data, out);
-	if (walk->len_this_page == 0 || !more)
-		scatter_page_done(walk, out, more);
-}
-
-/*
- * Do not call this unless the total length of all of the fragments 
- * has been verified as multiple of the block size.
- */
-static int copy_chunks(void *buf, struct scatter_walk *walk,
-			size_t nbytes, int out)
-{
-	if (buf != walk->data) {
-		while (nbytes > walk->len_this_page) {
-			memcpy_dir(buf, walk->data, walk->len_this_page, out);
-			buf += walk->len_this_page;
-			nbytes -= walk->len_this_page;
-
-			crypto_kunmap(walk->data, out);
-			scatter_page_done(walk, out, 1);
-			scatterwalk_map(walk, out);
-		}
-
-		memcpy_dir(buf, walk->data, nbytes, out);
-	}
-
-	walk->offset += nbytes;
-	walk->len_this_page -= nbytes;
-	walk->len_this_segment -= nbytes;
-	return 0;
-}
-
 /* 
  * Generic encrypt/decrypt wrapper for ciphers, handles operations across
  * multiple page boundaries by using temporary blocks.  In user context,

@@ -191,19 +71,19 @@
 
 		scatterwalk_map(&walk_in, 0);
 		scatterwalk_map(&walk_out, 1);
-		src_p = which_buf(&walk_in, bsize, tmp_src);
-		dst_p = which_buf(&walk_out, bsize, tmp_dst);
+		src_p = scatterwalk_whichbuf(&walk_in, bsize, tmp_src);
+		dst_p = scatterwalk_whichbuf(&walk_out, bsize, tmp_dst);
 
 		nbytes -= bsize;
 
-		copy_chunks(src_p, &walk_in, bsize, 0);
+		scatterwalk_copychunks(src_p, &walk_in, bsize, 0);
 
 		prfn(tfm, dst_p, src_p, crfn, enc, info);
 
-		scatter_done(&walk_in, 0, nbytes);
+		scatterwalk_done(&walk_in, 0, nbytes);
 
-		copy_chunks(dst_p, &walk_out, bsize, 1);
-		scatter_done(&walk_out, 1, nbytes);
+		scatterwalk_copychunks(dst_p, &walk_out, bsize, 1);
+		scatterwalk_done(&walk_out, 1, nbytes);
 
 		if (!nbytes)
 			return 0;

@@ -229,7 +109,7 @@
 		const int need_stack = (src == dst);
 		u8 stack[need_stack ? crypto_tfm_alg_blocksize(tfm) : 0];
 		u8 *buf = need_stack ? stack : dst;
-		
+
 		fn(crypto_tfm_ctx(tfm), buf, src);
 		tfm->crt_u.cipher.cit_xor_block(buf, iv);
 		memcpy(iv, src, crypto_tfm_alg_blocksize(tfm));
crypto/internal.h +1 -0
diff -Nur linux.orig/crypto/internal.h linux/crypto/internal.h
@@ -11,6 +11,7 @@
  */
 #ifndef _CRYPTO_INTERNAL_H
 #define _CRYPTO_INTERNAL_H
+#include <linux/crypto.h>
 #include <linux/mm.h>
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
crypto/Makefile +1 -1
diff -Nur linux.orig/crypto/Makefile linux/crypto/Makefile
@@ -4,7 +4,7 @@
 
 proc-crypto-$(CONFIG_PROC_FS) = proc.o
 
-obj-$(CONFIG_CRYPTO) += api.o cipher.o digest.o compress.o \
+obj-$(CONFIG_CRYPTO) += api.o scatterwalk.o cipher.o digest.o compress.o \
 			$(proc-crypto-y)
 
 obj-$(CONFIG_CRYPTO_HMAC) += hmac.o
crypto/scatterwalk.c +124 -0

124 lines not on this page · raw message

crypto/scatterwalk.h +42 -0

42 lines not on this page · raw message

author
#11 lore raw

PATCH 2/2 fix in-place de/encryption bug with highmem

In reply to Andrew Morton

This patch fixes the bug where in-place encryption was not detected when the same highmem pages is mapped twice to different virtual addresses.

This adds a parameter to xxx_process to indicate whether this is an in-place encryption and moves the responsability to the caller using a helper function scatterwalk.h.

crypto/cipher.c +9 -8
diff -Nur linux.orig/crypto/cipher.c linux/crypto/cipher.c
@@ -22,7 +22,7 @@
 
 typedef void (cryptfn_t)(void *, u8 *, const u8 *);
 typedef void (procfn_t)(struct crypto_tfm *, u8 *,
-                        u8*, cryptfn_t, int enc, void *);
+                        u8*, cryptfn_t, int enc, void *, int);
 
 static inline void xor_64(u8 *a, const u8 *b)
 {

@@ -78,7 +78,9 @@
 
 		scatterwalk_copychunks(src_p, &walk_in, bsize, 0);
 
-		prfn(tfm, dst_p, src_p, crfn, enc, info);
+		prfn(tfm, dst_p, src_p, crfn, enc, info,
+		     scatterwalk_samebuf(&walk_in, &walk_out,
+					 src_p, dst_p));
 
 		scatterwalk_done(&walk_in, 0, nbytes);
 

@@ -92,8 +94,8 @@
 	}
 }
 
-static void cbc_process(struct crypto_tfm *tfm,
-                        u8 *dst, u8 *src, cryptfn_t fn, int enc, void *info)
+static void cbc_process(struct crypto_tfm *tfm, u8 *dst, u8 *src,
+			cryptfn_t fn, int enc, void *info, int in_place)
 {
 	u8 *iv = info;
 	

@@ -106,9 +108,8 @@
 		fn(crypto_tfm_ctx(tfm), dst, iv);
 		memcpy(iv, dst, crypto_tfm_alg_blocksize(tfm));
 	} else {
-		const int need_stack = (src == dst);
-		u8 stack[need_stack ? crypto_tfm_alg_blocksize(tfm) : 0];
-		u8 *buf = need_stack ? stack : dst;
+		u8 stack[in_place ? crypto_tfm_alg_blocksize(tfm) : 0];
+		u8 *buf = in_place ? stack : dst;
 
 		fn(crypto_tfm_ctx(tfm), buf, src);
 		tfm->crt_u.cipher.cit_xor_block(buf, iv);

@@ -119,7 +120,7 @@
 }
 
 static void ecb_process(struct crypto_tfm *tfm, u8 *dst, u8 *src,
-                        cryptfn_t fn, int enc, void *info)
+			cryptfn_t fn, int enc, void *info, int in_place)
 {
 	fn(crypto_tfm_ctx(tfm), dst, src);
 }
crypto/scatterwalk.h +8 -0
diff -Nur linux.orig/crypto/scatterwalk.h linux/crypto/scatterwalk.h
@@ -33,6 +33,14 @@
 	return sg + 1;
 }
 
+static inline int scatterwalk_samebuf(struct scatter_walk *walk_in,
+				      struct scatter_walk *walk_out,
+				      void *src_p, void *dst_p)
+{
+	return walk_in->page == walk_out->page &&
+	       walk_in->data == src_p && walk_out->data == dst_p;
+}
+
 void *scatterwalk_whichbuf(struct scatter_walk *walk, unsigned int nbytes, void *scratch);
 void scatterwalk_start(struct scatter_walk *walk, struct scatterlist *sg);
 int scatterwalk_copychunks(void *buf, struct scatter_walk *walk, size_t nbytes, int out);
#12 lore raw

Re: [PATCH 2/2] fix in-place de/encryption bug with highmem

Quoted 7 lines · Christophe Saout

This patch fixes the bug where in-place encryption was not detected when the same highmem pages is mapped twice to different virtual addresses.

This adds a parameter to xxx_process to indicate whether this is an in-place encryption and moves the responsability to the caller using a helper function scatterwalk.h.

These patches look good (now in -mm4), thanks for doing this.

Have you verified that the data corruption bug you saw has been fixed? (Just in case there's more to it).

- James

Signature · 2 lines
James Morris
<[email protected]>
author
#13 lore raw

Re: [PATCH 2/2] fix in-place de/encryption bug with highmem

Am Do, den 26.02.2004 schrieb James Morris um 05:13:

Quoted 2 lines

Have you verified that the data corruption bug you saw has been fixed? (Just in case there's more to it).

Yes, it's fixed. I've got a collection of tests scripts here to verify the data integrity. They are able to trigger all kinds of bugs (assuming there is a bug of course).

author
#14 lore raw
In reply to Andrew Morton
Quoted 5 lines · Andrew Morton

I don't think there is a practical way of doing this. It would involve comparing the virtual address with the kmap and atomic kmap regions, performing a pagetable walk, extracting the pageframe. If the page is not in a kmap area generate the pageframe directly. Make that work on all architectures. Very yuk.

We could also "simply" avoid to map the same page twice. My first idea turned out slightly more complicated than I expected but works.

It's just a proof of concept though, it would be less complicated if we would just pass the other walk struct to the functions that take one and let them do the checking (no need to disable preemption and use per cpu variables). Hmm.

crypto/cipher.c +44 -4
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/crypto.h>
 #include <linux/errno.h>
+#include <linux/percpu.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/pagemap.h>

@@ -42,6 +43,14 @@
 	KM_SOFTIRQ1,
 };
 
+struct kmapped_page {
+	struct page	*page;
+	void		*vaddr;
+	int		out;
+};
+
+static DEFINE_PER_CPU(struct kmapped_page[4], kmapped_pages);
+
 static inline void xor_64(u8 *a, const u8 *b)
 {
 	((u32 *)a)[0] ^= ((u32 *)b)[0];

@@ -64,7 +73,7 @@
 	return sg + 1;
 }
 
-void *which_buf(struct scatter_walk *walk, unsigned int nbytes, void *scratch)
+static void *which_buf(struct scatter_walk *walk, unsigned int nbytes, void *scratch)
 {
 	if (nbytes <= walk->len_this_page &&
 	    (((unsigned long)walk->data) & (PAGE_CACHE_SIZE - 1)) + nbytes <=

@@ -98,7 +107,38 @@
 
 static void scatterwalk_map(struct scatter_walk *walk, int out)
 {
-	walk->data = crypto_kmap(walk->page, out) + walk->offset;
+	struct kmapped_page *kmaps;
+	int idx = (in_softirq() ? 2 : 0) + out;
+	void *vaddr;
+
+	preempt_disable();
+	kmaps = __get_cpu_var(kmapped_pages);
+
+	if (kmaps[idx ^ 1].page == walk->page) {
+		vaddr = kmaps[idx ^ 1].vaddr;
+		out = kmaps[idx ^ 1].out;
+	} else
+		vaddr = crypto_kmap(walk->page, out);
+
+	kmaps[idx].page = walk->page;
+	kmaps[idx].vaddr = vaddr;
+	kmaps[idx].out = out;
+
+	walk->data = vaddr + walk->offset;
+}
+
+static void scatterwalk_unmap(void *vaddr, int out)
+{
+	struct kmapped_page *kmaps = __get_cpu_var(kmapped_pages);
+	int idx = (in_softirq() ? 2 : 0) + out;
+
+	if (kmaps[idx ^ 1].page != kmaps[idx].page)
+		crypto_kunmap(vaddr, kmaps[idx].out);
+
+	kmaps[idx].page = NULL;
+	kmaps[idx].vaddr = NULL;
+
+	preempt_enable();
 }
 
 static void scatter_page_done(struct scatter_walk *walk, int out,

@@ -127,7 +167,7 @@
 
 static void scatter_done(struct scatter_walk *walk, int out, int more)
 {
-	crypto_kunmap(walk->data, out);
+	scatterwalk_unmap(walk->data, out);
 	if (walk->len_this_page == 0 || !more)
 		scatter_page_done(walk, out, more);
 }

@@ -145,7 +185,7 @@
 			buf += walk->len_this_page;
 			nbytes -= walk->len_this_page;
 
-			crypto_kunmap(walk->data, out);
+			scatterwalk_unmap(walk->data, out);
 			scatter_page_done(walk, out, 1);
 			scatterwalk_map(walk, out);
 		}
author
#15 lore raw
Quoted 4 lines · Christophe Saout

It's just a proof of concept though, it would be less complicated if we would just pass the other walk struct to the functions that take one and let them do the checking (no need to disable preemption and use per cpu variables). Hmm.

Ok, this also works. It makes copy_chunks and crypt responsible for tracking the walk struct which might contain a page to reuse:

crypto/cipher.c +30 -14
@@ -29,6 +29,7 @@
 struct scatter_walk {
 	struct scatterlist	*sg;
 	struct page		*page;
+	int			out;
 	void			*data;
 	unsigned int		len_this_page;
 	unsigned int		len_this_segment;

@@ -64,7 +65,7 @@
 	return sg + 1;
 }
 
-void *which_buf(struct scatter_walk *walk, unsigned int nbytes, void *scratch)
+static void *which_buf(struct scatter_walk *walk, unsigned int nbytes, void *scratch)
 {
 	if (nbytes <= walk->len_this_page &&
 	    (((unsigned long)walk->data) & (PAGE_CACHE_SIZE - 1)) + nbytes <=

@@ -96,9 +97,23 @@
 	walk->offset = sg->offset;
 }
 
-static void scatterwalk_map(struct scatter_walk *walk, int out)
+static void scatterwalk_map(struct scatter_walk *walk,
+			    struct scatter_walk *other, int out)
 {
-	walk->data = crypto_kmap(walk->page, out) + walk->offset;
+	if (other && other->page == walk->page) {
+		walk->data = (other->data - other->offset) + walk->offset;
+		walk->out = other->out;
+	} else {
+		walk->data = crypto_kmap(walk->page, out) + walk->offset;
+		walk->out = out;
+	}
+}
+
+static void scatterwalk_unmap(struct scatter_walk *walk,
+			      struct scatter_walk *other, int out)
+{
+	if (!other || other->page != walk->page)
+		crypto_kunmap(walk->data, walk->out);
 }
 
 static void scatter_page_done(struct scatter_walk *walk, int out,

@@ -125,9 +140,10 @@
 	}
 }
 
-static void scatter_done(struct scatter_walk *walk, int out, int more)
+static void scatter_done(struct scatter_walk *walk,
+			 struct scatter_walk *other, int out, int more)
 {
-	crypto_kunmap(walk->data, out);
+	scatterwalk_unmap(walk, other, out);
 	if (walk->len_this_page == 0 || !more)
 		scatter_page_done(walk, out, more);
 }

@@ -137,7 +153,7 @@
  * has been verified as multiple of the block size.
  */
 static int copy_chunks(void *buf, struct scatter_walk *walk,
-			size_t nbytes, int out)
+		       struct scatter_walk *other, size_t nbytes, int out)
 {
 	if (buf != walk->data) {
 		while (nbytes > walk->len_this_page) {

@@ -145,9 +161,9 @@
 			buf += walk->len_this_page;
 			nbytes -= walk->len_this_page;
 
-			crypto_kunmap(walk->data, out);
+			scatterwalk_unmap(walk, other, out);
 			scatter_page_done(walk, out, 1);
-			scatterwalk_map(walk, out);
+			scatterwalk_map(walk, other, out);
 		}
 
 		memcpy_dir(buf, walk->data, nbytes, out);

@@ -189,21 +205,21 @@
 	for(;;) {
 		u8 *src_p, *dst_p;
 
-		scatterwalk_map(&walk_in, 0);
-		scatterwalk_map(&walk_out, 1);
+		scatterwalk_map(&walk_in, NULL, 0);
+		scatterwalk_map(&walk_out, &walk_in, 1);
 		src_p = which_buf(&walk_in, bsize, tmp_src);
 		dst_p = which_buf(&walk_out, bsize, tmp_dst);
 
 		nbytes -= bsize;
 
-		copy_chunks(src_p, &walk_in, bsize, 0);
+		copy_chunks(src_p, &walk_in, &walk_out, bsize, 0);
 
 		prfn(tfm, dst_p, src_p, crfn, enc, info);
 
-		scatter_done(&walk_in, 0, nbytes);
+		scatter_done(&walk_in, &walk_out, 0, nbytes);
 
-		copy_chunks(dst_p, &walk_out, bsize, 1);
-		scatter_done(&walk_out, 1, nbytes);
+		copy_chunks(dst_p, &walk_out, NULL, bsize, 1);
+		scatter_done(&walk_out, NULL, 1, nbytes);
 
 		if (!nbytes)
 			return 0;
#16 lore raw

Not to be annoying...

Could you make this change against my patch at:
http://jlcooke.ca/lkml/crypto_28feb2004.patch

I moved all the scatterwalk stuff into a scatterwalk.c file.

JLC

Quoted 118 lines · Christophe Saout
Quoted 4 lines · Christophe Saout

It's just a proof of concept though, it would be less complicated if we would just pass the other walk struct to the functions that take one and let them do the checking (no need to disable preemption and use per cpu variables). Hmm.

Ok, this also works. It makes copy_chunks and crypt responsible for tracking the walk struct which might contain a page to reuse:

crypto/cipher.c +30 -14
@@ -29,6 +29,7 @@
 struct scatter_walk {
 	struct scatterlist	*sg;
 	struct page		*page;
+	int			out;
 	void			*data;
 	unsigned int		len_this_page;
 	unsigned int		len_this_segment;

@@ -64,7 +65,7 @@
 	return sg + 1;
 }
 
-void *which_buf(struct scatter_walk *walk, unsigned int nbytes, void *scratch)
+static void *which_buf(struct scatter_walk *walk, unsigned int nbytes, void *scratch)
 {
 	if (nbytes <= walk->len_this_page &&
 	    (((unsigned long)walk->data) & (PAGE_CACHE_SIZE - 1)) + nbytes <=

@@ -96,9 +97,23 @@
 	walk->offset = sg->offset;
 }
 
-static void scatterwalk_map(struct scatter_walk *walk, int out)
+static void scatterwalk_map(struct scatter_walk *walk,
+			    struct scatter_walk *other, int out)
 {
-	walk->data = crypto_kmap(walk->page, out) + walk->offset;
+	if (other && other->page == walk->page) {
+		walk->data = (other->data - other->offset) + walk->offset;
+		walk->out = other->out;
+	} else {
+		walk->data = crypto_kmap(walk->page, out) + walk->offset;
+		walk->out = out;
+	}
+}
+
+static void scatterwalk_unmap(struct scatter_walk *walk,
+			      struct scatter_walk *other, int out)
+{
+	if (!other || other->page != walk->page)
+		crypto_kunmap(walk->data, walk->out);
 }
 
 static void scatter_page_done(struct scatter_walk *walk, int out,

@@ -125,9 +140,10 @@
 	}
 }
 
-static void scatter_done(struct scatter_walk *walk, int out, int more)
+static void scatter_done(struct scatter_walk *walk,
+			 struct scatter_walk *other, int out, int more)
 {
-	crypto_kunmap(walk->data, out);
+	scatterwalk_unmap(walk, other, out);
 	if (walk->len_this_page == 0 || !more)
 		scatter_page_done(walk, out, more);
 }

@@ -137,7 +153,7 @@
  * has been verified as multiple of the block size.
  */
 static int copy_chunks(void *buf, struct scatter_walk *walk,
-			size_t nbytes, int out)
+		       struct scatter_walk *other, size_t nbytes, int out)
 {
 	if (buf != walk->data) {
 		while (nbytes > walk->len_this_page) {

@@ -145,9 +161,9 @@
 			buf += walk->len_this_page;
 			nbytes -= walk->len_this_page;
 
-			crypto_kunmap(walk->data, out);
+			scatterwalk_unmap(walk, other, out);
 			scatter_page_done(walk, out, 1);
-			scatterwalk_map(walk, out);
+			scatterwalk_map(walk, other, out);
 		}
 
 		memcpy_dir(buf, walk->data, nbytes, out);

@@ -189,21 +205,21 @@
 	for(;;) {
 		u8 *src_p, *dst_p;
 
-		scatterwalk_map(&walk_in, 0);
-		scatterwalk_map(&walk_out, 1);
+		scatterwalk_map(&walk_in, NULL, 0);
+		scatterwalk_map(&walk_out, &walk_in, 1);
 		src_p = which_buf(&walk_in, bsize, tmp_src);
 		dst_p = which_buf(&walk_out, bsize, tmp_dst);
 
 		nbytes -= bsize;
 
-		copy_chunks(src_p, &walk_in, bsize, 0);
+		copy_chunks(src_p, &walk_in, &walk_out, bsize, 0);
 
 		prfn(tfm, dst_p, src_p, crfn, enc, info);
 
-		scatter_done(&walk_in, 0, nbytes);
+		scatter_done(&walk_in, &walk_out, 0, nbytes);
 
-		copy_chunks(dst_p, &walk_out, bsize, 1);
-		scatter_done(&walk_out, 1, nbytes);
+		copy_chunks(dst_p, &walk_out, NULL, bsize, 1);
+		scatter_done(&walk_out, NULL, 1, nbytes);
 
 		if (!nbytes)
 			return 0;
Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6
author
#19 lore raw

cryptoapi OMAC (was: cryptoapi highmem bug)

Quoted 1 line · Jean-Luc Cooke

http://jlcooke.ca/lkml/crypto_24feb2004.patch will work.

It didn't compile. I fixed some compile problems so that it works. I have disabled CRYPTO_OMAC though. And it goes boom when calling hmac_init (or something like that), the machine halts without Oops.

I've seen the cit_omac is kmalloc'ed. Hmm. Didn't we want to try to avoid that? Well. Perhaps it should be allocated when the omac_init is called and freed after omac_final.

Or what about this:
If the user wants OMAC he calls omac_init and passes a pointer to a buffer where the omac will be computed. omac_init then sets the omac_update function so that xxx_process will call it. After the encryption is finished the user calls omac_final and finds the omac in his buffer.

And shouldn't the omac functions be put into a separate omac.c?

Moving the scatterwalk functions seems like a good idea to me.

Well, here are the compile fixes:

crypto/cipher.c +5 -2
diff -Nur linux.orig/crypto/cipher.c linux/crypto/cipher.c
@@ -280,7 +280,7 @@
 			/* mac = Zeros */
 			memset((u8*)tfm->crt_u.cipher.cit_omac, 0, crypto_tfm_alg_blocksize(tfm));
 		}
-#endif
+#endif /* CONFIG_CRYPTO_OMAC */
 
 		return ret;
 	}

@@ -475,9 +475,12 @@
 	    	ops->cit_iv = kmalloc(ops->cit_ivsize, GFP_KERNEL);
 		if (ops->cit_iv == NULL)
 			ret = -ENOMEM;
+
+#ifdef CONFIG_CRYPTO_OMAC
 		ops->cit_omac = kmalloc(ops->cit_ivsize, GFP_KERNEL);
 		if (ops->cit_omac == NULL)
 			ret = -ENOMEM;
+#endif /* CONFIG_CRYPTO_OMAC */
 	}
 
 #ifdef CONFIG_CRYPTO_OMAC

@@ -499,5 +502,5 @@
 #ifdef CONFIG_CRYPTO_OMAC
 	if (tfm->crt_cipher.cit_omac)
 		kfree(tfm->crt_cipher.cit_omac);
-#endif
+#endif /* CONFIG_CRYPTO_OMAC */
 }
#20 lore raw

Re: cryptoapi OMAC (was: cryptoapi highmem bug)

OK.

Using a base kernel, apply the scatterwalk change:
http://jlcooke.ca/lkml/cryptowalk_christophe_25feb2004.patch

Then apply this:
http://jlcooke.ca/lkml/crypto_omac_hmac_ctr_25feb2004.patch

This is my HMAC/OMAC/CTR patch. I think I fixed your HMAC issue. I was giving a scatterlist a stack memory reference (!). It now more closely uses the digest.c functions.

JLC

Quoted 62 lines · Christophe Saout
Quoted 1 line · Jean-Luc Cooke

http://jlcooke.ca/lkml/crypto_24feb2004.patch will work.

It didn't compile. I fixed some compile problems so that it works. I have disabled CRYPTO_OMAC though. And it goes boom when calling hmac_init (or something like that), the machine halts without Oops.

I've seen the cit_omac is kmalloc'ed. Hmm. Didn't we want to try to avoid that? Well. Perhaps it should be allocated when the omac_init is called and freed after omac_final.

Or what about this:
If the user wants OMAC he calls omac_init and passes a pointer to a buffer where the omac will be computed. omac_init then sets the omac_update function so that xxx_process will call it. After the encryption is finished the user calls omac_final and finds the omac in his buffer.

And shouldn't the omac functions be put into a separate omac.c?

Moving the scatterwalk functions seems like a good idea to me.

Well, here are the compile fixes:

diff -Nur linux.orig/crypto/cipher.c linux/crypto/cipher.c

crypto/cipher.c +5 -3
@@ -280,7 +280,7 @@
 			/* mac = Zeros */
 			memset((u8*)tfm->crt_u.cipher.cit_omac, 0, crypto_tfm_alg_blocksize(tfm));
 		}
-#endif
+#endif /* CONFIG_CRYPTO_OMAC */
 
 		return ret;
 	}

@@ -475,9 +475,12 @@
 	    	ops->cit_iv = kmalloc(ops->cit_ivsize, GFP_KERNEL);
 		if (ops->cit_iv == NULL)
 			ret = -ENOMEM;
+
+#ifdef CONFIG_CRYPTO_OMAC
 		ops->cit_omac = kmalloc(ops->cit_ivsize, GFP_KERNEL);
 		if (ops->cit_omac == NULL)
 			ret = -ENOMEM;
+#endif /* CONFIG_CRYPTO_OMAC */
 	}
 
 #ifdef CONFIG_CRYPTO_OMAC

@@ -499,5 +502,5 @@
 #ifdef CONFIG_CRYPTO_OMAC
 	if (tfm->crt_cipher.cit_omac)
 		kfree(tfm->crt_cipher.cit_omac);
-#endif
+#endif /* CONFIG_CRYPTO_OMAC */
 }
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6
author
#21 lore raw

Re: cryptoapi OMAC (was: cryptoapi highmem bug)

Am Mi, den 25.02.2004 schrieb Jean-Luc Cooke um 21:59:

Quoted 5 lines

Then apply this:
http://jlcooke.ca/lkml/crypto_omac_hmac_ctr_25feb2004.patch

This is my HMAC/OMAC/CTR patch. I think I fixed your HMAC issue. I was giving a scatterlist a stack memory reference (!).

Hmm? That's not a problem per se.

I will look at it. But I still see a lot of room for improvement especially concerning the cipher.c changes. And I don't think all of your changes to the digest code are really necessary.

author
#22 lore raw
In reply to Jean-Luc Cooke
Quoted 4 lines · Jean-Luc Cooke

Not to be annoying...

Could you make this change against my patch at:
http://jlcooke.ca/lkml/crypto_28feb2004.patch

Ok, here it is. Still working (when not using omac or hmac) after fixing the compile problems (see other mail).

If this is ok someone should split out the scatterwalk_* move from your patch and submit it and this one to Andrew so that dm-crypt doesn't go boom on highmem machines.

crypto/cipher.c +6 -6
diff -Nur linux.orig/crypto/cipher.c linux/crypto/cipher.c
@@ -72,21 +72,21 @@
 	for(;;) {
 		u8 *src_p, *dst_p;
 
-		scatterwalk_map(&walk_in, 0);
-		scatterwalk_map(&walk_out, 1);
+		scatterwalk_map(&walk_in, NULL, 0);
+		scatterwalk_map(&walk_out, &walk_in, 1);
 		src_p = scatterwalk_whichbuf(&walk_in, bsize, tmp_src);
 		dst_p = scatterwalk_whichbuf(&walk_out, bsize, tmp_dst);
 
 		nbytes -= bsize;
 
-		scatterwalk_copychunks(src_p, &walk_in, bsize, 0);
+		scatterwalk_copychunks(src_p, &walk_in, &walk_out, bsize, 0);
 
 		prfn(tfm, dst_p, src_p, crfn, enc, info);
 
-		scatterwalk_done(&walk_in, 0, nbytes);
+		scatterwalk_done(&walk_in, &walk_out, 0, nbytes);
 
-		scatterwalk_copychunks(dst_p, &walk_out, bsize, 1);
-		scatterwalk_done(&walk_out, 1, nbytes);
+		scatterwalk_copychunks(dst_p, &walk_out, NULL, bsize, 1);
+		scatterwalk_done(&walk_out, NULL, 1, nbytes);
 
 		if (!nbytes)
 			return 0;
crypto/scatterwalk.c +23 -8
diff -Nur linux.orig/crypto/scatterwalk.c linux/crypto/scatterwalk.c
@@ -63,13 +63,27 @@
 	walk->offset = sg->offset;
 }
 
-void scatterwalk_map(struct scatter_walk *walk, int out)
+void scatterwalk_map(struct scatter_walk *walk, struct scatter_walk *other,
+		     int out)
 {
-	walk->data = crypto_kmap(walk->page, out) + walk->offset;
+	if (other && other->page == walk->page) {
+		walk->data = (other->data - other->offset) + walk->offset;
+		walk->slot = other->slot;
+	} else {
+		walk->data = crypto_kmap(walk->page, out) + walk->offset;
+		walk->slot = out;
+	}
+}
+
+static void scatterwalk_unmap(struct scatter_walk *walk,
+			      struct scatter_walk *other)
+{
+	if (!other || other->page != walk->page)
+		crypto_kunmap(walk->data, walk->slot);
 }
 
 static void scatterwalk_pagedone(struct scatter_walk *walk, int out,
-			      unsigned int more)
+				 unsigned int more)
 {
 	/* walk->data may be pointing the first byte of the next page;
 	   however, we know we transfered at least one byte.  So,

@@ -92,9 +106,10 @@
 	}
 }
 
-void scatterwalk_done(struct scatter_walk *walk, int out, int more)
+void scatterwalk_done(struct scatter_walk *walk, struct scatter_walk *reuse,
+		      int out, int more)
 {
-	crypto_kunmap(walk->data, out);
+	scatterwalk_unmap(walk, reuse);
 	if (walk->len_this_page == 0 || !more)
 		scatterwalk_pagedone(walk, out, more);
 }

@@ -104,7 +119,7 @@
  * has been verified as multiple of the block size.
  */
 int scatterwalk_copychunks(void *buf, struct scatter_walk *walk,
-			size_t nbytes, int out)
+			   struct scatter_walk *reuse, size_t nbytes, int out)
 {
 	if (buf != walk->data) {
 		while (nbytes > walk->len_this_page) {

@@ -112,9 +127,9 @@
 			buf += walk->len_this_page;
 			nbytes -= walk->len_this_page;
 
-			crypto_kunmap(walk->data, out);
+			scatterwalk_unmap(walk, reuse);
 			scatterwalk_pagedone(walk, out, 1);
-			scatterwalk_map(walk, out);
+			scatterwalk_map(walk, reuse, out);
 		}
 
 		memcpy_dir(buf, walk->data, nbytes, out);
crypto/scatterwalk.h +4 -3
diff -Nur linux.orig/crypto/scatterwalk.h linux/crypto/scatterwalk.h
@@ -26,6 +26,7 @@
 	struct scatterlist	*sg;
 	struct page		*page;
 	void			*data;
+	int			slot;
 	unsigned int		len_this_page;
 	unsigned int		len_this_segment;
 	unsigned int		offset;

@@ -40,8 +41,8 @@
 
 void *scatterwalk_whichbuf(struct scatter_walk *walk, unsigned int nbytes, void *scratch);
 void scatterwalk_start(struct scatter_walk *walk, struct scatterlist *sg);
-int scatterwalk_copychunks(void *buf, struct scatter_walk *walk, size_t nbytes, int out);
-void scatterwalk_map(struct scatter_walk *walk, int out);
-void scatterwalk_done(struct scatter_walk *walk, int out, int more);
+int scatterwalk_copychunks(void *buf, struct scatter_walk *walk, struct scatter_walk *reuse, size_t nbytes, int out);
+void scatterwalk_map(struct scatter_walk *walk, struct scatter_walk *reuse, int out);
+void scatterwalk_done(struct scatter_walk *walk, struct scatter_walk *reuse, int out, int more);
 
 #endif  /* _CRYPTO_SCATTERWALK_H */
#23 lore raw
Quoted 6 lines · Christophe Saout

Ok, here it is. Still working (when not using omac or hmac) after fixing the compile problems (see other mail).

If this is ok someone should split out the scatterwalk_* move from your patch and submit it and this one to Andrew so that dm-crypt doesn't go boom on highmem machines.

Great thanks a bunch.

Here is the scatterlist+"Le Patch de Christophe":
http://jlcooke.ca/lkml/cryptowalk_christophe_25feb2004.patch

Reguarding dm-crypt:
I didn't get a response back when suggesting we store IV and MAC info for each block. Can we do this? Can I do this? Where's the source, in 2.3.6-main? I'd like to implement this IV and optional OMAC stuff for encrypted filesyetems (I assume that's that dm-crypt is replacing)

JLC

Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6
author
#24 lore raw
Quoted 2 lines · Jean-Luc Cooke

Here is the scatterlist+"Le Patch de Christophe":
http://jlcooke.ca/lkml/cryptowalk_christophe_25feb2004.patch

Andrew hat another idea that would be even shorter. I don't know if it's much cleaner though. Trying to implement it.

Quoted 3 lines

Reguarding dm-crypt:
I didn't get a response back when suggesting we store IV and MAC info for each block.

Yes, sorry, it's on my todo list (but I kept pushing it back because explaining the problems in detail would have taken a lot of time). ;)

Quoted 1 line

Can we do this?

It's very non-trivial. Think about journalling filesystems, write ordering and atomicity. If the system crashes between two write operations we must be able to still correctly read the data. And write to these "crypto info blocks" should be done in a ways that doesn't kill performance. Do you have a proposal?

It would make dm-crypt *a lot more* complicated. We need caches for the info blocks, etc...

Quoted 2 lines

Can I do this? Where's the source, in
2.3.6-main?

Which source? dm-crypt? In 2.6.3-bk and 2.6.3-mm*. Andrew's latest tree also has my first "more secure IV proposal" patch in it and I posted a (broken, racy) hmac IV patch in the other thread.

#25 lore raw
Quoted 5 lines · Christophe Saout
Quoted 2 lines

I didn't get a response back when suggesting we store IV and MAC info for each block.

Yes, sorry, it's on my todo list (but I kept pushing it back because explaining the problems in detail would have taken a lot of time). ;)

:) That's cool.

Quoted 7 lines
Quoted 1 line

Can we do this?

It's very non-trivial. Think about journalling filesystems, write ordering and atomicity. If the system crashes between two write operations we must be able to still correctly read the data. And write to these "crypto info blocks" should be done in a ways that doesn't kill performance. Do you have a proposal?

I see. From a security point of view, no. OMACs need to be updated after the data is updated to keep integrity checks passing. IVs need to be updated before the data is updated or plaintext is leaked. (IV + data + OMAC can be written to device at once).

I assume then that IVs and OMACs will not be stored in the same read-chunk as the data then? Bummer if this is the case.

Quoted 2 lines

It would make dm-crypt *a lot more* complicated. We need caches for the info blocks, etc...

Yes I see how that would be nasty. But I've never written for things this low to the device so you are likly better off without me.

Quoted 11 lines
Quoted 2 lines

Can I do this? Where's the source, in
2.3.6-main?

Which source? dm-crypt? In 2.6.3-bk and 2.6.3-mm*. Andrew's latest tree also has my first "more secure IV proposal" patch in it and I posted a (broken, racy) hmac IV patch in the other thread.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6
author
#26 lore raw
Quoted 10 lines · Jean-Luc Cooke
Quoted 7 lines
Quoted 1 line

Can we do this?

It's very non-trivial. Think about journalling filesystems, write ordering and atomicity. If the system crashes between two write operations we must be able to still correctly read the data. And write to these "crypto info blocks" should be done in a ways that doesn't kill performance. Do you have a proposal?

I see. From a security point of view, no. OMACs need to be updated after the data is updated to keep integrity checks passing.

Yes. But if the machine doesn't get to update the OMAC but the data has already been written you must be able to still read the data somehow.

Quoted 1 line

IVs need to be updated before the data is updated or plaintext is leaked.

Hmm? What could be done: The IV "sequence number" is incremented by one every time a sector gets written. The IV sequence numbers get written to the info block later (after a timeout, memory pressure and we to free some space in the cache or if the sequence has gone too far). When we read and the OMAC doesn't match we can try to increase the IV several times until it matches. Still the problem with the OMAC atomicity...

Quoted 1 line

(IV + data + OMAC can be written to device at once).

You can't guarantee that anything gets written at once. You can only make sure that something has been written. Or that something gets written before something else (using barriers, but I don't know if that is stable, it has never been used on bio level yet).

Quoted 2 lines

I assume then that IVs and OMACs will not be stored in the same read-chunk as the data then? Bummer if this is the case.

Well, we can't store it in the same sector because all 512 bytes are already used data.

We could store less than 512 bytes in a sector but that would mean splitting up data on a sub-sector level. That means we have to read some sectors with untouched data (the first and the last), update the data and write several sectors. But then we can't even guarantee that sectors are atomicically written as seen by the filesystem. This is... yuck.

#27 lore raw
Quoted 10 lines · Christophe Saout
Quoted 8 lines · Jean-Luc Cooke
Quoted 7 lines
Quoted 1 line

Can we do this?

It's very non-trivial. Think about journalling filesystems, write ordering and atomicity. If the system crashes between two write operations we must be able to still correctly read the data. And write to these "crypto info blocks" should be done in a ways that doesn't kill performance. Do you have a proposal?

Quoted 5 lines
Quoted 2 lines

I see. From a security point of view, no. OMACs need to be updated after the data is updated to keep integrity checks passing.

Yes. But if the machine doesn't get to update the OMAC but the data has already been written you must be able to still read the data somehow.

Agreed. Some magic is needed here.

Quoted 9 lines
Quoted 1 line

IVs need to be updated before the data is updated or plaintext is leaked.

Hmm? What could be done: The IV "sequence number" is incremented by one every time a sector gets written. The IV sequence numbers get written to the info block later (after a timeout, memory pressure and we to free some space in the cache or if the sequence has gone too far). When we read and the OMAC doesn't match we can try to increase the IV several times until it matches. Still the problem with the OMAC atomicity...

At mkfs, each block is given IV=HMAC(Key,blockNum). Every time data is written to the block, encrypt with IV'=IV+1 and store IV'.

Donno if I like the "if at first we don't succeed, try try again" approach. But you know better then I do how difficult caching info blocks will be.

Quoted 6 lines
Quoted 1 line

(IV + data + OMAC can be written to device at once).

You can't guarantee that anything gets written at once. You can only make sure that something has been written. Or that something gets written before something else (using barriers, but I don't know if that is stable, it has never been used on bio level yet).

Caching sounds like a good way out. Always consult the cache struct with a lock for each N-blocks in the FS to reduce bottleneck-ing and let the cache update at a sensible interval. ...or am I talking black (block?) magic of the worst kind here?

How about we set a goal for ourselves saying "we're prepared to accept X performance degradation for encrypted file systems"?

Quoted 6 lines
Quoted 2 lines

I assume then that IVs and OMACs will not be stored in the same read-chunk as the data then? Bummer if this is the case.

Well, we can't store it in the same sector because all 512 bytes are already used data.

Quoted 6 lines

We could store less than 512 bytes in a sector but that would mean splitting up data on a sub-sector level. That means we have to read some sectors with untouched data (the first and the last), update the data and write several sectors. But then we can't even guarantee that sectors are atomicically written as seen by the filesystem. This is... yuck.

The "try try again" approach maybe our only way. Yet, if someone reads data from the FS and the IV wasn't written to disk yet (or ever in the case of a crash) then incrementing will not solve anything. Screwed again.

JLC - will think about this on the bus ride home.

Signature · 4 lines
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6