* [PATCH v2 0/2] crypto: virtio: Fix two crash issue @ 2020-05-26 3:19 Longpeng(Mike) 2020-05-26 3:19 ` [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() Longpeng(Mike) 2020-05-26 3:19 ` [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() Longpeng(Mike) 0 siblings, 2 replies; 17+ messages in thread From: Longpeng(Mike) @ 2020-05-26 3:19 UTC (permalink / raw) To: linux-crypto Cc: Longpeng(Mike), Gonglei, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Markus Elfring, virtualization, linux-kernel, stable Link: https://lkml.org/lkml/2020/1/23/205 Changes since v1: - remove some redundant checks [Jason] - normalize the commit message [Markus] 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: Markus Elfring <Markus.Elfring@web.de> Cc: virtualization@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Longpeng(Mike) (2): crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() drivers/crypto/virtio/virtio_crypto_algs.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() 2020-05-26 3:19 [PATCH v2 0/2] crypto: virtio: Fix two crash issue Longpeng(Mike) @ 2020-05-26 3:19 ` Longpeng(Mike) 2020-05-26 7:03 ` Markus Elfring 2020-05-26 14:11 ` [PATCH v2 " Sasha Levin 2020-05-26 3:19 ` [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() Longpeng(Mike) 1 sibling, 2 replies; 17+ messages in thread From: Longpeng(Mike) @ 2020-05-26 3:19 UTC (permalink / raw) To: linux-crypto Cc: Longpeng(Mike), LABBE Corentin, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Markus Elfring, virtualization, linux-kernel, stable, Gonglei The system will crash when the users insmod crypto/tcrypt.ko with mode=38 ( testing "cts(cbc(aes))" ). 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. Fix it by sg_next() on calculation of src/dst scatterlist. Fixes: dbaf0624ffa5 ("crypto: add virtio-crypto driver") Reported-by: LABBE Corentin <clabbe@baylibre.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: Markus Elfring <Markus.Elfring@web.de> Cc: virtualization@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Message-Id: <20200123101000.GB24255@Red> Signed-off-by: Gonglei <arei.gonglei@huawei.com> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> --- drivers/crypto/virtio/virtio_crypto_algs.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c b/drivers/crypto/virtio/virtio_crypto_algs.c index fd045e64972a..5f8243563009 100644 --- a/drivers/crypto/virtio/virtio_crypto_algs.c +++ b/drivers/crypto/virtio/virtio_crypto_algs.c @@ -350,13 +350,18 @@ __virtio_crypto_skcipher_do_req(struct virtio_crypto_sym_request *vc_sym_req, int err; unsigned long flags; struct scatterlist outhdr, iv_sg, status_sg, **sgs; - int i; u64 dst_len; 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", @@ -442,12 +447,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; src_nents; sg = sg_next(sg), src_nents--) + 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; sg; sg = sg_next(sg)) + sgs[num_out + num_in++] = sg; /* Status */ sg_init_one(&status_sg, &vc_req->status, sizeof(vc_req->status)); -- 2.23.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() 2020-05-26 3:19 ` [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() Longpeng(Mike) @ 2020-05-26 7:03 ` Markus Elfring 2020-05-26 7:21 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 14:11 ` [PATCH v2 " Sasha Levin 1 sibling, 1 reply; 17+ messages in thread From: Markus Elfring @ 2020-05-26 7:03 UTC (permalink / raw) To: Longpeng(Mike), linux-crypto, virtualization Cc: Corentin Labbe, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Gonglei, linux-kernel, stable > Fix it by sg_next() on calculation of src/dst scatterlist. Wording adjustment: … by calling the function “sg_next” … … > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c > @@ -350,13 +350,18 @@ __virtio_crypto_skcipher_do_req(struct virtio_crypto_sym_request *vc_sym_req, … > 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); … I suggest to move the addition of such input parameter validation to a separate update step. Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() 2020-05-26 7:03 ` Markus Elfring @ 2020-05-26 7:21 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 8:25 ` [v2 " Markus Elfring 0 siblings, 1 reply; 17+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-05-26 7:21 UTC (permalink / raw) To: Markus Elfring, linux-crypto, virtualization Cc: Corentin Labbe, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Gonglei, linux-kernel, stable Hi Markus, On 2020/5/26 15:03, Markus Elfring wrote: >> Fix it by sg_next() on calculation of src/dst scatterlist. > > Wording adjustment: > … by calling the function “sg_next” … > OK, thanks. > > … >> +++ b/drivers/crypto/virtio/virtio_crypto_algs.c >> @@ -350,13 +350,18 @@ __virtio_crypto_skcipher_do_req(struct virtio_crypto_sym_request *vc_sym_req, > … >> 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); > … > > I suggest to move the addition of such input parameter validation > to a separate update step. > Um...The 'src_nents' will be used as a loop condition, so validate it here is needed ? ''' /* Source data */ - for (i = 0; i < src_nents; i++) - sgs[num_out++] = &req->src[i]; + for (sg = req->src; src_nents; sg = sg_next(sg), src_nents--) + sgs[num_out++] = sg; ''' > Regards, > Markus > -- --- Regards, Longpeng(Mike) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() 2020-05-26 7:21 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-05-26 8:25 ` Markus Elfring 0 siblings, 0 replies; 17+ messages in thread From: Markus Elfring @ 2020-05-26 8:25 UTC (permalink / raw) To: Longpeng (Mike), linux-crypto, virtualization Cc: Corentin Labbe, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Gonglei, linux-kernel, stable >> I suggest to move the addition of such input parameter validation >> to a separate update step. >> > Um...The 'src_nents' will be used as a loop condition, > so validate it here is needed ? Would you prefer to add such checking as the first update in another small patch series? Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() 2020-05-26 3:19 ` [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() Longpeng(Mike) 2020-05-26 7:03 ` Markus Elfring @ 2020-05-26 14:11 ` Sasha Levin 1 sibling, 0 replies; 17+ messages in thread From: Sasha Levin @ 2020-05-26 14:11 UTC (permalink / raw) To: Sasha Levin, Longpeng(Mike), linux-crypto Cc: Longpeng(Mike), Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Markus Elfring, virtualization, linux-kernel, stable, stable <20200123101000.GB24255@Red> References: <20200526031956.1897-2-longpeng2@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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 3:19 [PATCH v2 0/2] crypto: virtio: Fix two crash issue Longpeng(Mike) 2020-05-26 3:19 ` [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() Longpeng(Mike) @ 2020-05-26 3:19 ` Longpeng(Mike) 2020-05-26 7:19 ` Markus Elfring ` (2 more replies) 1 sibling, 3 replies; 17+ messages in thread From: Longpeng(Mike) @ 2020-05-26 3:19 UTC (permalink / raw) To: linux-crypto Cc: Longpeng(Mike), LABBE Corentin, Gonglei, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Markus Elfring, virtualization, linux-kernel, stable 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 <clabbe@baylibre.com> 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: Markus Elfring <Markus.Elfring@web.de> Cc: virtualization@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Message-Id: <20200123101000.GB24255@Red> 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 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(&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.23.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 3:19 ` [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() Longpeng(Mike) @ 2020-05-26 7:19 ` Markus Elfring 2020-05-26 7:38 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 12:00 ` [PATCH v2 " Gonglei (Arei) 2020-05-26 14:11 ` Sasha Levin 2 siblings, 1 reply; 17+ messages in thread From: Markus Elfring @ 2020-05-26 7:19 UTC (permalink / raw) To: Longpeng(Mike), linux-crypto, virtualization Cc: Corentin Labbe, Gonglei, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, linux-kernel, stable > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 7:19 ` Markus Elfring @ 2020-05-26 7:38 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 9:01 ` [v2 " Markus Elfring 0 siblings, 1 reply; 17+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-05-26 7:38 UTC (permalink / raw) To: Markus Elfring, linux-crypto, virtualization Cc: Corentin Labbe, Gonglei, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, linux-kernel, stable 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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 7:38 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-05-26 9:01 ` Markus Elfring 2020-05-26 9:24 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 0 siblings, 1 reply; 17+ messages in thread From: Markus Elfring @ 2020-05-26 9:01 UTC (permalink / raw) To: Longpeng (Mike), linux-crypto, virtualization Cc: Corentin Labbe, Gonglei, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, linux-kernel, stable >>> … Thus release specific resources before >> >> Is there a need to improve also this information another bit? >> > You mean the last two paragraph is redundant ? No. I became curious if you would like to choose a more helpful information according to the wording “specific resources”. Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 9:01 ` [v2 " Markus Elfring @ 2020-05-26 9:24 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 9:30 ` Markus Elfring 2020-05-26 10:16 ` Greg KH 0 siblings, 2 replies; 17+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-05-26 9:24 UTC (permalink / raw) To: Markus Elfring, linux-crypto, virtualization, Gonglei, Herbert Xu Cc: Corentin Labbe, Michael S. Tsirkin, Jason Wang, David S. Miller, linux-kernel, stable On 2020/5/26 17:01, Markus Elfring wrote: >>>> … Thus release specific resources before >>> >>> Is there a need to improve also this information another bit? >>> >> You mean the last two paragraph is redundant ? > > No. > > I became curious if you would like to choose a more helpful information > according to the wording “specific resources”. > > Regards, > Markus > Hi Markus, I respect your work, but please let us to focus on the code itself. I think experts in this area know what these patches want to solve after look at the code. I hope experts in the thread could review the code when you free, thanks :) --- Regards, Longpeng(Mike) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 9:24 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-05-26 9:30 ` Markus Elfring 2020-05-26 10:16 ` Greg KH 1 sibling, 0 replies; 17+ messages in thread From: Markus Elfring @ 2020-05-26 9:30 UTC (permalink / raw) To: Longpeng (Mike), linux-crypto, virtualization Cc: Gonglei, Herbert Xu, Corentin Labbe, Michael S. Tsirkin, Jason Wang, David S. Miller, linux-kernel, stable > I respect your work, but please let us to focus on the code itself. I think > experts in this area know what these patches want to solve after look at the code. I suggest to reconsider such a view. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=9cb1fd0efd195590b828b9b865421ad345a4a145#n102 Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 9:24 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 9:30 ` Markus Elfring @ 2020-05-26 10:16 ` Greg KH 1 sibling, 0 replies; 17+ messages in thread From: Greg KH @ 2020-05-26 10:16 UTC (permalink / raw) To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) Cc: Markus Elfring, linux-crypto, virtualization, Gonglei, Herbert Xu, Corentin Labbe, Michael S. Tsirkin, Jason Wang, David S. Miller, linux-kernel, stable On Tue, May 26, 2020 at 05:24:03PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: > > > On 2020/5/26 17:01, Markus Elfring wrote: > >>>> … Thus release specific resources before > >>> > >>> Is there a need to improve also this information another bit? > >>> > >> You mean the last two paragraph is redundant ? > > > > No. > > > > I became curious if you would like to choose a more helpful information > > according to the wording “specific resources”. > > > > Regards, > > Markus > > > Hi Markus, > > I respect your work, but please let us to focus on the code itself. I think > experts in this area know what these patches want to solve after look at the code. > > I hope experts in the thread could review the code when you free, thanks :) Please note that you are responding to someone who is known to be a pain in patch reviews and has been blocked by many kernel developers/maintainers because they just waste people's time. I suggest you all do the same here, and just ignore them, like I do :) thanks, greg k-h ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 3:19 ` [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() Longpeng(Mike) 2020-05-26 7:19 ` Markus Elfring @ 2020-05-26 12:00 ` Gonglei (Arei) 2020-05-26 14:11 ` Sasha Levin 2 siblings, 0 replies; 17+ messages in thread From: Gonglei (Arei) @ 2020-05-26 12:00 UTC (permalink / raw) To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.), linux-crypto Cc: LABBE Corentin, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Markus Elfring, virtualization, linux-kernel, stable > -----Original Message----- > From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > Sent: Tuesday, May 26, 2020 11:20 AM > To: linux-crypto@vger.kernel.org > Cc: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > <longpeng2@huawei.com>; LABBE Corentin <clabbe@baylibre.com>; Gonglei > (Arei) <arei.gonglei@huawei.com>; Herbert Xu > <herbert@gondor.apana.org.au>; Michael S. Tsirkin <mst@redhat.com>; Jason > Wang <jasowang@redhat.com>; David S. Miller <davem@davemloft.net>; > Markus Elfring <Markus.Elfring@web.de>; > virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org; > stable@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 <clabbe@baylibre.com> > 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: Markus Elfring <Markus.Elfring@web.de> > Cc: virtualization@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org > Cc: stable@vger.kernel.org > Message-Id: <20200123101000.GB24255@Red> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > --- Acked-by: Gonglei <arei.gonglei@huawei.com> 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(&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.23.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 3:19 ` [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() Longpeng(Mike) 2020-05-26 7:19 ` Markus Elfring 2020-05-26 12:00 ` [PATCH v2 " Gonglei (Arei) @ 2020-05-26 14:11 ` Sasha Levin 2020-05-31 9:21 ` Michael S. Tsirkin 2 siblings, 1 reply; 17+ messages in thread From: Sasha Levin @ 2020-05-26 14:11 UTC (permalink / raw) To: Sasha Levin, Longpeng(Mike), linux-crypto Cc: Longpeng(Mike), Gonglei, Herbert Xu, Michael S. Tsirkin, Jason Wang, David S. Miller, Markus Elfring, virtualization, linux-kernel, stable, stable <20200123101000.GB24255@Red> References: <20200526031956.1897-3-longpeng2@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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-26 14:11 ` Sasha Levin @ 2020-05-31 9:21 ` Michael S. Tsirkin 2020-06-02 3:54 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 0 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2020-05-31 9:21 UTC (permalink / raw) To: Sasha Levin Cc: Longpeng(Mike), linux-crypto, Gonglei, Herbert Xu, Jason Wang, David S. Miller, Markus Elfring, virtualization, linux-kernel, stable On Tue, May 26, 2020 at 02:11:37PM +0000, Sasha Levin wrote: > <20200123101000.GB24255@Red> > References: <20200526031956.1897-3-longpeng2@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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() 2020-05-31 9:21 ` Michael S. Tsirkin @ 2020-06-02 3:54 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 0 siblings, 0 replies; 17+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-06-02 3:54 UTC (permalink / raw) To: Michael S. Tsirkin, Sasha Levin Cc: linux-crypto, Gonglei, Herbert Xu, Jason Wang, David S. Miller, Markus Elfring, virtualization, linux-kernel, stable On 2020/5/31 17:21, Michael S. Tsirkin wrote: > On Tue, May 26, 2020 at 02:11:37PM +0000, Sasha Levin wrote: >> <20200123101000.GB24255@Red> >> References: <20200526031956.1897-3-longpeng2@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) ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-06-02 3:54 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-26 3:19 [PATCH v2 0/2] crypto: virtio: Fix two crash issue Longpeng(Mike) 2020-05-26 3:19 ` [PATCH v2 1/2] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req() Longpeng(Mike) 2020-05-26 7:03 ` Markus Elfring 2020-05-26 7:21 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 8:25 ` [v2 " Markus Elfring 2020-05-26 14:11 ` [PATCH v2 " Sasha Levin 2020-05-26 3:19 ` [PATCH v2 2/2] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req() Longpeng(Mike) 2020-05-26 7:19 ` Markus Elfring 2020-05-26 7:38 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 9:01 ` [v2 " Markus Elfring 2020-05-26 9:24 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-05-26 9:30 ` Markus Elfring 2020-05-26 10:16 ` Greg KH 2020-05-26 12:00 ` [PATCH v2 " Gonglei (Arei) 2020-05-26 14:11 ` Sasha Levin 2020-05-31 9:21 ` Michael S. Tsirkin 2020-06-02 3:54 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).