diff mbox series

[5.10,129/593] crypto: qce: skcipher: Fix incorrect sg count for dma transfers

Message ID 20210712060857.335221127@linuxfoundation.org
State Superseded
Headers show
Series None | expand

Commit Message

Greg KH July 12, 2021, 6:04 a.m. UTC 
From: Thara Gopinath <thara.gopinath@linaro.org>


[ Upstream commit 1339a7c3ba05137a2d2fe75f602311bbfc6fab33 ]

Use the sg count returned by dma_map_sg to call into
dmaengine_prep_slave_sg rather than using the original sg count. dma_map_sg
can merge consecutive sglist entries, thus making the original sg count
wrong. This is a fix for memory coruption issues observed while testing
encryption/decryption of large messages using libkcapi framework.

Patch has been tested further by running full suite of tcrypt.ko tests
including fuzz tests.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Signed-off-by: Sasha Levin <sashal@kernel.org>

---
 drivers/crypto/qce/skcipher.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

-- 
2.30.2

Comments

Pavel Machek July 14, 2021, 7:40 p.m. UTC | #1 
Hi!

> [ Upstream commit 1339a7c3ba05137a2d2fe75f602311bbfc6fab33 ]

> 

> Use the sg count returned by dma_map_sg to call into

> dmaengine_prep_slave_sg rather than using the original sg count. dma_map_sg

> can merge consecutive sglist entries, thus making the original sg count

> wrong. This is a fix for memory coruption issues observed while testing

> encryption/decryption of large messages using libkcapi framework.

> 

> Patch has been tested further by running full suite of tcrypt.ko tests

> including fuzz tests.


This still needs more work AFAICT.

> index a2d3da0ad95f..5a6559131eac 100644

> --- a/drivers/crypto/qce/skcipher.c

> +++ b/drivers/crypto/qce/skcipher.c

> @@ -122,21 +122,22 @@ qce_skcipher_async_req_handle(struct crypto_async_request *async_req)

>  	sg_mark_end(sg);

>  	rctx->dst_sg = rctx->dst_tbl.sgl;


ret is == 0 at this point.

> -	ret = dma_map_sg(qce->dev, rctx->dst_sg, rctx->dst_nents, dir_dst);

> -	if (ret < 0)

> +	dst_nents = dma_map_sg(qce->dev, rctx->dst_sg, rctx->dst_nents, dir_dst);

> +	if (dst_nents < 0)

>  		goto error_free;


And we go to the error path, and return ret... instead of returning failure.

