[QUESTION] hash import and request initialization

[QUESTION] hash import and request initialization

[Gilad Ben-Yossef]

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

Re: [QUESTION] hash import and request initialization

[Herbert Xu]

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

Re: [QUESTION] hash import and request initialization

[Gilad Ben-Yossef]

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

Re: [QUESTION] hash import and request initialization

[Herbert Xu]

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

Re: [QUESTION] hash import and request initialization

[Gilad Ben-Yossef]

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

Re: [QUESTION] hash import and request initialization

[Herbert Xu]

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

Re: [QUESTION] hash import and request initialization

[Gilad Ben-Yossef]

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

[PATCH] dm-integrity: don't store cipher request on the stack (was: [QUESTION] hash import and request initialization)

[Mikulas Patocka]

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, &section_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;
 }
 

Re: dm-integrity: don't store cipher request on the stack (was: [QUESTION] hash import and request initialization)

[Mike Snitzer]

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