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

[Antoine Tenart <antoine.tenart@free-electrons.com>]

Hi Herbert,

On Mon, Dec 11, 2017 at 06:29:46PM +1100, Herbert Xu wrote:
> On Tue, Nov 28, 2017 at 10:05:17AM +0100, Antoine Tenart wrote:
> >  
> >  	if (sreq->finish)
> >  		result_sz = crypto_ahash_digestsize(ahash);
> > -	memcpy(sreq->state, areq->result, result_sz);
> > +
> > +	/* The called isn't required to supply a result buffer. Updated it only
> > +	 * when provided.
> > +	 */
> > +	if (areq->result)
> > +		memcpy(sreq->state, areq->result, result_sz);
> 
> I don't think you should use whether areq->result is NULL to
> determine whether to copy data into it.  For example, I could
> be making an update call with a non-NULL areq->result and it would
> be completely wrong if you overwrote that with the above memcpy.
> 
> IOW areq->result should not be touched at all unless you are doing
> a finalisation.

I didn't know that. It means the SafeXcel driver logic is wrong
regarding this point, as areq->result is DMA mapped and used of all hash
operations (including updates).

So this patch is indeed fixing an issue, which should probably not be
there in the first place. I guess you recommend using a buffer local to
the driver instead, and only update areq->request on completion (final).

Thanks!
Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com