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
