| Message ID | 20171128090518.12469-4-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
On Tue, Nov 28, 2017 at 10:05:17AM +0100, Antoine Tenart wrote: > The patch fixes the ahash support by only updating the result buffer > when provided. Otherwise the driver could crash with NULL pointer > exceptions, because the ahash caller isn't required to supply a result > buffer on all calls. > > Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > --- > drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > index 6135c9f5742c..424f4c5d4d25 100644 > --- a/drivers/crypto/inside-secure/safexcel_hash.c > +++ b/drivers/crypto/inside-secure/safexcel_hash.c > @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin > > 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. Thanks,
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
On Mon, Dec 11, 2017 at 08:49:57AM +0100, Antoine Tenart wrote: > > 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). That's one way to do it. Ideally you'd only use the local buffer for the non-final case so that the final case just does the DMA to the final destination. Cheers,
diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c index 6135c9f5742c..424f4c5d4d25 100644 --- a/drivers/crypto/inside-secure/safexcel_hash.c +++ b/drivers/crypto/inside-secure/safexcel_hash.c @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin 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); dma_unmap_sg(priv->dev, areq->src, sg_nents_for_len(areq->src, areq->nbytes), DMA_TO_DEVICE);
The patch fixes the ahash support by only updating the result buffer when provided. Otherwise the driver could crash with NULL pointer exceptions, because the ahash caller isn't required to supply a result buffer on all calls. Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)