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
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,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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
On Tue, 6 May 2008, Mikulas Patocka wrote:
> 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
Do you think that it would make sense to add a flag to crypto API that
says that the encryption function reads the input only once? --- so that
we wouldn't have to copy the data when using an algorithm which doesn't
need it.
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?
Mikulas
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,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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.000000000 +0200
+++ linux-2.6.25.3/drivers/md/dm-crypt.c 2008-05-13 22:14:12.000000000 +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,
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,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>> 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
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 ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 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
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 ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>> 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.
If you don't set the bit when it should be set => you get slight
performance drop (problem 1)
You set the bit when it shouldn't be set => you get data corruption
(problem 2)
Problem 1 is acceptable, problem 2 is not.
So if I forgot some code path when the bit should be set, I created
problem 1, but I am sure that I didn't create problem 2.
If you invert the meaning of the bit, you risk creating problem 2. And you
need more review (by someone who understands what all structures are being
created in crypto code).
Mikulas