Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()
On 2020/5/31 17:21, Michael S. Tsirkin wrote:
> On Tue, May 26, 2020 at 02:11:37PM +, Sasha Levin wrote:
>> <20200123101000.GB24255@Red>
>> References: <20200526031956.1897-3-longpe...@huawei.com>
>> <20200123101000.GB24255@Red>
>>
>> Hi
>>
>> [This is an automated email]
>>
>> This commit has been processed because it contains a "Fixes:" tag
>> fixing commit: dbaf0624ffa5 ("crypto: add virtio-crypto driver").
>>
>> The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124,
>> v4.14.181.
>>
>> v5.6.14: Build OK!
>> v5.4.42: Failed to apply! Possible dependencies:
>> eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
>>
>> v4.19.124: Failed to apply! Possible dependencies:
>> eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
>>
>> v4.14.181: Failed to apply! Possible dependencies:
>> 500e6807ce93 ("crypto: virtio - implement missing support for output
>> IVs")
>> 67189375bb3a ("crypto: virtio - convert to new crypto engine API")
>> d0d859bb87ac ("crypto: virtio - Register an algo only if it's supported")
>> e02b8b43f55a ("crypto: virtio - pr_err() strings should end with
>> newlines")
>> eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
>>
>>
>> NOTE: The patch will not be queued to stable trees until it is upstream.
>>
>> How should we proceed with this patch?
>
> Mike could you comment on backporting?
>
Hi Michael,
I will send V3, so I will resolve these conflicts later. :)
>> --
>> Thanks
>> Sasha
>
> .
>
---
Regards,
Longpeng(Mike)
Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()
On Tue, May 26, 2020 at 02:11:37PM +, Sasha Levin wrote:
> <20200123101000.GB24255@Red>
> References: <20200526031956.1897-3-longpe...@huawei.com>
> <20200123101000.GB24255@Red>
>
> Hi
>
> [This is an automated email]
>
> This commit has been processed because it contains a "Fixes:" tag
> fixing commit: dbaf0624ffa5 ("crypto: add virtio-crypto driver").
>
> The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124,
> v4.14.181.
>
> v5.6.14: Build OK!
> v5.4.42: Failed to apply! Possible dependencies:
> eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
>
> v4.19.124: Failed to apply! Possible dependencies:
> eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
>
> v4.14.181: Failed to apply! Possible dependencies:
> 500e6807ce93 ("crypto: virtio - implement missing support for output IVs")
> 67189375bb3a ("crypto: virtio - convert to new crypto engine API")
> d0d859bb87ac ("crypto: virtio - Register an algo only if it's supported")
> e02b8b43f55a ("crypto: virtio - pr_err() strings should end with
> newlines")
> eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?
Mike could you comment on backporting?
> --
> Thanks
> Sasha
Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()
<20200123101000.GB24255@Red>
References: <20200526031956.1897-3-longpe...@huawei.com>
<20200123101000.GB24255@Red>
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag
fixing commit: dbaf0624ffa5 ("crypto: add virtio-crypto driver").
The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124, v4.14.181.
v5.6.14: Build OK!
v5.4.42: Failed to apply! Possible dependencies:
eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
v4.19.124: Failed to apply! Possible dependencies:
eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
v4.14.181: Failed to apply! Possible dependencies:
500e6807ce93 ("crypto: virtio - implement missing support for output IVs")
67189375bb3a ("crypto: virtio - convert to new crypto engine API")
d0d859bb87ac ("crypto: virtio - Register an algo only if it's supported")
e02b8b43f55a ("crypto: virtio - pr_err() strings should end with newlines")
eee1d6fca0a0 ("crypto: virtio - switch to skcipher API")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
RE: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()
> -Original Message-
> From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> Sent: Tuesday, May 26, 2020 11:20 AM
> To: linux-cry...@vger.kernel.org
> Cc: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> ; LABBE Corentin ; Gonglei
> (Arei) ; Herbert Xu
> ; Michael S. Tsirkin ; Jason
> Wang ; David S. Miller ;
> Markus Elfring ;
> virtualizat...@lists.linux-foundation.org; linux-kernel@vger.kernel.org;
> sta...@vger.kernel.org
> Subject: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in
> virtio_crypto_skcipher_finalize_req()
>
> The system'll crash when the users insmod crypto/tcrypto.ko with mode=155
> ( testing "authenc(hmac(sha1),cbc(aes))" ). It's caused by reuse the memory of
> request structure.
>
> In crypto_authenc_init_tfm(), the reqsize is set to:
> [PART 1] sizeof(authenc_request_ctx) +
> [PART 2] ictx->reqoff +
> [PART 3] MAX(ahash part, skcipher part) and the 'PART 3' is used by both
> ahash and skcipher in turn.
>
> When the virtio_crypto driver finish skcipher req, it'll call ->complete
> callback(in
> crypto_finalize_skcipher_request) and then free its resources whose 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's memory. So the system will crash when these memory will be used
> again.
>
> The resources which need to be cleaned up are not used any more. But the
> pointers of these resources may be changed in the function
> "crypto_finalize_skcipher_request". Thus release specific resources before
> calling this function.
>
> Fixes: dbaf0624ffa5 ("crypto: add virtio-crypto driver")
> Reported-by: LABBE Corentin
> Cc: Gonglei
> Cc: Herbert Xu
> Cc: "Michael S. Tsirkin"
> Cc: Jason Wang
> Cc: "David S. Miller"
> Cc: Markus Elfring
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Cc: sta...@vger.kernel.org
> Message-Id: <20200123101000.GB24255@Red>
> Signed-off-by: Longpeng(Mike)
> ---
Acked-by: Gonglei
Regards,
-Gonglei
> 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 5f8243563009..52261b6c247e 100644
> --- a/drivers/crypto/virtio/virtio_crypto_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> @@ -582,10 +582,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(_sym_req->base);
> +
> + crypto_finalize_skcipher_request(vc_sym_req->base.dataq->engine,
> +req, err);
> }
>
> static struct virtio_crypto_algo virtio_crypto_algs[] = { {
> --
> 2.23.0
Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()
Hi Markus, On 2020/5/26 15:19, Markus Elfring wrote: >> The system'll crash when the users insmod crypto/tcrypto.ko with mode=155 >> ( testing "authenc(hmac(sha1),cbc(aes))" ). It's caused by reuse the memory >> of request structure. > > Wording adjustments: > * … system will crash … > * … It is caused by reusing the … > > >> when these memory will be used again. > > when this memory … > OK. > >> … Thus release specific resources before > > Is there a need to improve also this information another bit? > You mean the last two paragraph is redundant ? ''' When the virtio_crypto driver finish skcipher req, it'll call ->complete callback(in crypto_finalize_skcipher_request) and then free its resources whose 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's memory. So the system will crash when these memory will be used again. The resources which need to be cleaned up are not used any more. But the pointers of these resources may be changed in the function "crypto_finalize_skcipher_request". Thus release specific resources before calling this function. ''' How about: ''' When the virtio_crypto driver finish the skcipher request, it will call the function "crypto_finalize_skcipher_request()" and then free the resources whose pointers are stored in the 'skcipher parts', but the pointers of these resources may be changed in that function. Thus fix it by releasing these resources befored calling the function "crypto_finalize_skcipher_request()". ''' > Regards, > Markus > --- Regards, Longpeng(Mike)
Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()
> The system'll crash when the users insmod crypto/tcrypto.ko with mode=155 > ( testing "authenc(hmac(sha1),cbc(aes))" ). It's caused by reuse the memory > of request structure. Wording adjustments: * … system will crash … * … It is caused by reusing the … > when these memory will be used again. when this memory … > … Thus release specific resources before Is there a need to improve also this information another bit? Regards, Markus
[PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()
The system'll crash when the users insmod crypto/tcrypto.ko with mode=155
( testing "authenc(hmac(sha1),cbc(aes))" ). It's caused by reuse the memory
of request structure.
In crypto_authenc_init_tfm(), the reqsize is set to:
[PART 1] sizeof(authenc_request_ctx) +
[PART 2] ictx->reqoff +
[PART 3] MAX(ahash part, skcipher part)
and the 'PART 3' is used by both ahash and skcipher in turn.
When the virtio_crypto driver finish skcipher req, it'll call ->complete
callback(in crypto_finalize_skcipher_request) and then free its
resources whose 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's memory. So the system will crash
when these memory will be used again.
The resources which need to be cleaned up are not used any more. But the
pointers of these resources may be changed in the function
"crypto_finalize_skcipher_request". Thus release specific resources before
calling this function.
Fixes: dbaf0624ffa5 ("crypto: add virtio-crypto driver")
Reported-by: LABBE Corentin
Cc: Gonglei
Cc: Herbert Xu
Cc: "Michael S. Tsirkin"
Cc: Jason Wang
Cc: "David S. Miller"
Cc: Markus Elfring
Cc: virtualizat...@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Cc: sta...@vger.kernel.org
Message-Id: <20200123101000.GB24255@Red>
Signed-off-by: Longpeng(Mike)
---
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 5f8243563009..52261b6c247e 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -582,10 +582,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(_sym_req->base);
+
+ crypto_finalize_skcipher_request(vc_sym_req->base.dataq->engine,
+ req, err);
}
static struct virtio_crypto_algo virtio_crypto_algs[] = { {
--
2.23.0
