Re: Why are we testing an intermediate result in ahash?

[Herbert Xu]

On Mon, Mar 05, 2018 at 02:30:57PM -0600, Gary R Hook wrote:
>
> So a failure to communicate a newly-required behavior change, much less
> document it, is addressed by breaking drivers?  For some reason this seems
> wrong.
> 
> It's also wrong to change the documentation after the fact, and after
> surprising people. The documentation change should have gone in with the
> change to testmgr. And all that -after- doing as suggested below.

You should never touch req->result unless you're finalising the hash.
This has always been the case, it's just never been tested by the
test frame work and therefore some buggy drivers have slipped
through.

The generic implementation (e.g., crypto/sha1_generic.c) is always
the authoritative source when it comes to driver behaviour.

Anyway, rather than wasting time debating this, you could've just
spent a couple of minutes fixing your driver.

---8<---
Subject: crypto: ccp - Do not touch result buffer except on final

The req->final buffer should only be modified when writing the
final hash.  It is not meant to be used to write incomplete hashes
when performing an update operation.

Fixes: 466d7b9f6175 ("crypto: testmgr - test misuse of result in...")
Signed-off-by: Herbert Xu 

diff --git a/drivers/crypto/ccp/ccp-crypto-aes-cmac.c 
b/drivers/crypto/ccp/ccp-crypto-aes-cmac.c
index 60fc0fa..acc8c64 100644
--- a/drivers/crypto/ccp/ccp-crypto-aes-cmac.c
+++ b/drivers/crypto/ccp/ccp-crypto-aes-cmac.c
@@ -45,8 +45,8 @@ static int ccp_aes_cmac_complete(struct crypto_async_request 
*async_req,
rctx->buf_count = 0;
}
 
-   /* Update result area if supplied */
-   if (req->result)
+   /* Update result area if final */
+   if (rctx->final)
memcpy(req->result, rctx->iv, digest_size);
 
 e_free:
diff --git a/drivers/crypto/ccp/ccp-crypto-sha.c 
b/drivers/crypto/ccp/ccp-crypto-sha.c
index 8b9b16d..e290836 100644
--- a/drivers/crypto/ccp/ccp-crypto-sha.c
+++ b/drivers/crypto/ccp/ccp-crypto-sha.c
@@ -46,8 +46,8 @@ static int ccp_sha_complete(struct crypto_async_request 
*async_req, int ret)
rctx->buf_count = 0;
}
 
-   /* Update result area if supplied */
-   if (req->result)
+   /* Update result area if final. */
+   if (rctx->final)
memcpy(req->result, rctx->ctx, digest_size);
 
 e_free:
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Why are we testing an intermediate result in ahash?

[Gary R Hook]

On 03/05/2018 12:31 PM, Kamil Konieczny wrote:



On 05.03.2018 18:47, Gary R Hook wrote:

On 03/05/2018 03:57 AM, Kamil Konieczny wrote:



On 02.03.2018 22:11, Gary R Hook wrote:

Commit 466d7b9f6 (cryptodev-2.6) added code to testmgr to populate, for async 
hash operations,
the result buffer with a known value and to test the buffer against that value 
at intermediate
steps. If the result buffer changes the operation is failed.

My question is: why?

What problem does this solve? Has this requirement existed all along, or is it 
new?

I'm now seeing complaints for AES/CMAC and SHA in my driver. I have no problem 
updating the driver,
of course, but I'd like to better understand the precipitating issue for the 
commit.

Mar  2 12:30:56 sosxen2 kernel: [   60.919198] alg: No test for cfb(aes) 
(cfb-aes-ccp)
Mar  2 12:30:56 sosxen2 kernel: [   60.924787] 367: alg: hash: update failed on 
test 3 for cmac-aes-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.946571] 367: alg: hash: update failed on 
test 4 for sha1-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.956461] 367: alg: hash: update failed on 
test 1 for hmac-sha1-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.966117] 367: alg: hash: update failed on 
test 4 for sha224-ccp: used req->result


