* [QUESTION] hash import and request initialization @ 2017-12-25 8:07 Gilad Ben-Yossef 2017-12-26 3:07 ` Herbert Xu 0 siblings, 1 reply; 9+ messages in thread From: Gilad Ben-Yossef @ 2017-12-25 8:07 UTC (permalink / raw) To: Linux Crypto Mailing List, Herbert Xu Hi there, I have ran into something I am unsure about regarding the ccree driver implementation and would love to get some guidance: I have assumed, based on previous list correspondence, that both of the following sequences are legal: Scenario A 1. ahash_request_alloc 2. ahash_import Scenario B 1. ahash_request_alloc 2. crypto_ahash_init 3. ahash_import However, I've run into the problem that since ahash_request_alloc does not zero the allocated request memory and specifically does not zero the tfm specific context part of it, I have no way to distinguish between A and B and therefore if I have any initialization sequence that needs to happen once per request (DMA buffer allocation and mapping in my case), I cannot tell if I need to do them in ahash_import callback or not. The testmgr test uses scenario A, but this is nor here nor there. So, am I'm missing something here or is the B scenario simply not legal? Many thanks and Happy Xmas if this applies to you, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [QUESTION] hash import and request initialization 2017-12-25 8:07 [QUESTION] hash import and request initialization Gilad Ben-Yossef @ 2017-12-26 3:07 ` Herbert Xu 2017-12-26 9:06 ` Gilad Ben-Yossef 0 siblings, 1 reply; 9+ messages in thread From: Herbert Xu @ 2017-12-26 3:07 UTC (permalink / raw) To: Gilad Ben-Yossef; +Cc: Linux Crypto Mailing List On Mon, Dec 25, 2017 at 10:07:54AM +0200, Gilad Ben-Yossef wrote: > Hi there, > > I have ran into something I am unsure about regarding the ccree driver > implementation and would love to get some guidance: > > I have assumed, based on previous list correspondence, that both of > the following sequences are legal: > > Scenario A > 1. ahash_request_alloc > 2. ahash_import > > Scenario B > 1. ahash_request_alloc > 2. crypto_ahash_init > 3. ahash_import > > However, I've run into the problem that since ahash_request_alloc does > not zero the allocated request memory > and specifically does not zero the tfm specific context part of it, I > have no way to distinguish between A and B and therefore > if I have any initialization sequence that needs to happen once per > request (DMA buffer allocation and mapping in my > case), I cannot tell if I need to do them in ahash_import callback or not. crypto_ahash_init is not necessary prior to an ahash_import. If crypto_ahash_init does any hardware initialisations, then the same would be needed in ahash_import and crypto_ahash_digest. > So, am I'm missing something here or is the B scenario simply not legal? B is completely legal, just as it would be legal to do this: 1. ahash_request_alloc 2. crypto_ahash_init 3. crypto_ahash_update 4. crypto_ahash_init // discard previous hash state Basically you must not hold onto any permanent state at the end of a hash operation such as init. IOW you cannot expect a hash state to be finalised. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [QUESTION] hash import and request initialization 2017-12-26 3:07 ` Herbert Xu @ 2017-12-26 9:06 ` Gilad Ben-Yossef 2017-12-26 10:40 ` Herbert Xu 0 siblings, 1 reply; 9+ messages in thread From: Gilad Ben-Yossef @ 2017-12-26 9:06 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Tue, Dec 26, 2017 at 5:07 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Mon, Dec 25, 2017 at 10:07:54AM +0200, Gilad Ben-Yossef wrote: >> Hi there, >> >> I have ran into something I am unsure about regarding the ccree driver >> implementation and would love to get some guidance: >> >> I have assumed, based on previous list correspondence, that both of >> the following sequences are legal: >> >> Scenario A >> 1. ahash_request_alloc >> 2. ahash_import >> >> Scenario B >> 1. ahash_request_alloc >> 2. crypto_ahash_init >> 3. ahash_import >> >> However, I've run into the problem that since ahash_request_alloc does >> not zero the allocated request memory >> and specifically does not zero the tfm specific context part of it, I >> have no way to distinguish between A and B and therefore >> if I have any initialization sequence that needs to happen once per >> request (DMA buffer allocation and mapping in my >> case), I cannot tell if I need to do them in ahash_import callback or not. > > crypto_ahash_init is not necessary prior to an ahash_import. If > crypto_ahash_init does any hardware initialisations, then the same > would be needed in ahash_import and crypto_ahash_digest. > >> So, am I'm missing something here or is the B scenario simply not legal? > > B is completely legal, just as it would be legal to do this: > > 1. ahash_request_alloc > 2. crypto_ahash_init > 3. crypto_ahash_update > 4. crypto_ahash_init // discard previous hash state > > Basically you must not hold onto any permanent state at the end > of a hash operation such as init. IOW you cannot expect a hash > state to be finalised. Thank you. I understand. Many thanks for your assistance. I do have a follow up question, if I may: My driver does require a buffer to keep interim hash state. Ideally, I would allocate this buffer as part of the ahash request context as I assume was the intent. Indeed, this is what I've noticed the caam, atmel and chelsio drivers are doing, at least. Unfortunately, I don't understand how this is safe: The hash state needs to be DMAed in and out of the hardware on each operation and I have no guarantee that the ahash request is allocated from DMAable memory (i.e. AHASH_REQUEST_ON_STACK). The only option I see open to me is to allocate a new buffer, copy into it the interim state stored in the ahash request context, DMA map it and than reverse the operation at the end of each op. That will work, but it sounds less than ideal and since I've noticed at least some of the other drivers do not do this (caam, atmel and chelsio as mentioned), I wonder yet again if I have missed something. Thanks again! Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [QUESTION] hash import and request initialization 2017-12-26 9:06 ` Gilad Ben-Yossef @ 2017-12-26 10:40 ` Herbert Xu 2017-12-26 12:21 ` Gilad Ben-Yossef 0 siblings, 1 reply; 9+ messages in thread From: Herbert Xu @ 2017-12-26 10:40 UTC (permalink / raw) To: Gilad Ben-Yossef; +Cc: Linux Crypto Mailing List On Tue, Dec 26, 2017 at 11:06:02AM +0200, Gilad Ben-Yossef wrote: > > The hash state needs to be DMAed in and out of the hardware on each operation > and I have no guarantee that the ahash request is allocated from DMAable memory > (i.e. AHASH_REQUEST_ON_STACK). AHASH_REQUEST_ON_STACK cannot possibly be used on an async ahash implementation for obvious reasons so this shouldn't be an issue. The construct is simply a compatibility aid for sync-only users to use the ahash interface. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [QUESTION] hash import and request initialization 2017-12-26 10:40 ` Herbert Xu @ 2017-12-26 12:21 ` Gilad Ben-Yossef 2017-12-27 3:35 ` Herbert Xu 0 siblings, 1 reply; 9+ messages in thread From: Gilad Ben-Yossef @ 2017-12-26 12:21 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Tue, Dec 26, 2017 at 12:40 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, Dec 26, 2017 at 11:06:02AM +0200, Gilad Ben-Yossef wrote: >> >> The hash state needs to be DMAed in and out of the hardware on each operation >> and I have no guarantee that the ahash request is allocated from DMAable memory >> (i.e. AHASH_REQUEST_ON_STACK). > > AHASH_REQUEST_ON_STACK cannot possibly be used on an async ahash > implementation for obvious reasons so this shouldn't be an issue. > OK, you lost me on this statement. See how SKCIPHER_REQUEST_ON_STACK is being used with an asymmetric skcipher in drivers/md/dm-integrity.c The case with AHASH_REQUEST_ON_STACK looks symmetric to me. E.g. why would this dummy code be illegal - DECLARE_CRYPTO_WAIT(wait); AHASH_REQUEST_ON_STACK(req, tfm); sg_init_table(&sg, 1); sg_set_buf(&sg, buf); ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP | CRYPTO_TFM_REQ_MAY_BACKLOG, crypto_req_done, (void *)wait); ahash_request_set_crypt(req, &sg, NULL, len); rc = crypto_ahash_digest(req); /* do some stuff here, possibly stuff that can sleep */ rc = crypto_wait_req(r, wait); ... > The construct is simply a compatibility aid for sync-only users > to use the ahash interface. Please don't get me wrong, I'm not trying to argue. Simply putting the internal state in the request context is the simplest most straight forward thing for me. If you say I don't need to worry about it I'm fine with that. I just want to understand why it's OK so I can understand the system better. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [QUESTION] hash import and request initialization 2017-12-26 12:21 ` Gilad Ben-Yossef @ 2017-12-27 3:35 ` Herbert Xu 2017-12-27 6:27 ` Gilad Ben-Yossef 2018-01-10 14:32 ` [PATCH] dm-integrity: don't store cipher request on the stack (was: [QUESTION] hash import and request initialization) Mikulas Patocka 0 siblings, 2 replies; 9+ messages in thread From: Herbert Xu @ 2017-12-27 3:35 UTC (permalink / raw) To: Gilad Ben-Yossef; +Cc: Linux Crypto Mailing List, Mikulas Patocka On Tue, Dec 26, 2017 at 02:21:53PM +0200, Gilad Ben-Yossef wrote: > > See how SKCIPHER_REQUEST_ON_STACK is being used with an asymmetric skcipher > in drivers/md/dm-integrity.c That's just broken. SKCIPHER_REQUEST_ON_STACK is only meant for sync algorithms and this code needs to be changed to either do the proper request allocation or switch over to allocating sync algorithms. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [QUESTION] hash import and request initialization 2017-12-27 3:35 ` Herbert Xu @ 2017-12-27 6:27 ` Gilad Ben-Yossef 2018-01-10 14:32 ` [PATCH] dm-integrity: don't store cipher request on the stack (was: [QUESTION] hash import and request initialization) Mikulas Patocka 1 sibling, 0 replies; 9+ messages in thread From: Gilad Ben-Yossef @ 2017-12-27 6:27 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List, Mikulas Patocka On Wed, Dec 27, 2017 at 5:35 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, Dec 26, 2017 at 02:21:53PM +0200, Gilad Ben-Yossef wrote: >> >> See how SKCIPHER_REQUEST_ON_STACK is being used with an asymmetric skcipher >> in drivers/md/dm-integrity.c > > That's just broken. SKCIPHER_REQUEST_ON_STACK is only meant for > sync algorithms and this code needs to be changed to either do the > proper request allocation or switch over to allocating sync > algorithms. OK, great. Thanks for clearing that up. Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] dm-integrity: don't store cipher request on the stack (was: [QUESTION] hash import and request initialization) 2017-12-27 3:35 ` Herbert Xu 2017-12-27 6:27 ` Gilad Ben-Yossef @ 2018-01-10 14:32 ` Mikulas Patocka 2018-01-10 15:13 ` Mike Snitzer 1 sibling, 1 reply; 9+ messages in thread From: Mikulas Patocka @ 2018-01-10 14:32 UTC (permalink / raw) To: Herbert Xu, Mike Snitzer Cc: Gilad Ben-Yossef, dm-devel, Linux Crypto Mailing List On Wed, 27 Dec 2017, Herbert Xu wrote: > On Tue, Dec 26, 2017 at 02:21:53PM +0200, Gilad Ben-Yossef wrote: > > > > See how SKCIPHER_REQUEST_ON_STACK is being used with an asymmetric skcipher > > in drivers/md/dm-integrity.c > > That's just broken. SKCIPHER_REQUEST_ON_STACK is only meant for > sync algorithms and this code needs to be changed to either do the > proper request allocation or switch over to allocating sync > algorithms. > > Cheers, Hi Here I send a patch that moves those allocations to the heap. Mikulas From: Mikulas Patocka <mpatocka@redhat.com> Subject: [PATCH] dm-integrity: don't store cipher request on the stack dm-integrity: don't store cipher request on the stack Some asynchronous cipher implementations may use DMA. The stack may be mapped in the vmalloc area that doesn't support DMA. Therefore, the cipher request and initialization vector shouldn't be on the stack. This patch allocates the request and iv with kmalloc. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@vger.kernel.org $ v4.12+ Index: linux-2.6/drivers/md/dm-integrity.c =================================================================== --- linux-2.6.orig/drivers/md/dm-integrity.c +++ linux-2.6/drivers/md/dm-integrity.c @@ -2559,7 +2559,8 @@ static int create_journal(struct dm_inte int r = 0; unsigned i; __u64 journal_pages, journal_desc_size, journal_tree_size; - unsigned char *crypt_data = NULL; + unsigned char *crypt_data = NULL, *crypt_iv = NULL; + struct skcipher_request *req = NULL; ic->commit_ids[0] = cpu_to_le64(0x1111111111111111ULL); ic->commit_ids[1] = cpu_to_le64(0x2222222222222222ULL); @@ -2617,9 +2618,20 @@ static int create_journal(struct dm_inte if (blocksize == 1) { struct scatterlist *sg; - SKCIPHER_REQUEST_ON_STACK(req, ic->journal_crypt); - unsigned char iv[ivsize]; - skcipher_request_set_tfm(req, ic->journal_crypt); + + req = skcipher_request_alloc(ic->journal_crypt, GFP_KERNEL); + if (!req) { + *error = "Could not allocate crypt request"; + r = -ENOMEM; + goto bad; + } + + crypt_iv = kmalloc(ivsize, GFP_KERNEL); + if (!crypt_iv) { + *error = "Could not allocate iv"; + r = -ENOMEM; + goto bad; + } ic->journal_xor = dm_integrity_alloc_page_list(ic); if (!ic->journal_xor) { @@ -2641,9 +2653,9 @@ static int create_journal(struct dm_inte sg_set_buf(&sg[i], va, PAGE_SIZE); } sg_set_buf(&sg[i], &ic->commit_ids, sizeof ic->commit_ids); - memset(iv, 0x00, ivsize); + memset(crypt_iv, 0x00, ivsize); - skcipher_request_set_crypt(req, sg, sg, PAGE_SIZE * ic->journal_pages + sizeof ic->commit_ids, iv); + skcipher_request_set_crypt(req, sg, sg, PAGE_SIZE * ic->journal_pages + sizeof ic->commit_ids, crypt_iv); init_completion(&comp.comp); comp.in_flight = (atomic_t)ATOMIC_INIT(1); if (do_crypt(true, req, &comp)) @@ -2659,10 +2671,22 @@ static int create_journal(struct dm_inte crypto_free_skcipher(ic->journal_crypt); ic->journal_crypt = NULL; } else { - SKCIPHER_REQUEST_ON_STACK(req, ic->journal_crypt); - unsigned char iv[ivsize]; unsigned crypt_len = roundup(ivsize, blocksize); + req = skcipher_request_alloc(ic->journal_crypt, GFP_KERNEL); + if (!req) { + *error = "Could not allocate crypt request"; + r = -ENOMEM; + goto bad; + } + + crypt_iv = kmalloc(ivsize, GFP_KERNEL); + if (!crypt_iv) { + *error = "Could not allocate iv"; + r = -ENOMEM; + goto bad; + } + crypt_data = kmalloc(crypt_len, GFP_KERNEL); if (!crypt_data) { *error = "Unable to allocate crypt data"; @@ -2670,8 +2694,6 @@ static int create_journal(struct dm_inte goto bad; } - skcipher_request_set_tfm(req, ic->journal_crypt); - ic->journal_scatterlist = dm_integrity_alloc_journal_scatterlist(ic, ic->journal); if (!ic->journal_scatterlist) { *error = "Unable to allocate sg list"; @@ -2695,12 +2717,12 @@ static int create_journal(struct dm_inte struct skcipher_request *section_req; __u32 section_le = cpu_to_le32(i); - memset(iv, 0x00, ivsize); + memset(crypt_iv, 0x00, ivsize); memset(crypt_data, 0x00, crypt_len); memcpy(crypt_data, §ion_le, min((size_t)crypt_len, sizeof(section_le))); sg_init_one(&sg, crypt_data, crypt_len); - skcipher_request_set_crypt(req, &sg, &sg, crypt_len, iv); + skcipher_request_set_crypt(req, &sg, &sg, crypt_len, crypt_iv); init_completion(&comp.comp); comp.in_flight = (atomic_t)ATOMIC_INIT(1); if (do_crypt(true, req, &comp)) @@ -2758,6 +2780,9 @@ retest_commit_id: } bad: kfree(crypt_data); + kfree(crypt_iv); + skcipher_request_free(req); + return r; } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: dm-integrity: don't store cipher request on the stack (was: [QUESTION] hash import and request initialization) 2018-01-10 14:32 ` [PATCH] dm-integrity: don't store cipher request on the stack (was: [QUESTION] hash import and request initialization) Mikulas Patocka @ 2018-01-10 15:13 ` Mike Snitzer 0 siblings, 0 replies; 9+ messages in thread From: Mike Snitzer @ 2018-01-10 15:13 UTC (permalink / raw) To: Mikulas Patocka Cc: Herbert Xu, Gilad Ben-Yossef, dm-devel, Linux Crypto Mailing List On Wed, Jan 10 2018 at 9:32am -0500, Mikulas Patocka <mpatocka@redhat.com> wrote: > > > On Wed, 27 Dec 2017, Herbert Xu wrote: > > > On Tue, Dec 26, 2017 at 02:21:53PM +0200, Gilad Ben-Yossef wrote: > > > > > > See how SKCIPHER_REQUEST_ON_STACK is being used with an asymmetric skcipher > > > in drivers/md/dm-integrity.c > > > > That's just broken. SKCIPHER_REQUEST_ON_STACK is only meant for > > sync algorithms and this code needs to be changed to either do the > > proper request allocation or switch over to allocating sync > > algorithms. > > > > Cheers, > > Hi > > Here I send a patch that moves those allocations to the heap. Thanks, I'll review and hope to stage for 4.16 today. Mike ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-01-10 15:14 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-25 8:07 [QUESTION] hash import and request initialization Gilad Ben-Yossef 2017-12-26 3:07 ` Herbert Xu 2017-12-26 9:06 ` Gilad Ben-Yossef 2017-12-26 10:40 ` Herbert Xu 2017-12-26 12:21 ` Gilad Ben-Yossef 2017-12-27 3:35 ` Herbert Xu 2017-12-27 6:27 ` Gilad Ben-Yossef 2018-01-10 14:32 ` [PATCH] dm-integrity: don't store cipher request on the stack (was: [QUESTION] hash import and request initialization) Mikulas Patocka 2018-01-10 15:13 ` Mike Snitzer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.