[PATCH 0/2] crypto: virtio: fix two crash issue

[PATCH 0/2] crypto: virtio: fix two crash issue

[Longpeng(Mike)]

Link: https://lkml.org/lkml/2020/1/23/205

Cc: Gonglei <arei.gonglei@huawei.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org

Longpeng(Mike) (2):
  crypto: virtio: fix src/dst scatterlist calculation
  crypto: virtio: fix an memory use-after-free bug

 drivers/crypto/virtio/virtio_crypto_algs.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/2] crypto: virtio: fix src/dst scatterlist calculation

[Longpeng(Mike)]

The system will crash when we insmod crypto/tcrypt.ko whit mode=38.

Usually the next entry of one sg will be @sg@ + 1, but if this sg element
is part of a chained scatterlist, it could jump to the start of a new
scatterlist array. Let's fix it by sg_next() on calculation of src/dst
scatterlist.

BTW I add a check for sg_nents_for_len() its return value since
sg_nents_for_len() function could fail.

Cc: Gonglei <arei.gonglei@huawei.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org

Reported-by: LABBE Corentin <clabbe@baylibre.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 drivers/crypto/virtio/virtio_crypto_algs.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c b/drivers/crypto/virtio/virtio_crypto_algs.c
index 372babb44112..2fa1129f96d6 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -359,8 +359,14 @@ __virtio_crypto_skcipher_do_req(struct virtio_crypto_sym_request *vc_sym_req,
 	unsigned int num_out = 0, num_in = 0;
 	int sg_total;
 	uint8_t *iv;
+	struct scatterlist *sg;
 
 	src_nents = sg_nents_for_len(req->src, req->cryptlen);
+	if (src_nents < 0) {
+		pr_err("Invalid number of src SG.\n");
+		return src_nents;
+	}
+
 	dst_nents = sg_nents(req->dst);
 
 	pr_debug("virtio_crypto: Number of sgs (src_nents: %d, dst_nents: %d)\n",
@@ -446,12 +452,12 @@ __virtio_crypto_skcipher_do_req(struct virtio_crypto_sym_request *vc_sym_req,
 	vc_sym_req->iv = iv;
 
 	/* Source data */
-	for (i = 0; i < src_nents; i++)
-		sgs[num_out++] = &req->src[i];
+	for (sg = req->src, i = 0; sg && i < src_nents; sg = sg_next(sg), i++)
+		sgs[num_out++] = sg;
 
 	/* Destination data */
-	for (i = 0; i < dst_nents; i++)
-		sgs[num_out + num_in++] = &req->dst[i];
+	for (sg = req->dst, i = 0; sg && i < dst_nents; sg = sg_next(sg), i++)
+		sgs[num_out + num_in++] = sg;
 
 	/* Status */
 	sg_init_one(&status_sg, &vc_req->status, sizeof(vc_req->status));
-- 
2.17.1

Re: [PATCH 1/2] crypto: virtio: fix src/dst scatterlist calculation

[Jason Wang]

On 2020/5/25 上午8:56, Longpeng(Mike) wrote:
> The system will crash when we insmod crypto/tcrypt.ko whit mode=38.
>
> Usually the next entry of one sg will be @sg@ + 1, but if this sg element
> is part of a chained scatterlist, it could jump to the start of a new
> scatterlist array. Let's fix it by sg_next() on calculation of src/dst
> scatterlist.
>
> BTW I add a check for sg_nents_for_len() its return value since
> sg_nents_for_len() function could fail.
>
> Cc: Gonglei <arei.gonglei@huawei.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
>
> Reported-by: LABBE Corentin <clabbe@baylibre.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>   drivers/crypto/virtio/virtio_crypto_algs.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c b/drivers/crypto/virtio/virtio_crypto_algs.c
> index 372babb44112..2fa1129f96d6 100644
> --- a/drivers/crypto/virtio/virtio_crypto_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> @@ -359,8 +359,14 @@ __virtio_crypto_skcipher_do_req(struct virtio_crypto_sym_request *vc_sym_req,
>   	unsigned int num_out = 0, num_in = 0;
>   	int sg_total;
>   	uint8_t *iv;
> +	struct scatterlist *sg;
>   
>   	src_nents = sg_nents_for_len(req->src, req->cryptlen);
> +	if (src_nents < 0) {
> +		pr_err("Invalid number of src SG.\n");
> +		return src_nents;
> +	}
> +
>   	dst_nents = sg_nents(req->dst);
>   
>   	pr_debug("virtio_crypto: Number of sgs (src_nents: %d, dst_nents: %d)\n",
> @@ -446,12 +452,12 @@ __virtio_crypto_skcipher_do_req(struct virtio_crypto_sym_request *vc_sym_req,
>   	vc_sym_req->iv = iv;
>   
>   	/* Source data */
> -	for (i = 0; i < src_nents; i++)
> -		sgs[num_out++] = &req->src[i];
> +	for (sg = req->src, i = 0; sg && i < src_nents; sg = sg_next(sg), i++)


Any reason sg is checked here?

I believe it should be checked in sg_nents_for_len().


> +		sgs[num_out++] = sg;
>   
>   	/* Destination data */
> -	for (i = 0; i < dst_nents; i++)
> -		sgs[num_out + num_in++] = &req->dst[i];
> +	for (sg = req->dst, i = 0; sg && i < dst_nents; sg = sg_next(sg), i++)
> +		sgs[num_out + num_in++] = sg;


I believe sg should be checked in sg_nents().

Thanks


>   
>   	/* Status */
>   	sg_init_one(&status_sg, &vc_req->status, sizeof(vc_req->status));

Re: [PATCH 1/2] crypto: virtio: fix src/dst scatterlist calculation

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

Hi Jason,

On 2020/5/25 11:12, Jason Wang wrote:
> 
> On 2020/5/25 上午8:56, Longpeng(Mike) wrote:
>> The system will crash when we insmod crypto/tcrypt.ko whit mode=38.
>>
>> Usually the next entry of one sg will be @sg@ + 1, but if this sg element
>> is part of a chained scatterlist, it could jump to the start of a new
>> scatterlist array. Let's fix it by sg_next() on calculation of src/dst
>> scatterlist.
>>
>> BTW I add a check for sg_nents_for_len() its return value since
>> sg_nents_for_len() function could fail.
>>
>> Cc: Gonglei <arei.gonglei@huawei.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: virtualization@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org
>>
>> Reported-by: LABBE Corentin <clabbe@baylibre.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
>> ---
>>   drivers/crypto/virtio/virtio_crypto_algs.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
>> b/drivers/crypto/virtio/virtio_crypto_algs.c
>> index 372babb44112..2fa1129f96d6 100644
>> --- a/drivers/crypto/virtio/virtio_crypto_algs.c
>> +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
>> @@ -359,8 +359,14 @@ __virtio_crypto_skcipher_do_req(struct
>> virtio_crypto_sym_request *vc_sym_req,
>>       unsigned int num_out = 0, num_in = 0;
>>       int sg_total;
>>       uint8_t *iv;
>> +    struct scatterlist *sg;
>>         src_nents = sg_nents_for_len(req->src, req->cryptlen);
>> +    if (src_nents < 0) {
>> +        pr_err("Invalid number of src SG.\n");
>> +        return src_nents;
>> +    }
>> +
>>       dst_nents = sg_nents(req->dst);
>>         pr_debug("virtio_crypto: Number of sgs (src_nents: %d, dst_nents: %d)\n",
>> @@ -446,12 +452,12 @@ __virtio_crypto_skcipher_do_req(struct
>> virtio_crypto_sym_request *vc_sym_req,
>>       vc_sym_req->iv = iv;
>>         /* Source data */
>> -    for (i = 0; i < src_nents; i++)
>> -        sgs[num_out++] = &req->src[i];
>> +    for (sg = req->src, i = 0; sg && i < src_nents; sg = sg_next(sg), i++)
> 
> 
> Any reason sg is checked here?
> 
> I believe it should be checked in sg_nents_for_len().
> 
Do you means：
for (sg = req->src, i = 0; i < src_nents; sg = sg_next(sg), i++) ?

> 
>> +        sgs[num_out++] = sg;
>>         /* Destination data */
>> -    for (i = 0; i < dst_nents; i++)
>> -        sgs[num_out + num_in++] = &req->dst[i];
>> +    for (sg = req->dst, i = 0; sg && i < dst_nents; sg = sg_next(sg), i++)
>> +        sgs[num_out + num_in++] = sg;
> 
> 
> I believe sg should be checked in sg_nents().
>
How about
for (sg = req->dst; sg; sg = sg_next(sg)) ?

> Thanks
> 
> 
>>         /* Status */
>>       sg_init_one(&status_sg, &vc_req->status, sizeof(vc_req->status));
> 
> .
> 

-- 
---
Regards,
Longpeng(Mike)

[PATCH 2/2] crypto: virtio: fix an memory use-after-free bug

[Longpeng(Mike)]

The system'll crash when we insmod crypto/tcrypto.ko with mode=155.

After dig into this case, I find it's caused by reuse the request
memory.

In crypto_authenc_init_tfm, we'll set the reqsize to:
  [PART 1]sizeof(authenc_request_ctx) +
  [PART 2]ictx->reqoff +
  [PART 3]MAX(ahash part, skcipher part)
and the 'PART 3' will be used by both ahash and skcipher.

When virtio_crypto driver finish skcipher req, it'll call ->complete
callback(in crypto_finalize_skcipher_request) and then free its
resources which pointers are recorded in 'skcipher parts'.

However, the ->complete is 'crypto_authenc_encrypt_done' in this case,
it will use the 'ahash part' of the request and change its content,
so virtio_crypto driver will get the wrong pointer after ->complete
finish and mistakenly free some other memory. So the system will crash
at last when this memory be used again.

We can free the resources before calling ->complete to fix this issue.

Cc: Gonglei <arei.gonglei@huawei.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>                                                                                                                                                                      
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org

Reported-by: LABBE Corentin <clabbe@baylibre.com>
Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 drivers/crypto/virtio/virtio_crypto_algs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c b/drivers/crypto/virtio/virtio_crypto_algs.c
index 2fa1129f96d6..3800356fb764 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -587,10 +587,11 @@ static void virtio_crypto_skcipher_finalize_req(
 		scatterwalk_map_and_copy(req->iv, req->dst,
 					 req->cryptlen - AES_BLOCK_SIZE,
 					 AES_BLOCK_SIZE, 0);
-	crypto_finalize_skcipher_request(vc_sym_req->base.dataq->engine,
-					   req, err);
 	kzfree(vc_sym_req->iv);
 	virtcrypto_clear_request(&vc_sym_req->base);
+
+	crypto_finalize_skcipher_request(vc_sym_req->base.dataq->engine,
+					   req, err);
 }
 
 static struct virtio_crypto_algo virtio_crypto_algs[] = { {
-- 
2.17.1