Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided

[Kamil Konieczny <k.konieczny@partner.samsung.com>]

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