>  	if (diff_dst) {

> -		ret = dma_map_sg(qce->dev, req->src, rctx->src_nents, dir_src);

> -		if (ret < 0)

> +		src_nents = dma_map_sg(qce->dev, req->src, rctx->src_nents, dir_src);

> +		if (src_nents < 0)

>  			goto error_unmap_dst;

>  		rctx->src_sg = req->src;


Same problem happens here.

The problem is already fixed in the mainline; I believe we want that
in 5.10-stable at least.

commit a8bc4f5e7a72e4067f5afd7e98b61624231713ca
Author: Wei Yongjun <weiyongjun1@huawei.com>
Date:   Wed Jun 2 11:36:45 2021 +0000

    crypto: qce - fix error return code in qce_skcipher_async_req_handle()

    Fix to return a negative error code from the error handling
        case instead of 0, as done elsewhere in this function.

    Fixes: 1339a7c3ba05 ("crypto: qce: skcipher: Fix incorrect sg
    count for dma transfers")
        Reported-by: Hulk Robot <hulkci@huawei.com>
	    Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

	    

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Greg KH July 15, 2021, 10:46 a.m. UTC | #2 
On Wed, Jul 14, 2021 at 09:40:28PM +0200, Pavel Machek wrote:
> Hi!

> 

> > [ Upstream commit 1339a7c3ba05137a2d2fe75f602311bbfc6fab33 ]

> > 

> > Use the sg count returned by dma_map_sg to call into

> > dmaengine_prep_slave_sg rather than using the original sg count. dma_map_sg

> > can merge consecutive sglist entries, thus making the original sg count

> > wrong. This is a fix for memory coruption issues observed while testing

> > encryption/decryption of large messages using libkcapi framework.

> > 

> > Patch has been tested further by running full suite of tcrypt.ko tests

> > including fuzz tests.

> 

> This still needs more work AFAICT.

> 

> > index a2d3da0ad95f..5a6559131eac 100644

> > --- a/drivers/crypto/qce/skcipher.c

> > +++ b/drivers/crypto/qce/skcipher.c

> > @@ -122,21 +122,22 @@ qce_skcipher_async_req_handle(struct crypto_async_request *async_req)

> >  	sg_mark_end(sg);

> >  	rctx->dst_sg = rctx->dst_tbl.sgl;

> 

> ret is == 0 at this point.

> 

> > -	ret = dma_map_sg(qce->dev, rctx->dst_sg, rctx->dst_nents, dir_dst);

> > -	if (ret < 0)

> > +	dst_nents = dma_map_sg(qce->dev, rctx->dst_sg, rctx->dst_nents, dir_dst);

> > +	if (dst_nents < 0)

> >  		goto error_free;

> 

> And we go to the error path, and return ret... instead of returning failure.

> 

> >  	if (diff_dst) {

> > -		ret = dma_map_sg(qce->dev, req->src, rctx->src_nents, dir_src);

> > -		if (ret < 0)

> > +		src_nents = dma_map_sg(qce->dev, req->src, rctx->src_nents, dir_src);

> > +		if (src_nents < 0)

> >  			goto error_unmap_dst;

> >  		rctx->src_sg = req->src;

> 

> Same problem happens here.

> 

> The problem is already fixed in the mainline; I believe we want that

> in 5.10-stable at least.

> 

> commit a8bc4f5e7a72e4067f5afd7e98b61624231713ca

> Author: Wei Yongjun <weiyongjun1@huawei.com>

> Date:   Wed Jun 2 11:36:45 2021 +0000

> 

>     crypto: qce - fix error return code in qce_skcipher_async_req_handle()

> 

>     Fix to return a negative error code from the error handling

>         case instead of 0, as done elsewhere in this function.

> 

>     Fixes: 1339a7c3ba05 ("crypto: qce: skcipher: Fix incorrect sg

>     count for dma transfers")

>         Reported-by: Hulk Robot <hulkci@huawei.com>

> 	    Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

> 	    

> 


This is also already in this 5.10.50 release.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index a2d3da0ad95f..5a6559131eac 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -71,7 +71,7 @@  qce_skcipher_async_req_handle(struct crypto_async_request *async_req)
 	struct scatterlist *sg;
 	bool diff_dst;
 	gfp_t gfp;
-	int ret;
+	int dst_nents, src_nents, ret;
 
 	rctx->iv = req->iv;
 	rctx->ivsize = crypto_skcipher_ivsize(skcipher);
@@ -122,21 +122,22 @@  qce_skcipher_async_req_handle(struct crypto_async_request *async_req)
 	sg_mark_end(sg);
 	rctx->dst_sg = rctx->dst_tbl.sgl;
 
-	ret = dma_map_sg(qce->dev, rctx->dst_sg, rctx->dst_nents, dir_dst);
-	if (ret < 0)
+	dst_nents = dma_map_sg(qce->dev, rctx->dst_sg, rctx->dst_nents, dir_dst);
+	if (dst_nents < 0)
 		goto error_free;
 
 	if (diff_dst) {
-		ret = dma_map_sg(qce->dev, req->src, rctx->src_nents, dir_src);
-		if (ret < 0)
+		src_nents = dma_map_sg(qce->dev, req->src, rctx->src_nents, dir_src);
+		if (src_nents < 0)
 			goto error_unmap_dst;
 		rctx->src_sg = req->src;
 	} else {
 		rctx->src_sg = rctx->dst_sg;
+		src_nents = dst_nents - 1;
 	}
 
-	ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, rctx->src_nents,
-			       rctx->dst_sg, rctx->dst_nents,
+	ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, src_nents,
+			       rctx->dst_sg, dst_nents,
 			       qce_skcipher_done, async_req);
 	if (ret)
 		goto error_unmap_src;