ahash req->result can be used in digit, final and finup hash operations.
It should not be used in init and update (nor in export and import).


Where is this documented, please? I'm not seeing it in Documentation/crypto. Of 
course, I could be looking for the wrong thing.


It was recent addition, and you are right, the doc needs update.


So a failure to communicate a newly-required behavior change, much less 
document it, is addressed by breaking drivers?  For some reason this 
seems wrong.


It's also wrong to change the documentation after the fact, and after 
surprising people. The documentation change should have gone in with the 
change to testmgr. And all that -after- doing as suggested below.



There were some bugs in past, when drivers try to use req->result
as theirs temporary storage.
The bug comes up in some scenarios when caller reused ahash request
and leaves in req->result undefined value, it can be NULL or container_of(NULL)
or whatever was on stack



As I mention in my other post, our driver vets the pointer before dereference.


Problem will not happen with NULL, but only because there is code like 
(pseudocode)
in ahash_update:

1: if (req->result == NULL)
2:   // use as temp memory ahash request context
3: else
4:  // use as temp memory req->result

The point is - if we need temporary storage for keeping some state between 
updates,
we should use ahash request context, and get rid of the code lines 1,3,4


We are not using the buffer for temporary storage, we are returning an 
intermediate result. Which, as far as we can tell, is in no way a problem.


IOW I still fail to see how this is an issue for our driver. All I see 
is a change that impacts us yet isn't relevant.



The line 4 can bite us when req->result will be neither NULL nor valid memory 
pointer.
The second argument is, why we bother to check with 1: when we can should 
already do 2: only


And I don't have a problem with a clear definition of what should and should 
not happen to
buffers offered by a caller. I simply want to know where this behavior is 
defined, and is it a change from the past?


This come up after some code review.


I'd like to suggest that if a change in the framework is required that 
the change is communicated to the maintainers, along with the expected 
impact of that change, and that a discussion takes place proactively. 
Not after getting surprised by driver breakage and a patch out of left 
field.


All IMO, of course.

Re: Why are we testing an intermediate result in ahash?

[Tom Lendacky]

On 3/5/2018 12:31 PM, Kamil Konieczny wrote:
> 
> 
> On 05.03.2018 18:47, Gary R Hook wrote:
>> On 03/05/2018 03:57 AM, Kamil Konieczny wrote:
>>>
>>>
>>> On 02.03.2018 22:11, Gary R Hook wrote:
 Commit 466d7b9f6 (cryptodev-2.6) added code to testmgr to populate, for 
 async hash operations,
 the result buffer with a known value and to test the buffer against that 
 value at intermediate
 steps. If the result buffer changes the operation is failed.

 My question is: why?

 What problem does this solve? Has this requirement existed all along, or 
 is it new?

 I'm now seeing complaints for AES/CMAC and SHA in my driver. I have no 
 problem updating the driver,
 of course, but I'd like to better understand the precipitating issue for 
 the commit.

 Mar  2 12:30:56 sosxen2 kernel: [   60.919198] alg: No test for cfb(aes) 
 (cfb-aes-ccp)
 Mar  2 12:30:56 sosxen2 kernel: [   60.924787] 367: alg: hash: update 
 failed on test 3 for cmac-aes-ccp: used req->result
 Mar  2 12:30:56 sosxen2 kernel: [   60.946571] 367: alg: hash: update 
 failed on test 4 for sha1-ccp: used req->result
 Mar  2 12:30:56 sosxen2 kernel: [   60.956461] 367: alg: hash: update 
 failed on test 1 for hmac-sha1-ccp: used req->result
 Mar  2 12:30:56 sosxen2 kernel: [   60.966117] 367: alg: hash: update 
 failed on test 4 for sha224-ccp: used req->result
