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. >
