Re: [dm-devel] Desynchronizing dm-raid1
On Thu, May 22, 2008 at 08:32:45AM -0400, Mikulas Patocka wrote: There may be external modules. Sorry but we don't support external modules. They should be merged upstream rather than distributed in the wild. Cheers, If you want to negate the meaning of the flag, then you have to write it yourself. I, as non-developer of crypto code, can prove that on given path the input data are read only once --- but I can't prove that on all paths and all possible chaining modes of algorithms the data are read once, because I don't know about all of them. Mikulas -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [dm-devel] Desynchronizing dm-raid1
On Fri, May 23, 2008 at 10:59:33AM -0400, Mikulas Patocka wrote:
If you want to negate the meaning of the flag, then you have to write it
yourself. I, as non-developer of crypto code, can prove that on given path
the input data are read only once --- but I can't prove that on all paths
and all possible chaining modes of algorithms the data are read once,
because I don't know about all of them.
Huh? Inverting it would give exactly the same result as your current
patch. If you're not confident with it inverted, then I can't see
how you could be confident about the patch as it is.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [dm-devel] Desynchronizing dm-raid1
All the ciphers comply, so the bug is only a theroretical issue (but I didn't check assembler versions --- they should be checked by the person who wrote them, assembler is write-only language). Since every current algorithm sets the flag could you invert its sense? Sorry to have to do this to you :) Thanks, There may be external modules. If you don't set the flag when it should be set, nothing happens (just a slight performance drop), if you set the flag when it shouldn't be set, you get data corruption. So the safest way is this meaning of flag, so that not-yet-reviewed algorithms set the flag to 0 and prevent data corruption. Mikulas -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [dm-devel] Desynchronizing dm-raid1
On Thu, May 22, 2008 at 08:32:45AM -0400, Mikulas Patocka wrote:
There may be external modules.
Sorry but we don't support external modules. They should be merged
upstream rather than distributed in the wild.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [dm-devel] Desynchronizing dm-raid1
On Wed, 14 May 2008, Herbert Xu wrote:
On Tue, May 13, 2008 at 04:35:03PM -0400, Mikulas Patocka wrote:
And where would you propose to place this bit?
One possibility would be struct crypto_tfm-crt_flags
Another possibility is struct crypto_alg-cra_flags
The latter definitely because this is an algorithm property.
Can chaining mode change the value of the flag? (I presume that yes)
If you mean templates like CBC then it depends. You should
set it to zero by default for safety but most of them should
be able to turn it on once audited.
If it turns out that the majority of algorithms support this,
you could even decide to only select those algorithms that do.
Suppose your bit is
CRYPTO_ALG_FOO
then you could do
crypto_alloc_blkcipher(name, CRYPTO_ALG_FOO, CRYPTO_ALG_FOO)
to demand only those algorithms that comply.
Cheers,
Hi
Here I send the patches, the first one copied data in dm-crypt
unconditionally. The second one adds a flag to coplying algorithms. The
third one skips the copy for complying algorithms. The fourth one is
removing useless increment in arc4 that I found while reviewing the
ciphers.
All the ciphers comply, so the bug is only a theroretical issue (but I
didn't check assembler versions --- they should be checked by the person
who wrote them, assembler is write-only language).
Please review my changes to crypto code, I am not crypto developer and I
do not understand it as well as people maintaining it.
MikulasCopy data when writing to the device.
This patch fixes damaged blocks on encrypted device when the computer crashes.
Linux IO architecture allows the data to be modified while they are being
written.
While we are processing write requests, the data may change, and it is expected,
that each byte written to the device is either old byte or new byte.
Some crypto functions may read the input buffer more than once and so they'll
produce garbage if the buffer changes under them. When this garbage is
read again and decrypted, it may produce damage in all bytes in the block. This
damage is visible only if the computer crashes; if it didn't crash, the memory
manager writes correct buffer or page contents some times later.
So we first copy data to a temporary buffer with memcpy and then encrypt them
in place.
Signed-off-by: Mikulas Patocka [EMAIL PROTECTED]
---
drivers/md/dm-crypt.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
Index: linux-2.6.25.3/drivers/md/dm-crypt.c
===
--- linux-2.6.25.3.orig/drivers/md/dm-crypt.c 2008-05-14 18:41:10.0
+0200
+++ linux-2.6.25.3/drivers/md/dm-crypt.c2008-05-14 18:41:11.0
+0200
@@ -361,8 +361,18 @@ static int crypt_convert_block(struct cr
crypto_ablkcipher_alignmask(cc-tfm) + 1);
sg_init_table(dmreq-sg_in, 1);
- sg_set_page(dmreq-sg_in, bv_in-bv_page, 1 SECTOR_SHIFT,
- bv_in-bv_offset + ctx-offset_in);
+ if (bio_data_dir(ctx-bio_in) == WRITE) {
+ char *page_in = kmap_atomic(bv_in-bv_page, KM_USER0);
+ char *page_out = kmap_atomic(bv_out-bv_page, KM_USER1);
+ memcpy(page_out + bv_out-bv_offset + ctx-offset_out, page_in
+ bv_in-bv_offset + ctx-offset_in, 1 SECTOR_SHIFT);
+ kunmap_atomic(page_in, KM_USER0);
+ kunmap_atomic(page_out, KM_USER1);
+
+ sg_set_page(dmreq-sg_in, bv_out-bv_page, 1 SECTOR_SHIFT,
+ bv_out-bv_offset + ctx-offset_out);
+ } else
+ sg_set_page(dmreq-sg_in, bv_in-bv_page, 1 SECTOR_SHIFT,
+ bv_in-bv_offset + ctx-offset_in);
sg_init_table(dmreq-sg_out, 1);
sg_set_page(dmreq-sg_out, bv_out-bv_page, 1 SECTOR_SHIFT,
Add a flag that tells that the algorithm reads the data only once when
encrypting.
It will be used by dm-crypt to skip a copy operation if not needed.
Signed-off-by: Mikulas Patocka [EMAIL PROTECTED]
---
crypto/aes_generic.c |2 +-
crypto/anubis.c |2 +-
crypto/arc4.c|2 +-
crypto/blkcipher.c |2 +-
crypto/blowfish.c|2 +-
crypto/camellia.c|2 +-
crypto/cast5.c |2 +-
crypto/cast6.c |2 +-
crypto/cbc.c |2 +-
crypto/cryptd.c |2 +-
crypto/crypto_null.c |2 +-
crypto/ctr.c |2 +-
crypto/des_generic.c |4 ++--
crypto/ecb.c |2 +-
crypto/fcrypt.c |2 +-
crypto/khazad.c |2 +-
crypto/lrw.c |2 +-
crypto/pcbc.c|2 +-
crypto/salsa20_generic.c |2 +-
crypto/seed.c|2 +-
crypto/serpent.c |4 ++--
crypto/tea.c |4 ++--
crypto/twofish.c |2 +-
crypto/xts.c |2 +-
include/linux/crypto.h |9 +
25 files changed,
Re: [dm-devel] Desynchronizing dm-raid1
On Wed, May 21, 2008 at 10:18:43PM -0400, Mikulas Patocka wrote:
All the ciphers comply, so the bug is only a theroretical issue (but I
didn't check assembler versions --- they should be checked by the person
who wrote them, assembler is write-only language).
Since every current algorithm sets the flag could you invert
its sense? Sorry to have to do this to you :)
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [dm-devel] Desynchronizing dm-raid1
On Tue, 13 May 2008, Herbert Xu wrote:
On Mon, May 12, 2008 at 11:28:44PM -0400, Mikulas Patocka wrote:
Or do you thint it would be useless (all major disk-encryption algorithms
read input buffer more times?) or it would create too much code
complexity?
I'm open to this approach. As long as you do the work and come up
with the patch that is :)
Cheers,
And where would you propose to place this bit?
One possibility would be struct crypto_tfm-crt_flags
Another possibility is struct crypto_alg-cra_flags
Can chaining mode change the value of the flag? (I presume that yes)
So make a bit in struct crypto_alg. And propagate it to
crypto_alg-cra_flags of chaining modes that read input data only once?
I'm not familiar with Linux crypto API, so the location of the flag should
be selected by someone who knows it well.
Here I'm sending the patch for dm-crypt. To optimize it further, you
should add the test after bio_data_dir(ctx-bio_in) == WRITE test --- i.e.
if the algorithm is safe to read input only once, the smaller and faster
branch can be executed.
Mikukas
---
Copy data when writing to the device.
This patch fixes damaged blocks on encrypted device when the computer crashes.
Linux IO architecture allows the data to be modified while they are being
written.
While we are processing write requests, the data may change, and it is expected,
that each byte written to the device is either old byte or new byte.
Some crypto functions may read the input buffer more than once and so they'll
produce garbage if the buffer changes under them. When this garbage is
read again and decrypted, it may produce damage in all bytes in the block. This
damage is visible only if the computer crashes; if it didn't crash, the memory
manager writes correct buffer or page contents some times later.
So we first copy data to a temporary buffer with memcpy and then encrypt them
in place.
Mikulas Patocka [EMAIL PROTECTED]
---
drivers/md/dm-crypt.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
Index: linux-2.6.25.3/drivers/md/dm-crypt.c
===
--- linux-2.6.25.3.orig/drivers/md/dm-crypt.c 2008-05-13 21:48:32.0
+0200
+++ linux-2.6.25.3/drivers/md/dm-crypt.c2008-05-13 22:14:12.0
+0200
@@ -360,8 +360,18 @@ static int crypt_convert_block(struct cr
crypto_ablkcipher_alignmask(cc-tfm) + 1);
sg_init_table(dmreq-sg_in, 1);
- sg_set_page(dmreq-sg_in, bv_in-bv_page, 1 SECTOR_SHIFT,
- bv_in-bv_offset + ctx-offset_in);
+ if (bio_data_dir(ctx-bio_in) == WRITE) {
+ char *page_in = kmap_atomic(bv_in-bv_page, KM_USER0);
+ char *page_out = kmap_atomic(bv_out-bv_page, KM_USER1);
+ memcpy(page_out + bv_out-bv_offset + ctx-offset_out, page_in +
bv_in-bv_offset + ctx-offset_in, 1 SECTOR_SHIFT);
+ kunmap_atomic(page_in, KM_USER0);
+ kunmap_atomic(page_out, KM_USER1);
+
+ sg_set_page(dmreq-sg_in, bv_out-bv_page, 1 SECTOR_SHIFT,
+ bv_out-bv_offset + ctx-offset_out);
+ } else
+ sg_set_page(dmreq-sg_in, bv_in-bv_page, 1 SECTOR_SHIFT,
+ bv_in-bv_offset + ctx-offset_in);
sg_init_table(dmreq-sg_out, 1);
sg_set_page(dmreq-sg_out, bv_out-bv_page, 1 SECTOR_SHIFT,
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [dm-devel] Desynchronizing dm-raid1
On Tue, 6 May 2008, Herbert Xu wrote: Mikulas Patocka [EMAIL PROTECTED] wrote: BTW: is it guaranteed for crypto functions that they read the source data only once? There is no such guarantee. Cheers, So we'll have to copy the sector to temporary space before encrypting it. I'll look at it. Mikulas -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [dm-devel] Desynchronizing dm-raid1
On Mon, 7 Apr 2008, Martin K. Petersen wrote: Malahal == malahal [EMAIL PROTECTED] writes: [I sent this last week but it never made it to the list] Malahal Very few drivers require it, so how about an interface to Malahal lock the pages of an I/O available to drivers. Only needed Malahal RAID drivers would lock the I/O while it is in progress and Malahal they only pay the performance penalty. mmap pages are a bit Malahal tricky. They need to go into read-only mode when an I/O is in Malahal progress. I know this would likely be rejected too!!! I have exactly the same problem with the data integrity stuff I'm working on. Essentially a checksum gets generated when a bio is submitted, and both the I/O controller and the disk drive verify the checksum. With ext2 in particular I often experience that the page (usually containing directory metadata) has been modified before the controller does the DMA. And the I/O will then be rejected by the controller or drive because the checksum doesn't match the data. So this problem is not specific to DM/MD... -- Martin K. Petersen Oracle Linux Engineering BTW: is it guaranteed for crypto functions that they read the source data only once? I.e. if you encrypt a block while another CPU modifies it, and then decrypt it, is it guaranteed that each byte of the result will be either old byte or new byte of the original data? If not, we have a subtle case of disk corruption here too. (imagine that you update for example atime field in inode while the block is being written and the crypto interface will trash the inode block). Mikulas -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
