Re: [PATCH] virtio-mem: Fix build error due to improper use 'select'

2020-06-19 Thread Jason Wang


On 2020/6/19 下午4:03, Weilong Chen wrote:

As noted in:
https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt
"select should be used with care. select will force a symbol to a
value without visiting the dependencies."
Config VIRTIO_MEM should not select CONTIG_ALLOC directly.
Otherwise it will cause an error:
https://bugzilla.kernel.org/show_bug.cgi?id=208245

Signed-off-by: Weilong Chen 
---
  drivers/virtio/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 5809e5f5b157..5c92e4a50882 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -85,7 +85,7 @@ config VIRTIO_MEM
depends on VIRTIO
depends on MEMORY_HOTPLUG_SPARSE
depends on MEMORY_HOTREMOVE
-   select CONTIG_ALLOC
+   depends on CONTIG_ALLOC
help
 This driver provides access to virtio-mem paravirtualized memory
 devices, allowing to hotplug and hotunplug memory.



Acked-by: Jason Wang 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 1/1] s390: virtio: let arch accept devices without IOMMU feature

2020-06-19 Thread Cornelia Huck
On Thu, 18 Jun 2020 00:29:56 +0200
Halil Pasic  wrote:

> On Wed, 17 Jun 2020 12:43:57 +0200
> Pierre Morel  wrote:
> 
> > An architecture protecting the guest memory against unauthorized host
> > access may want to enforce VIRTIO I/O device protection through the
> > use of VIRTIO_F_IOMMU_PLATFORM.
> > 
> > Let's give a chance to the architecture to accept or not devices
> > without VIRTIO_F_IOMMU_PLATFORM.
> >   
> [..]
> 
> 
> I'm still not really satisfied with your commit message, furthermore
> I did some thinking about the abstraction you introduce here. I will
> give a short analysis of that, but first things first. Your patch does
> the job of preventing calamity, and the details can be changed any time,
> thus: 
> 
> Acked-by: Halil Pasic 
> 
> Regarding the interaction of architecture specific code with virtio core,
> I believe we could have made the interface more generic.
> 
> One option is to introduce virtio_arch_finalize_features(), a hook that
> could reject any feature that is inappropriate.

s/any feature/any combination of features/

This sounds like a good idea (for a later update).

> 
> Another option would be to find a common name for is_prot_virt_guest()
> (arch/s390) sev_active() (arch/x86) and is_secure_guest() (arch/powerpc)
> and use that instead of arch_needs_virtio_iommu_platform() and where-ever
> appropriate. Currently we seem to want this info in driver code only for
> virtio, but if the virtio driver has a legitimate need to know, other
> drivers may as well have a legitimate need to know. For example if we
> wanted to protect ourselves in ccw device drivers from somebody
> setting up a vfio-ccw device and attach it to the prot-virt guest (AFAICT
> we only lack guest enablement for this) such a function could be useful.

I'm not really sure if we can find enough commonality between
architectures, unless you propose to have a function for checking
things like device memory only.

> 
> But since this can be rewritten any time, let's go with the option
> people already agree with, instead of more discussion.

Yes, there's nothing wrong with the patch as-is.

Acked-by: Cornelia Huck 

Which tree should this go through? Virtio? s390?

> 
> Just another question. Do we want this backported? Do we need cc stable?

It does change behaviour of virtio-ccw devices; but then, it only
fences off configurations that would not have worked anyway.
Distributions should probably pick this; but I do not consider it
strictly a "fix" (more a mitigation for broken configurations), so I'm
not sure whether stable applies.