>>>
>>> ahash req->result can be used in digit, final and finup hash operations.
>>> It should not be used in init and update (nor in export and import).
>>
>> Where is this documented, please? I'm not seeing it in Documentation/crypto. 
>> Of course, I could be looking for the wrong thing.
> 
> It was recent addition, and you are right, the doc needs update.
> 
>>> There were some bugs in past, when drivers try to use req->result
>>> as theirs temporary storage.
>>> The bug comes up in some scenarios when caller reused ahash request
>>> and leaves in req->result undefined value, it can be NULL or 
>>> container_of(NULL)
>>> or whatever was on stack
>>>
>>
>> As I mention in my other post, our driver vets the pointer before 
>> dereference. 
> 
> Problem will not happen with NULL, but only because there is code like 
> (pseudocode)
> in ahash_update:
> 
> 1: if (req->result == NULL)
> 2:   // use as temp memory ahash request context
> 3: else
> 4:  // use as temp memory req->result
> 
> The point is - if we need temporary storage for keeping some state between 
> updates,

Maybe other drivers are using req->result for temporary storage, but the
CCP driver isn't using this as temporary storage.  It was assumed that if
req->result is non-zero then the caller wanted the intermediate result of
the hash operation.  In that case, the hash value up to that point is
copied to the caller's buffer.

> we should use ahash request context, and get rid of the code lines 1,3,4

Which the CCP driver does.  All storage needed by the driver is part of
the request context.

> 
> The line 4 can bite us when req->result will be neither NULL nor valid memory 
> pointer.

That sounds like a caller problem to me.  Similar to someone allocating
memory and passing it to a function without clearing it.  If the caller
has left garbage in it, it's the caller's issue.

> The second argument is, why we bother to check with 1: when we can should 
> already do 2: only

As mentioned above, the driver already is doing the proper thing by
allocating space in the request context.  It isn't using req->result as
temporary storage, just returning the intermediate result of the hash
operation back to the caller.

If the requirements are now to only use req->result for a final operation,
that's ok, and it's simple enough for the CCP driver to be updated to do
that.

In the future, I would expect that any change that can impact a driver
like this (testmgr failures where there were previously none) should be
better communicated to the crypto driver maintainers so they can be a bit
more proactive in making any required changes.

Thanks,
Tom

> 
>> And I don't have a problem with a clear definition of what should and should 
>> not happen to 
>> buffers offered by a caller. I simply want to know where this behavior is 
>> defined, and is it a change from the past?
> 
> This come up after some code review.
> 

Re: Why are we testing an intermediate result in ahash?

[Kamil Konieczny]

On 05.03.2018 18:47, Gary R Hook wrote:
> On 03/05/2018 03:57 AM, Kamil Konieczny wrote:
>>
>>
>> On 02.03.2018 22:11, Gary R Hook wrote:
>>> Commit 466d7b9f6 (cryptodev-2.6) added code to testmgr to populate, for 
>>> async hash operations,
>>> the result buffer with a known value and to test the buffer against that 
>>> value at intermediate
>>> steps. If the result buffer changes the operation is failed.
>>>
>>> My question is: why?
>>>
>>> What problem does this solve? Has this requirement existed all along, or is 
>>> it new?
>>>
>>> I'm now seeing complaints for AES/CMAC and SHA in my driver. I have no 
>>> problem updating the driver,
>>> of course, but I'd like to better understand the precipitating issue for 
>>> the commit.
>>>
>>> Mar  2 12:30:56 sosxen2 kernel: [   60.919198] alg: No test for cfb(aes) 
>>> (cfb-aes-ccp)
>>> Mar  2 12:30:56 sosxen2 kernel: [   60.924787] 367: alg: hash: update 
>>> failed on test 3 for cmac-aes-ccp: used req->result
>>> Mar  2 12:30:56 sosxen2 kernel: [   60.946571] 367: alg: hash: update 
>>> failed on test 4 for sha1-ccp: used req->result
>>> Mar  2 12:30:56 sosxen2 kernel: [   60.956461] 367: alg: hash: update 
>>> failed on test 1 for hmac-sha1-ccp: used req->result
>>> Mar  2 12:30:56 sosxen2 kernel: [   60.966117] 367: alg: hash: update 
>>> failed on test 4 for sha224-ccp: used req->result
>>
>> ahash req->result can be used in digit, final and finup hash operations.
>> It should not be used in init and update (nor in export and import).
> 
> Where is this documented, please? I'm not seeing it in Documentation/crypto. 
> Of course, I could be looking for the wrong thing.

