Hi All,
On 01.12.2017 09:11, Antoine Tenart wrote:
> Hi Herbert,
>
> On Fri, Dec 01, 2017 at 11:31:09AM +1100, Herbert Xu wrote:
>> On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote:
>>>
>>> can the driver get request for final/finup/digest with null req->result ?
>>> If yes (?), such checks can be done before any hardware processing, saving time,
>>> for example:
>>
>> This should not be possible through any user-space facing API.
>>
>> If a kernel API user did this then they're just shooting themselves
>> in the foot.
>>
>> So unless there is a valida call path that leads to this then I
>> would say that there is nothing to fix.
>
> I agree this should not be the case.
>
> But:
> - Other drivers are doing this check (grep "if (!req->result)" or
> "if (req->result)" to see some of them).
> - I see at least one commit fixing the exact same issue I'm facing here,
> 393897c5156a415533ff85aa381458840417b032:
>
> crypto: ccp - Check for caller result area before using it
>
> For a hash operation, the caller doesn't have to supply a result
> area on every call so don't use it / update it if it hasn't
> been supplied.
Herbert, is it possible for every init/update that areq->result can be NULL,
and only for final/update/digit user set it to actual memory ?
testmgr.c can check if hash update writes into areq->result and if yes,
then test fails ?
As I understand this, when crypto api user allocates ahash_request,
crypto allocates memory for itself _plus_ for driver's context. This allocated
ahash_request is "handle" for all subsequent updates/export/import,
and for last final/finup, so I do not need to copy hash state into areq->result,
but keep it whole time in context, in your code in sreq:
struct safexcel_ahash_req *sreq = ahash_request_ctx(areq);
so here sreq is async hash request context.
Do you set last_req true for digest/finup/final ? If yes,
then you need to copy result only when it is true,
if (sreq->last_req) {
result_sz = crypto_ahash_digestsize(ahash);
memcpy(sreq->state, areq->result, result_sz);
}
I do not read all your code though, so I can be wrong here.
> I'm not entirely sure what was the code path that leads to this, I'll
> reproduce the issue and try to understand what is going on (I clearly
> recall having this crash though).
>
> The crypto API does not enforce this somehow, and this should probably
> be fixed. That might break some users. But it was seen as a valid use
> for some, so we should probably fix this in previous versions of the
> driver anyway.
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