> [..]
> 
> 
> >  int virtio_finalize_features(struct virtio_device *dev)
> >  {
> > int ret = dev->config->finalize_features(dev);
> > @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
> > if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> > return 0;
> >  
> > +   if (arch_needs_virtio_iommu_platform(dev) &&
> > +   !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > +   dev_warn(&dev->dev,
> > +"virtio: device must provide 
> > VIRTIO_F_IOMMU_PLATFORM\n");  
> 
> I'm not sure, divulging the current Linux name of this feature bit is a
> good idea, but if everybody else is fine with this, I don't care that

Not sure if that feature name will ever change, as it is exported in
headers. At most, we might want to add the new ACCESS_PLATFORM define
and keep the old one, but that would still mean some churn.

> much. An alternative would be:
> "virtio: device falsely claims to have full access to the memory,
> aborting the device"

"virtio: device does not work with limited memory access" ?

But no issue with keeping the current message.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH] virtio-mem: Fix build error due to improper use 'select'

2020-06-19 Thread Michael S. Tsirkin
On Fri, Jun 19, 2020 at 04:03:33PM +0800, Weilong Chen wrote:
> As noted in:
> https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt
> "select should be used with care. select will force a symbol to a
> value without visiting the dependencies."
> Config VIRTIO_MEM should not select CONTIG_ALLOC directly.
> Otherwise it will cause an error:
> https://bugzilla.kernel.org/show_bug.cgi?id=208245
> 
> Signed-off-by: Weilong Chen 

Cc virtio mem maintainer:
M:  David Hildenbrand 


> ---
>  drivers/virtio/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 5809e5f5b157..5c92e4a50882 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -85,7 +85,7 @@ config VIRTIO_MEM
>   depends on VIRTIO
>   depends on MEMORY_HOTPLUG_SPARSE
>   depends on MEMORY_HOTREMOVE
> - select CONTIG_ALLOC
> + depends on CONTIG_ALLOC
>   help
>This driver provides access to virtio-mem paravirtualized memory
>devices, allowing to hotplug and hotunplug memory.
> -- 
> 2.17.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH] virtio-mem: Fix build error due to improper use 'select'

2020-06-19 Thread David Hildenbrand
On 19.06.20 10:03, Weilong Chen wrote:
> As noted in:
> https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt
> "select should be used with care. select will force a symbol to a
> value without visiting the dependencies."

Right, rings a bell.

> Config VIRTIO_MEM should not select CONTIG_ALLOC directly.
> Otherwise it will cause an error:
> https://bugzilla.kernel.org/show_bug.cgi?id=208245

Thanks!

Acked-by: David Hildenbrand 

> 
> Signed-off-by: Weilong Chen 
> ---
>  drivers/virtio/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 5809e5f5b157..5c92e4a50882 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -85,7 +85,7 @@ config VIRTIO_MEM
>   depends on VIRTIO
>   depends on MEMORY_HOTPLUG_SPARSE
>   depends on MEMORY_HOTREMOVE
> - select CONTIG_ALLOC
> + depends on CONTIG_ALLOC
>   help
>This driver provides access to virtio-mem paravirtualized memory
>devices, allowing to hotplug and hotunplug memory.
> 


-- 
Thanks,

David / dhildenb

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 1/1] s390: virtio: let arch accept devices without IOMMU feature

2020-06-19 Thread Halil Pasic
On Fri, 19 Jun 2020 11:20:51 +0200
Cornelia Huck  wrote:

> > > + if (arch_needs_virtio_iommu_platform(dev) &&
> > > + !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > > + dev_warn(&dev->dev,
> > > +  "virtio: device must provide 
> > > VIRTIO_F_IOMMU_PLATFORM\n");
> > 
> > I'm not sure, divulging the current Linux name of this feature bit is a
> > good idea, but if everybody else is fine with this, I don't care that  
> 
> Not sure if that feature name will ever change, as it is exported in
> headers. At most, we might want to add the new ACCESS_PLATFORM define
> and keep the old one, but that would still mean some churn.
> 
> > much. An alternative would be:
> > "virtio: device falsely claims to have full access to the memory,
> > aborting the device"  
> 
> "virtio: device does not work with limited memory access" ?
> 
> But no issue with keeping the current message.

I think I prefer Conny's version, but no strong feelings here.

Halil
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH] drivers\block: Use kobj_to_dev() API

2020-06-19 Thread Stefan Hajnoczi
On Fri, Jun 12, 2020 at 03:10:56PM +0800, Wang Qing wrote:
> Use kobj_to_dev() API instead of container_of().
> 
> Signed-off-by: Wang Qing 
> ---
>  drivers/block/virtio_blk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>  mode change 100644 => 100755 drivers/block/virtio_blk.c

Please fix the '\' -> '/' in the commit message. Looks good otherwise:

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] drm/virtio: Fix an IS_ERR() vs NULL check in virtio_gpu_object_shmem_init()

2020-06-19 Thread Dan Carpenter
The drm_gem_shmem_get_pages_sgt() function returns error pointers on
error, it never returns NULL.

Fixes: d323bb44e4d2 ("drm/virtio: Call the right shmem helpers")
Signed-off-by: Dan Carpenter 
---
 drivers/gpu/drm/virtio/virtgpu_object.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c 