It was recent addition, and you are right, the doc needs update.

>> There were some bugs in past, when drivers try to use req->result
>> as theirs temporary storage.
>> The bug comes up in some scenarios when caller reused ahash request
>> and leaves in req->result undefined value, it can be NULL or 
>> container_of(NULL)
>> or whatever was on stack
>>
> 
> As I mention in my other post, our driver vets the pointer before 
> dereference. 

Problem will not happen with NULL, but only because there is code like 
(pseudocode)
in ahash_update:

1: if (req->result == NULL)
2:   // use as temp memory ahash request context
3: else
4:  // use as temp memory req->result

The point is - if we need temporary storage for keeping some state between 
updates,
we should use ahash request context, and get rid of the code lines 1,3,4

The line 4 can bite us when req->result will be neither NULL nor valid memory 
pointer.
The second argument is, why we bother to check with 1: when we can should 
already do 2: only

> And I don't have a problem with a clear definition of what should and should 
> not happen to 
> buffers offered by a caller. I simply want to know where this behavior is 
> defined, and is it a change from the past?

This come up after some code review.

-- 
Best regards,
Kamil Konieczny
Samsung R Institute Poland

Re: Why are we testing an intermediate result in ahash?

[Gary R Hook]

On 03/05/2018 03:57 AM, Kamil Konieczny wrote:



On 02.03.2018 22:11, Gary R Hook wrote:

Commit 466d7b9f6 (cryptodev-2.6) added code to testmgr to populate, for async 
hash operations,
the result buffer with a known value and to test the buffer against that value 
at intermediate
steps. If the result buffer changes the operation is failed.

My question is: why?

What problem does this solve? Has this requirement existed all along, or is it 
new?

I'm now seeing complaints for AES/CMAC and SHA in my driver. I have no problem 
updating the driver,
of course, but I'd like to better understand the precipitating issue for the 
commit.

Mar  2 12:30:56 sosxen2 kernel: [   60.919198] alg: No test for cfb(aes) 
(cfb-aes-ccp)
Mar  2 12:30:56 sosxen2 kernel: [   60.924787] 367: alg: hash: update failed on 
test 3 for cmac-aes-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.946571] 367: alg: hash: update failed on 
test 4 for sha1-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.956461] 367: alg: hash: update failed on 
test 1 for hmac-sha1-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.966117] 367: alg: hash: update failed on 
test 4 for sha224-ccp: used req->result


ahash req->result can be used in digit, final and finup hash operations.
It should not be used in init and update (nor in export and import).


Where is this documented, please? I'm not seeing it in 
Documentation/crypto. Of course, I could be looking for the wrong thing.




There were some bugs in past, when drivers try to use req->result
as theirs temporary storage.
The bug comes up in some scenarios when caller reused ahash request
and leaves in req->result undefined value, it can be NULL or container_of(NULL)
or whatever was on stack



As I mention in my other post, our driver vets the pointer before 
dereference. And I don't have a problem with a clear definition of what 
should and should not happen to buffers offered by a caller. I simply 
want to know where this behavior is defined, and is it a change from the 
past?

Re: Why are we testing an intermediate result in ahash?

[Gary R Hook]

On 03/05/2018 03:50 AM, Herbert Xu wrote:

On Fri, Mar 02, 2018 at 03:11:52PM -0600, Gary R Hook wrote:

Commit 466d7b9f6 (cryptodev-2.6) added code to testmgr to populate, for
async hash operations, the result buffer with a known value and to test the
buffer against that value at intermediate steps. If the result buffer
changes the operation is failed.

My question is: why?


The problem is that you must not touch the result buffer unless
you're doing a finalisation.  Indeed, the caller may have provided
a NULL pointer for what it's worth.

