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&D Institute Poland