b/drivers/gpu/drm/virtio/virtgpu_object.c
index 346cef5ce251..0cd5ecf4b3c0 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -151,9 +151,9 @@ static int virtio_gpu_object_shmem_init(struct 
virtio_gpu_device *vgdev,
return -EINVAL;
 
shmem->pages = drm_gem_shmem_get_pages_sgt(&bo->base.base);
-   if (!shmem->pages) {
+   if (IS_ERR(shmem->pages)) {
drm_gem_shmem_unpin(&bo->base.base);
-   return -EINVAL;
+   return PTR_ERR(shmem->pages);
}
 
if (use_dma_api) {
-- 
2.27.0

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH 4.14 049/190] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()

2020-06-19 Thread Greg Kroah-Hartman
From: Longpeng(Mike) 

[ Upstream commit 8c855f0720ff006d75d0a2512c7f6c4f60ff60ee ]

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: virtualization@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: sta...@vger.kernel.org
Link: https://lore.kernel.org/r/20200123101000.GB24255@Red
Acked-by: Gonglei 
Signed-off-by: Longpeng(Mike) 
Link: https://lore.kernel.org/r/20200602070501.2023-3-longpe...@huawei.com
Signed-off-by: Michael S. Tsirkin 
Signed-off-by: Sasha Levin 
---
 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 e2231a1a05a1..772d2b3137c6 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -569,10 +569,11 @@ static void virtio_crypto_ablkcipher_finalize_req(
struct ablkcipher_request *req,
int err)
 {
-   crypto_finalize_cipher_request(vc_sym_req->base.dataq->engine,
-   req, err);
kzfree(vc_sym_req->iv);
virtcrypto_clear_request(&vc_sym_req->base);
+
+   crypto_finalize_cipher_request(vc_sym_req->base.dataq->engine,
+  req, err);
 }
 
 static struct crypto_alg virtio_crypto_algs[] = { {
-- 
2.25.1



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH 4.14 050/190] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req()

2020-06-19 Thread Greg Kroah-Hartman
From: Longpeng(Mike) 

[ Upstream commit b02989f37fc5e865c9070907e4493b3a21e2 ]

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 
Cc: Herbert Xu 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
Cc: "David S. Miller" 
Cc: virtualization@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: sta...@vger.kernel.org
Link: https://lore.kernel.org/r/20200123101000.GB24255@Red
Signed-off-by: Gonglei 
Signed-off-by: Longpeng(Mike) 
Link: https://lore.kernel.org/r/20200602070501.2023-2-longpe...@huawei.com
Signed-off-by: Michael S. Tsirkin 
Signed-off-by: Sasha Levin 
---
 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 772d2b3137c6..fee78ec46bae 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -354,13 +354,18 @@ __virtio_crypto_ablkcipher_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->nbytes);
+   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",
@@ -441,12 +446,12 @@ __virtio_crypto_ablkcipher_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.25.1



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH 4.14 051/190] crypto: virtio: Fix dest length calculation in __virtio_crypto_skcipher_do_req()

2020-06-19 Thread Greg Kroah-Hartman
From: Longpeng(Mike) 

[ Upstream commit d90ca42012db2863a9a30b564a2ace6016594bda ]

The src/dst length is not aligned with AES_BLOCK_SIZE(which is 16) in some
testcases in tcrypto.ko.

For example, the src/dst length of one of cts(cbc(aes))'s testcase is 17, the
crypto_virtio driver will set @src_data_len=16 but @dst_data_len=17 in this
case and get a wrong at then end.

  SRC: pp pp pp pp pp pp pp pp pp pp pp pp pp pp pp pp pp (17 bytes)
  EXP: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc pp (17 bytes)
  DST: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc 00 (pollute the last 
bytes)
  (pp: plaintext  cc:ciphertext)

Fix this issue by limit the length of dest buffer.

Fixes: dbaf0624ffa5 ("crypto: add virtio-crypto driver")
Cc: Gonglei 
Cc: Herbert Xu 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
Cc: "David S. Miller" 
Cc: virtualization@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: sta...@vger.kernel.org
Signed-off-by: Longpeng(Mike) 
Link: https://lore.kernel.org/r/20200602070501.2023-4-longpe...@huawei.com
Signed-off-by: Michael S. Tsirkin 
Signed-off-by: Sasha Levin 
---
 drivers/crypto/virtio/virtio_crypto_algs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c 
b/drivers/crypto/virtio/virtio_crypto_algs.c
index fee78ec46bae..e6b889ce395e 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -411,6 +411,7 @@ __virtio_crypto_ablkcipher_do_req(struct 
virtio_crypto_sym_request *vc_sym_req,
goto free;
}
 
+   dst_len = min_t(unsigned int, req->nbytes, dst_len);
pr_debug("virtio_crypto: src_len: %u, dst_len: %llu\n",
req->nbytes, dst_len);
 
-- 
2.25.1



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH 4.19 066/267] crypto: virtio: Fix dest length calculation in __virtio_crypto_skcipher_do_req()

