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.