Cheers,



Good thing I'm not afraid of appearing dense.

Where is this documented? Has this requirement always been thus, and 
only the recent patch is testing for it? -Why- mustn't we touch that buffer?


As for a NULL pointer, our driver checks for that, so that's not an 
issue. Although I get your point.

Re: Why are we testing an intermediate result in ahash?

[Kamil Konieczny]

On 02.03.2018 22:11, Gary R Hook wrote:
> Commit 466d7b9f6 (cryptodev-2.6) added code to testmgr to populate, for async 
> hash operations, 
> the result buffer with a known value and to test the buffer against that 
> value at intermediate
> steps. If the result buffer changes the operation is failed.
> 
> My question is: why?
> 
> What problem does this solve? Has this requirement existed all along, or is 
> it new?
> 
> I'm now seeing complaints for AES/CMAC and SHA in my driver. I have no 
> problem updating the driver,
> of course, but I'd like to better understand the precipitating issue for the 
> commit.
> 
> Mar  2 12:30:56 sosxen2 kernel: [   60.919198] alg: No test for cfb(aes) 
> (cfb-aes-ccp)
> Mar  2 12:30:56 sosxen2 kernel: [   60.924787] 367: alg: hash: update failed 
> on test 3 for cmac-aes-ccp: used req->result
> Mar  2 12:30:56 sosxen2 kernel: [   60.946571] 367: alg: hash: update failed 
> on test 4 for sha1-ccp: used req->result
> Mar  2 12:30:56 sosxen2 kernel: [   60.956461] 367: alg: hash: update failed 
> on test 1 for hmac-sha1-ccp: used req->result
> Mar  2 12:30:56 sosxen2 kernel: [   60.966117] 367: alg: hash: update failed 
> on test 4 for sha224-ccp: used req->result

ahash req->result can be used in digit, final and finup hash operations.
It should not be used in init and update (nor in export and import).

There were some bugs in past, when drivers try to use req->result
as theirs temporary storage.
The bug comes up in some scenarios when caller reused ahash request
and leaves in req->result undefined value, it can be NULL or container_of(NULL)
or whatever was on stack

-- 
Best regards,
Kamil Konieczny
Samsung R Institute Poland

Re: Why are we testing an intermediate result in ahash?

[Herbert Xu]

On Fri, Mar 02, 2018 at 03:11:52PM -0600, Gary R Hook wrote:
> Commit 466d7b9f6 (cryptodev-2.6) added code to testmgr to populate, for
> async hash operations, the result buffer with a known value and to test the
> buffer against that value at intermediate steps. If the result buffer
> changes the operation is failed.
> 
> My question is: why?

The problem is that you must not touch the result buffer unless
you're doing a finalisation.  Indeed, the caller may have provided
a NULL pointer for what it's worth.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Why are we testing an intermediate result in ahash?

[Gary R Hook]

Commit 466d7b9f6 (cryptodev-2.6) added code to testmgr to populate, for 
async hash operations, the result buffer with a known value and to test 
the buffer against that value at intermediate steps. If the result 
buffer changes the operation is failed.


My question is: why?

What problem does this solve? Has this requirement existed all along, or 
is it new?


I'm now seeing complaints for AES/CMAC and SHA in my driver. I have no 
problem updating the driver, of course, but I'd like to better 
understand the precipitating issue for the commit.


Mar  2 12:30:56 sosxen2 kernel: [   60.919198] alg: No test for cfb(aes) 
(cfb-aes-ccp)
Mar  2 12:30:56 sosxen2 kernel: [   60.924787] 367: alg: hash: update 
failed on test 3 for cmac-aes-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.946571] 367: alg: hash: update 
failed on test 4 for sha1-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.956461] 367: alg: hash: update 
failed on test 1 for hmac-sha1-ccp: used req->result
Mar  2 12:30:56 sosxen2 kernel: [   60.966117] 367: alg: hash: update 
failed on test 4 for sha224-ccp: used req->result

...

Thanks,
Gary