2020-06-19 Thread Greg Kroah-Hartman
From: Longpeng(Mike) 

[ Upstream commit d90ca42012db2863a9a30b564a2ace6016594bda ]

The src/dst length is not aligned with AES_BLOCK_SIZE(which is 16) in some
testcases in tcrypto.ko.

For example, the src/dst length of one of cts(cbc(aes))'s testcase is 17, the
crypto_virtio driver will set @src_data_len=16 but @dst_data_len=17 in this
case and get a wrong at then end.

  SRC: pp pp pp pp pp pp pp pp pp pp pp pp pp pp pp pp pp (17 bytes)
  EXP: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc pp (17 bytes)
  DST: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc 00 (pollute the last 
bytes)
  (pp: plaintext  cc:ciphertext)

Fix this issue by limit the length of dest buffer.

Fixes: dbaf0624ffa5 ("crypto: add virtio-crypto driver")
Cc: Gonglei 
Cc: Herbert Xu 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
Cc: "David S. Miller" 
Cc: virtualization@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: sta...@vger.kernel.org
Signed-off-by: Longpeng(Mike) 
Link: https://lore.kernel.org/r/20200602070501.2023-4-longpe...@huawei.com
Signed-off-by: Michael S. Tsirkin 
Signed-off-by: Sasha Levin 
---
 drivers/crypto/virtio/virtio_crypto_algs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c 
b/drivers/crypto/virtio/virtio_crypto_algs.c
index e9a8485c4929..ab4700e4b409 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -424,6 +424,7 @@ __virtio_crypto_ablkcipher_do_req(struct 
virtio_crypto_sym_request *vc_sym_req,
goto free;
}
 
+   dst_len = min_t(unsigned int, req->nbytes, dst_len);
pr_debug("virtio_crypto: src_len: %u, dst_len: %llu\n",
req->nbytes, dst_len);
 
-- 
2.25.1



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH 4.19 064/267] crypto: virtio: Fix use-after-free in virtio_crypto_skcipher_finalize_req()

2020-06-19 Thread Greg Kroah-Hartman
From: Longpeng(Mike) 

[ Upstream commit 8c855f0720ff006d75d0a2512c7f6c4f60ff60ee ]

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: virtualization@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: sta...@vger.kernel.org
Link: https://lore.kernel.org/r/20200123101000.GB24255@Red
Acked-by: Gonglei 
Signed-off-by: Longpeng(Mike) 
Link: https://lore.kernel.org/r/20200602070501.2023-3-longpe...@huawei.com
Signed-off-by: Michael S. Tsirkin 
Signed-off-by: Sasha Levin 
---
 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 38432721069f..9348060cc32f 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -594,10 +594,11 @@ static void virtio_crypto_ablkcipher_finalize_req(
scatterwalk_map_and_copy(req->info, req->dst,
 req->nbytes - AES_BLOCK_SIZE,
 AES_BLOCK_SIZE, 0);
-   crypto_finalize_ablkcipher_request(vc_sym_req->base.dataq->engine,
-  req, err);
kzfree(vc_sym_req->iv);
virtcrypto_clear_request(&vc_sym_req->base);
+
+   crypto_finalize_ablkcipher_request(vc_sym_req->base.dataq->engine,
+  req, err);
 }
 
 static struct virtio_crypto_algo virtio_crypto_algs[] = { {
-- 
2.25.1



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH 4.19 065/267] crypto: virtio: Fix src/dst scatterlist calculation in __virtio_crypto_skcipher_do_req()

2020-06-19 Thread Greg Kroah-Hartman
From: Longpeng(Mike) 

[ Upstream commit b02989f37fc5e865c9070907e4493b3a21e2 ]

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 
Cc: Herbert Xu 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
Cc: "David S. Miller" 
Cc: virtualization@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: sta...@vger.kernel.org
Link: https://lore.kernel.org/r/20200123101000.GB24255@Red
Signed-off-by: Gonglei 
Signed-off-by: Longpeng(Mike) 
Link: https://lore.kernel.org/r/20200602070501.2023-2-longpe...@huawei.com
Signed-off-by: Michael S. Tsirkin 
Signed-off-by: Sasha Levin 
---
 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 9348060cc32f..e9a8485c4929 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -367,13 +367,18 @@ __virtio_crypto_ablkcipher_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->nbytes);
+   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",
@@ -459,12 +464,12 @@ __virtio_crypto_ablkcipher_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.25.1



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization