[PATCH v10 1/1] crypto: Introduce RSA algorithm

2022-06-10 Thread zhenwei pi
There are two parts in this patch:
1, support akcipher service by cryptodev-builtin driver
2, virtio-crypto driver supports akcipher service

In principle, we should separate this into two patches, to avoid
compiling error, merge them into one.

Then virtio-crypto gets request from guest side, and forwards the
request to builtin driver to handle it.

Test with a guest linux:
1, The self-test framework of crypto layer works fine in guest kernel
2, Test with Linux guest(with asym support), the following script
test(note that pkey_XXX is supported only in a newer version of keyutils):
  - both public key & private key
  - create/close session
  - encrypt/decrypt/sign/verify basic driver operation
  - also test with kernel crypto layer(pkey add/query)

All the cases work fine.

Run script in guest:
rm -rf *.der *.pem *.pfx
modprobe pkcs8_key_parser # if CONFIG_PKCS8_PRIVATE_KEY_PARSER=m
rm -rf /tmp/data
dd if=/dev/random of=/tmp/data count=1 bs=20

openssl req -nodes -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -subj 
"/C=CN/ST=BJ/L=HD/O=qemu/OU=dev/CN=qemu/emailAddress=q...@qemu.org"
openssl pkcs8 -in key.pem -topk8 -nocrypt -outform DER -out key.der
openssl x509 -in cert.pem -inform PEM -outform DER -out cert.der

PRIV_KEY_ID=`cat key.der | keyctl padd asymmetric test_priv_key @s`
echo "priv key id = "$PRIV_KEY_ID
PUB_KEY_ID=`cat cert.der | keyctl padd asymmetric test_pub_key @s`
echo "pub key id = "$PUB_KEY_ID

keyctl pkey_query $PRIV_KEY_ID 0
keyctl pkey_query $PUB_KEY_ID 0

echo "Enc with priv key..."
keyctl pkey_encrypt $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.priv
echo "Dec with pub key..."
keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.priv enc=pkcs1 >/tmp/dec
cmp /tmp/data /tmp/dec

echo "Sign with priv key..."
keyctl pkey_sign $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 hash=sha1 > /tmp/sig
echo "Verify with pub key..."
keyctl pkey_verify $PRIV_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1

echo "Enc with pub key..."
keyctl pkey_encrypt $PUB_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.pub
echo "Dec with priv key..."
keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.pub enc=pkcs1 >/tmp/dec
cmp /tmp/data /tmp/dec

echo "Verify with pub key..."
keyctl pkey_verify $PUB_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1

Reviewed-by: Gonglei 
Signed-off-by: lei he 
---
 backends/cryptodev-builtin.c  | 276 +
 backends/cryptodev-vhost-user.c   |  34 +++-
 backends/cryptodev.c  |  32 ++-
 hw/virtio/virtio-crypto.c | 323 --
 include/hw/virtio/virtio-crypto.h |   5 +-
 include/sysemu/cryptodev.h|  83 ++--
 6 files changed, 609 insertions(+), 144 deletions(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index 0671bf9f3e..125cbad1d3 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "standard-headers/linux/virtio_crypto.h"
 #include "crypto/cipher.h"
+#include "crypto/akcipher.h"
 #include "qom/object.h"
 
 
@@ -42,10 +43,11 @@ typedef struct CryptoDevBackendBuiltinSession {
 QCryptoCipher *cipher;
 uint8_t direction; /* encryption or decryption */
 uint8_t type; /* cipher? hash? aead? */
+QCryptoAkCipher *akcipher;
 QTAILQ_ENTRY(CryptoDevBackendBuiltinSession) next;
 } CryptoDevBackendBuiltinSession;
 
-/* Max number of symmetric sessions */
+/* Max number of symmetric/asymmetric sessions */
 #define MAX_NUM_SESSIONS 256
 
 #define CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN512
@@ -80,15 +82,17 @@ static void cryptodev_builtin_init(
 backend->conf.crypto_services =
  1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
  1u << VIRTIO_CRYPTO_SERVICE_HASH |
- 1u << VIRTIO_CRYPTO_SERVICE_MAC;
+ 1u << VIRTIO_CRYPTO_SERVICE_MAC |
+ 1u << VIRTIO_CRYPTO_SERVICE_AKCIPHER;
 backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
 backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
+backend->conf.akcipher_algo = 1u << VIRTIO_CRYPTO_AKCIPHER_RSA;
 /*
  * Set the Maximum length of crypto request.
  * Why this value? Just avoid to overflow when
  * memory allocation for each crypto request.
  */
-backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendSymOpInfo);
+backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendOpInfo);
 backend->conf.max_cipher_key_len = CRYPTODEV_BUITLIN_MAX_CIPHER_KEY_LEN;
 backend->conf.max_auth_key_len = CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN;
 
@@ -148,6 +152,55 @@ err:
return -1;
 }
 
+static int cryptodev_builtin_get_rsa_hash_algo(
+int virtio_rsa_hash, Error **errp)
+{
+switch (virtio_rsa_hash) {
+case VIRTIO_CRYPTO_RSA_MD5:
+return QCRYPTO_HASH_ALG_MD5;
+
+case VIRTIO_CRYPTO_RSA_SHA1:
+return QCRYPTO_HASH_ALG_SHA1;
+
+case VIRTIO_CRYPTO_RSA_SHA256:
+return QCRYPTO_HASH_ALG_SHA2

[PATCH v10 0/1] Introduce akcipher service for virtio-crypto

2022-06-10 Thread zhenwei pi
v9 - v10:
- Minor fix of coding style by v9.

v8 - v9:
- Fix compiling error reported by clang-13/14:
  opt->hash_alg = cryptodev_builtin_get_rsa_hash_algo(); this leads
  implicit convertion from 'int' to 'uint32'. 'if (opt->hash_alg < 0)'
  is always false. Thanks to Philippe Mathieu-Daudé.

v7 - v8:
- The changes of QEMU crypto has been reviewed & merged by Daniel,
  remove this part from this series. Thanks to Daniel!
- virtio_crypto.h is updated by e4082063e47e
  ("linux-headers: Update to v5.18-rc6"), remove from this series.
- Minor fixes reviewed by Gonglei. Thanks to Gonglei!

v6 -> v7:
- Fix serval build errors for some specific platforms/configurations.
- Use '%zu' instead of '%lu' for size_t parameters.
- AkCipher-gcrypt: avoid setting wrong error messages when parsing RSA
  keys.
- AkCipher-benchmark: process constant amount of sign/verify instead
 of running sign/verify for a constant duration.

v5 -> v6:
- Fix build errors and codestyles.
- Add parameter 'Error **errp' for qcrypto_akcipher_rsakey_parse.
- Report more detailed errors.
- Fix buffer length check and return values of akcipher-nettle, allows caller to
 pass a buffer with larger size than actual needed.

A million thanks to Daniel!

v4 -> v5:
- Move QCryptoAkCipher into akcipherpriv.h, and modify the related comments.
- Rename asn1_decoder.c to der.c.
- Code style fix: use 'cleanup' & 'error' lables.
- Allow autoptr type to auto-free.
- Add test cases for rsakey to handle DER error.
- Other minor fixes.

v3 -> v4:
- Coding style fix: Akcipher -> AkCipher, struct XXX -> XXX, Rsa -> RSA,
XXX-alg -> XXX-algo.
- Change version info in qapi/crypto.json, from 7.0 -> 7.1.
- Remove ecdsa from qapi/crypto.json, it would be introduced with the 
implemetion later.
- Use QCryptoHashAlgothrim instead of QCryptoRSAHashAlgorithm(removed) in 
qapi/crypto.json.
- Rename arguments of qcrypto_akcipher_XXX to keep aligned with 
qcrypto_cipher_XXX(dec/enc/sign/vefiry -> in/out/in2), and add 
qcrypto_akcipher_max_XXX APIs.
- Add new API: qcrypto_akcipher_supports.
- Change the return value of qcrypto_akcipher_enc/dec/sign, these functions 
return the actual length of result.
- Separate ASN.1 source code and test case clean.
- Disable RSA raw encoding for akcipher-nettle.
- Separate RSA key parser into rsakey.{hc}, and implememts it with 
builtin-asn1-decoder and nettle respectivly.
- Implement RSA(pkcs1 and raw encoding) algorithm by gcrypt. This has higher 
priority than nettle.
- For some akcipher operations(eg, decryption of pkcs1pad(rsa)), the length of 
returned result maybe less than the dst buffer size, return the actual length 
of result instead of the buffer length to the guest side. (in function 
virtio_crypto_akcipher_input_data_helper)
- Other minor changes.

Thanks to Daniel!

Eric pointed out this missing part of use case, send it here again.

In our plan, the feature is designed for HTTPS offloading case and other 
applications which use kernel RSA/ecdsa by keyctl syscall. The full picture 
shows bellow:


 Nginx/openssl[1] ... Apps
Guest   -
  virtio-crypto driver[2]
-
  virtio-crypto backend[3]
Host-
 /  |  \
 builtin[4]   vhost keyctl[5] ...


[1] User applications can offload RSA calculation to kernel by keyctl syscall. 
There is no keyctl engine in openssl currently, we developed a engine and tried 
to contribute it to openssl upstream, but openssl 1.x does not accept new 
feature. Link:
   https://github.com/openssl/openssl/pull/16689

This branch is available and maintained by Lei 
   https://github.com/TousakaRin/openssl/tree/OpenSSL_1_1_1-kctl_engine

We tested nginx(change config file only) with openssl keyctl engine, it works 
fine.

[2] virtio-crypto driver is used to communicate with host side, send requests 
to host side to do asymmetric calculation.
   https://lkml.org/lkml/2022/3/1/1425

[3] virtio-crypto backend handles requests from guest side, and forwards 
request to crypto backend driver of QEMU.

[4] Currently RSA is supported only in builtin driver. This driver is supposed 
to test the full feature without other software(Ex vhost process) and hardware 
dependence. ecdsa is introduced into qapi type without implementation, this may 
be implemented in Q3-2022 or later. If ecdsa type definition should be added 
with the implementation together, I'll remove this in next version.

[5] keyctl backend is in development, we will post this feature in Q2-2022. 
keyctl backend can use hardware acceleration(Ex, Intel QAT).

Setup the full environment, tested with Intel QAT on host side, the QPS of 
HTTPS increase to ~200% in a guest.

VS PCI passthrough: the most important benefit of this solution makes the VM 
migratable.

v2 -> v3:
- Introduce akcipher types to qapi
- Add test/benchmark suite for akci

Re: [PATCH V6 8/9] virtio: harden vring IRQ

2022-06-10 Thread Michael S. Tsirkin
On Fri, May 27, 2022 at 02:01:19PM +0800, Jason Wang wrote:
> This is a rework on the previous IRQ hardening that is done for
> virtio-pci where several drawbacks were found and were reverted:
> 
> 1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ
>that is used by some device such as virtio-blk
> 2) done only for PCI transport
> 
> The vq->broken is re-used in this patch for implementing the IRQ
> hardening. The vq->broken is set to true during both initialization
> and reset. And the vq->broken is set to false in
> virtio_device_ready(). Then vring_interrupt() can check and return
> when vq->broken is true. And in this case, switch to return IRQ_NONE
> to let the interrupt core aware of such invalid interrupt to prevent
> IRQ storm.
> 
> The reason of using a per queue variable instead of a per device one
> is that we may need it for per queue reset hardening in the future.
> 
> Note that the hardening is only done for vring interrupt since the
> config interrupt hardening is already done in commit 22b7050a024d7
> ("virtio: defer config changed notifications"). But the method that is
> used by config interrupt can't be reused by the vring interrupt
> handler because it uses spinlock to do the synchronization which is
> expensive.
> 
> Cc: Thomas Gleixner 
> Cc: Peter Zijlstra 
> Cc: "Paul E. McKenney" 
> Cc: Marc Zyngier 
> Cc: Halil Pasic 
> Cc: Cornelia Huck 
> Cc: Vineeth Vijayan 
> Cc: Peter Oberparleiter 
> Cc: linux-s...@vger.kernel.org
> Signed-off-by: Jason Wang 


Jason, I am really concerned by all the fallout.
I propose adding a flag to suppress the hardening -
this will be a debugging aid and a work around for
users if we find more buggy drivers.

suppress_interrupt_hardening ?


> ---
>  drivers/s390/virtio/virtio_ccw.c   |  4 
>  drivers/virtio/virtio.c| 15 ---
>  drivers/virtio/virtio_mmio.c   |  5 +
>  drivers/virtio/virtio_pci_modern_dev.c |  5 +
>  drivers/virtio/virtio_ring.c   | 11 +++
>  include/linux/virtio_config.h  | 20 
>  6 files changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/s390/virtio/virtio_ccw.c 
> b/drivers/s390/virtio/virtio_ccw.c
> index c188e4f20ca3..97e51c34e6cf 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -971,6 +971,10 @@ static void virtio_ccw_set_status(struct virtio_device 
> *vdev, u8 status)
>   ccw->flags = 0;
>   ccw->count = sizeof(status);
>   ccw->cda = (__u32)(unsigned long)&vcdev->dma_area->status;
> + /* We use ssch for setting the status which is a serializing
> +  * instruction that guarantees the memory writes have
> +  * completed before ssch.
> +  */
>   ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_STATUS);
>   /* Write failed? We assume status is unchanged. */
>   if (ret)
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index aa1eb5132767..95fac4c97c8b 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -220,6 +220,15 @@ static int virtio_features_ok(struct virtio_device *dev)
>   * */
>  void virtio_reset_device(struct virtio_device *dev)
>  {
> + /*
> +  * The below virtio_synchronize_cbs() guarantees that any
> +  * interrupt for this line arriving after
> +  * virtio_synchronize_vqs() has completed is guaranteed to see
> +  * vq->broken as true.
> +  */
> + virtio_break_device(dev);

So make this conditional

> + virtio_synchronize_cbs(dev);
> +
>   dev->config->reset(dev);
>  }
>  EXPORT_SYMBOL_GPL(virtio_reset_device);
> @@ -428,6 +437,9 @@ int register_virtio_device(struct virtio_device *dev)
>   dev->config_enabled = false;
>   dev->config_change_pending = false;
>  
> + INIT_LIST_HEAD(&dev->vqs);
> + spin_lock_init(&dev->vqs_list_lock);
> +
>   /* We always start by resetting the device, in case a previous
>* driver messed it up.  This also tests that code path a little. */
>   virtio_reset_device(dev);
> @@ -435,9 +447,6 @@ int register_virtio_device(struct virtio_device *dev)
>   /* Acknowledge that we've seen the device. */
>   virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>  
> - INIT_LIST_HEAD(&dev->vqs);
> - spin_lock_init(&dev->vqs_list_lock);
> -
>   /*
>* device_add() causes the bus infrastructure to look for a matching
>* driver.
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index c9699a59f93c..f9a36bc7ac27 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -253,6 +253,11 @@ static void vm_set_status(struct virtio_device *vdev, u8 
> status)
>   /* We should never be setting status to 0. */
>   BUG_ON(status == 0);
>  
> + /*
> +  * Per memory-barriers.txt, wmb() is not needed to guarantee
> +  * that the the cache coherent memory writes have completed
> 

Re: [PATCH v9 1/1] crypto: Introduce RSA algorithm

2022-06-10 Thread Michael S. Tsirkin
On Sat, Jun 11, 2022 at 11:36:58AM +0800, zhenwei pi wrote:
> There are two parts in this patch:
> 1, support akcipher service by cryptodev-builtin driver
> 2, virtio-crypto driver supports akcipher service
> 
> In principle, we should separate this into two patches, to avoid
> compiling error, merge them into one.
> 
> Then virtio-crypto gets request from guest side, and forwards the
> request to builtin driver to handle it.
> 
> Test with a guest linux:
> 1, The self-test framework of crypto layer works fine in guest kernel
> 2, Test with Linux guest(with asym support), the following script
> test(note that pkey_XXX is supported only in a newer version of keyutils):
>   - both public key & private key
>   - create/close session
>   - encrypt/decrypt/sign/verify basic driver operation
>   - also test with kernel crypto layer(pkey add/query)
> 
> All the cases work fine.
> 
> Run script in guest:
> rm -rf *.der *.pem *.pfx
> modprobe pkcs8_key_parser # if CONFIG_PKCS8_PRIVATE_KEY_PARSER=m
> rm -rf /tmp/data
> dd if=/dev/random of=/tmp/data count=1 bs=20
> 
> openssl req -nodes -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -subj 
> "/C=CN/ST=BJ/L=HD/O=qemu/OU=dev/CN=qemu/emailAddress=q...@qemu.org"
> openssl pkcs8 -in key.pem -topk8 -nocrypt -outform DER -out key.der
> openssl x509 -in cert.pem -inform PEM -outform DER -out cert.der
> 
> PRIV_KEY_ID=`cat key.der | keyctl padd asymmetric test_priv_key @s`
> echo "priv key id = "$PRIV_KEY_ID
> PUB_KEY_ID=`cat cert.der | keyctl padd asymmetric test_pub_key @s`
> echo "pub key id = "$PUB_KEY_ID
> 
> keyctl pkey_query $PRIV_KEY_ID 0
> keyctl pkey_query $PUB_KEY_ID 0
> 
> echo "Enc with priv key..."
> keyctl pkey_encrypt $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.priv
> echo "Dec with pub key..."
> keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.priv enc=pkcs1 >/tmp/dec
> cmp /tmp/data /tmp/dec
> 
> echo "Sign with priv key..."
> keyctl pkey_sign $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 hash=sha1 > /tmp/sig
> echo "Verify with pub key..."
> keyctl pkey_verify $PRIV_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1
> 
> echo "Enc with pub key..."
> keyctl pkey_encrypt $PUB_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.pub
> echo "Dec with priv key..."
> keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.pub enc=pkcs1 >/tmp/dec
> cmp /tmp/data /tmp/dec
> 
> echo "Verify with pub key..."
> keyctl pkey_verify $PUB_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1
> 
> Reviewed-by: Gonglei 
> Signed-off-by: lei he  Signed-off-by: zhenwei pi 
> ---
>  backends/cryptodev-builtin.c  | 275 +
>  backends/cryptodev-vhost-user.c   |  34 +++-
>  backends/cryptodev.c  |  32 ++-
>  hw/virtio/virtio-crypto.c | 323 --
>  include/hw/virtio/virtio-crypto.h |   5 +-
>  include/sysemu/cryptodev.h|  83 ++--
>  6 files changed, 608 insertions(+), 144 deletions(-)
> 
> diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
> index 0671bf9f3e..ed73ea789b 100644
> --- a/backends/cryptodev-builtin.c
> +++ b/backends/cryptodev-builtin.c
> @@ -26,6 +26,7 @@
>  #include "qapi/error.h"
>  #include "standard-headers/linux/virtio_crypto.h"
>  #include "crypto/cipher.h"
> +#include "crypto/akcipher.h"
>  #include "qom/object.h"
>  
>  
> @@ -42,10 +43,11 @@ typedef struct CryptoDevBackendBuiltinSession {
>  QCryptoCipher *cipher;
>  uint8_t direction; /* encryption or decryption */
>  uint8_t type; /* cipher? hash? aead? */
> +QCryptoAkCipher *akcipher;
>  QTAILQ_ENTRY(CryptoDevBackendBuiltinSession) next;
>  } CryptoDevBackendBuiltinSession;
>  
> -/* Max number of symmetric sessions */
> +/* Max number of symmetric/asymmetric sessions */
>  #define MAX_NUM_SESSIONS 256
>  
>  #define CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN512
> @@ -80,15 +82,17 @@ static void cryptodev_builtin_init(
>  backend->conf.crypto_services =
>   1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
>   1u << VIRTIO_CRYPTO_SERVICE_HASH |
> - 1u << VIRTIO_CRYPTO_SERVICE_MAC;
> + 1u << VIRTIO_CRYPTO_SERVICE_MAC |
> + 1u << VIRTIO_CRYPTO_SERVICE_AKCIPHER;
>  backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
>  backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> +backend->conf.akcipher_algo = 1u << VIRTIO_CRYPTO_AKCIPHER_RSA;
>  /*
>   * Set the Maximum length of crypto request.
>   * Why this value? Just avoid to overflow when
>   * memory allocation for each crypto request.
>   */
> -backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendSymOpInfo);
> +backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendOpInfo);
>  backend->conf.max_cipher_key_len = CRYPTODEV_BUITLIN_MAX_CIPHER_KEY_LEN;
>  backend->conf.max_auth_key_len = CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN;
>  
> @@ -148,6 +152,54 @@ err:
> return -1;
>  }
>  
> +static int cryp

[PATCH v9 1/1] crypto: Introduce RSA algorithm

2022-06-10 Thread zhenwei pi
There are two parts in this patch:
1, support akcipher service by cryptodev-builtin driver
2, virtio-crypto driver supports akcipher service

In principle, we should separate this into two patches, to avoid
compiling error, merge them into one.

Then virtio-crypto gets request from guest side, and forwards the
request to builtin driver to handle it.

Test with a guest linux:
1, The self-test framework of crypto layer works fine in guest kernel
2, Test with Linux guest(with asym support), the following script
test(note that pkey_XXX is supported only in a newer version of keyutils):
  - both public key & private key
  - create/close session
  - encrypt/decrypt/sign/verify basic driver operation
  - also test with kernel crypto layer(pkey add/query)

All the cases work fine.

Run script in guest:
rm -rf *.der *.pem *.pfx
modprobe pkcs8_key_parser # if CONFIG_PKCS8_PRIVATE_KEY_PARSER=m
rm -rf /tmp/data
dd if=/dev/random of=/tmp/data count=1 bs=20

openssl req -nodes -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -subj 
"/C=CN/ST=BJ/L=HD/O=qemu/OU=dev/CN=qemu/emailAddress=q...@qemu.org"
openssl pkcs8 -in key.pem -topk8 -nocrypt -outform DER -out key.der
openssl x509 -in cert.pem -inform PEM -outform DER -out cert.der

PRIV_KEY_ID=`cat key.der | keyctl padd asymmetric test_priv_key @s`
echo "priv key id = "$PRIV_KEY_ID
PUB_KEY_ID=`cat cert.der | keyctl padd asymmetric test_pub_key @s`
echo "pub key id = "$PUB_KEY_ID

keyctl pkey_query $PRIV_KEY_ID 0
keyctl pkey_query $PUB_KEY_ID 0

echo "Enc with priv key..."
keyctl pkey_encrypt $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.priv
echo "Dec with pub key..."
keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.priv enc=pkcs1 >/tmp/dec
cmp /tmp/data /tmp/dec

echo "Sign with priv key..."
keyctl pkey_sign $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 hash=sha1 > /tmp/sig
echo "Verify with pub key..."
keyctl pkey_verify $PRIV_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1

echo "Enc with pub key..."
keyctl pkey_encrypt $PUB_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.pub
echo "Dec with priv key..."
keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.pub enc=pkcs1 >/tmp/dec
cmp /tmp/data /tmp/dec

echo "Verify with pub key..."
keyctl pkey_verify $PUB_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1

Reviewed-by: Gonglei 
Signed-off-by: lei he 
---
 backends/cryptodev-builtin.c  | 275 +
 backends/cryptodev-vhost-user.c   |  34 +++-
 backends/cryptodev.c  |  32 ++-
 hw/virtio/virtio-crypto.c | 323 --
 include/hw/virtio/virtio-crypto.h |   5 +-
 include/sysemu/cryptodev.h|  83 ++--
 6 files changed, 608 insertions(+), 144 deletions(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index 0671bf9f3e..ed73ea789b 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "standard-headers/linux/virtio_crypto.h"
 #include "crypto/cipher.h"
+#include "crypto/akcipher.h"
 #include "qom/object.h"
 
 
@@ -42,10 +43,11 @@ typedef struct CryptoDevBackendBuiltinSession {
 QCryptoCipher *cipher;
 uint8_t direction; /* encryption or decryption */
 uint8_t type; /* cipher? hash? aead? */
+QCryptoAkCipher *akcipher;
 QTAILQ_ENTRY(CryptoDevBackendBuiltinSession) next;
 } CryptoDevBackendBuiltinSession;
 
-/* Max number of symmetric sessions */
+/* Max number of symmetric/asymmetric sessions */
 #define MAX_NUM_SESSIONS 256
 
 #define CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN512
@@ -80,15 +82,17 @@ static void cryptodev_builtin_init(
 backend->conf.crypto_services =
  1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
  1u << VIRTIO_CRYPTO_SERVICE_HASH |
- 1u << VIRTIO_CRYPTO_SERVICE_MAC;
+ 1u << VIRTIO_CRYPTO_SERVICE_MAC |
+ 1u << VIRTIO_CRYPTO_SERVICE_AKCIPHER;
 backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
 backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
+backend->conf.akcipher_algo = 1u << VIRTIO_CRYPTO_AKCIPHER_RSA;
 /*
  * Set the Maximum length of crypto request.
  * Why this value? Just avoid to overflow when
  * memory allocation for each crypto request.
  */
-backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendSymOpInfo);
+backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendOpInfo);
 backend->conf.max_cipher_key_len = CRYPTODEV_BUITLIN_MAX_CIPHER_KEY_LEN;
 backend->conf.max_auth_key_len = CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN;
 
@@ -148,6 +152,54 @@ err:
return -1;
 }
 
+static int cryptodev_builtin_get_rsa_hash_algo(
+int virtio_rsa_hash, Error **errp)
+{
+switch (virtio_rsa_hash) {
+case VIRTIO_CRYPTO_RSA_MD5:
+return QCRYPTO_HASH_ALG_MD5;
+
+case VIRTIO_CRYPTO_RSA_SHA1:
+return QCRYPTO_HASH_ALG_SHA1;
+
+case VIRTIO_CRYPTO_RSA_SHA256:
+return QCRYPTO_HASH_ALG_SHA2

[PATCH v9 0/1] Introduce akcipher service for virtio-crypto

2022-06-10 Thread zhenwei pi
v8 - v9:
- Fix compiling error reported by clang-13/14:
  opt->hash_alg = cryptodev_builtin_get_rsa_hash_algo(); this leads
  implicit convertion from 'int' to 'uint32'. 'if (opt->hash_alg < 0)'
  is always false. Thanks to Philippe Mathieu-Daudé.

v7 - v8:
- The changes of QEMU crypto has been reviewed & merged by Daniel,
  remove this part from this series. Thanks to Daniel!
- virtio_crypto.h is updated by e4082063e47e
  ("linux-headers: Update to v5.18-rc6"), remove from this series.
- Minor fixes reviewed by Gonglei. Thanks to Gonglei!

v6 -> v7:
- Fix serval build errors for some specific platforms/configurations.
- Use '%zu' instead of '%lu' for size_t parameters.
- AkCipher-gcrypt: avoid setting wrong error messages when parsing RSA
  keys.
- AkCipher-benchmark: process constant amount of sign/verify instead
 of running sign/verify for a constant duration.

v5 -> v6:
- Fix build errors and codestyles.
- Add parameter 'Error **errp' for qcrypto_akcipher_rsakey_parse.
- Report more detailed errors.
- Fix buffer length check and return values of akcipher-nettle, allows caller to
 pass a buffer with larger size than actual needed.

A million thanks to Daniel!

v4 -> v5:
- Move QCryptoAkCipher into akcipherpriv.h, and modify the related comments.
- Rename asn1_decoder.c to der.c.
- Code style fix: use 'cleanup' & 'error' lables.
- Allow autoptr type to auto-free.
- Add test cases for rsakey to handle DER error.
- Other minor fixes.

v3 -> v4:
- Coding style fix: Akcipher -> AkCipher, struct XXX -> XXX, Rsa -> RSA,
XXX-alg -> XXX-algo.
- Change version info in qapi/crypto.json, from 7.0 -> 7.1.
- Remove ecdsa from qapi/crypto.json, it would be introduced with the 
implemetion later.
- Use QCryptoHashAlgothrim instead of QCryptoRSAHashAlgorithm(removed) in 
qapi/crypto.json.
- Rename arguments of qcrypto_akcipher_XXX to keep aligned with 
qcrypto_cipher_XXX(dec/enc/sign/vefiry -> in/out/in2), and add 
qcrypto_akcipher_max_XXX APIs.
- Add new API: qcrypto_akcipher_supports.
- Change the return value of qcrypto_akcipher_enc/dec/sign, these functions 
return the actual length of result.
- Separate ASN.1 source code and test case clean.
- Disable RSA raw encoding for akcipher-nettle.
- Separate RSA key parser into rsakey.{hc}, and implememts it with 
builtin-asn1-decoder and nettle respectivly.
- Implement RSA(pkcs1 and raw encoding) algorithm by gcrypt. This has higher 
priority than nettle.
- For some akcipher operations(eg, decryption of pkcs1pad(rsa)), the length of 
returned result maybe less than the dst buffer size, return the actual length 
of result instead of the buffer length to the guest side. (in function 
virtio_crypto_akcipher_input_data_helper)
- Other minor changes.

Thanks to Daniel!

Eric pointed out this missing part of use case, send it here again.

In our plan, the feature is designed for HTTPS offloading case and other 
applications which use kernel RSA/ecdsa by keyctl syscall. The full picture 
shows bellow:


 Nginx/openssl[1] ... Apps
Guest   -
  virtio-crypto driver[2]
-
  virtio-crypto backend[3]
Host-
 /  |  \
 builtin[4]   vhost keyctl[5] ...


[1] User applications can offload RSA calculation to kernel by keyctl syscall. 
There is no keyctl engine in openssl currently, we developed a engine and tried 
to contribute it to openssl upstream, but openssl 1.x does not accept new 
feature. Link:
   https://github.com/openssl/openssl/pull/16689

This branch is available and maintained by Lei 
   https://github.com/TousakaRin/openssl/tree/OpenSSL_1_1_1-kctl_engine

We tested nginx(change config file only) with openssl keyctl engine, it works 
fine.

[2] virtio-crypto driver is used to communicate with host side, send requests 
to host side to do asymmetric calculation.
   https://lkml.org/lkml/2022/3/1/1425

[3] virtio-crypto backend handles requests from guest side, and forwards 
request to crypto backend driver of QEMU.

[4] Currently RSA is supported only in builtin driver. This driver is supposed 
to test the full feature without other software(Ex vhost process) and hardware 
dependence. ecdsa is introduced into qapi type without implementation, this may 
be implemented in Q3-2022 or later. If ecdsa type definition should be added 
with the implementation together, I'll remove this in next version.

[5] keyctl backend is in development, we will post this feature in Q2-2022. 
keyctl backend can use hardware acceleration(Ex, Intel QAT).

Setup the full environment, tested with Intel QAT on host side, the QPS of 
HTTPS increase to ~200% in a guest.

VS PCI passthrough: the most important benefit of this solution makes the VM 
migratable.

v2 -> v3:
- Introduce akcipher types to qapi
- Add test/benchmark suite for akcipher class
- Seperate 'virtio_crypto: Support 

Re: [External] Re: [PATCH] virtio_ring : fix vring_packed_desc memory out of bounds bug

2022-06-10 Thread Michael S. Tsirkin
On Sat, Jun 11, 2022 at 12:38:10AM +0800, 黄杰 wrote:
> > This pattern was always iffy, but I don't think the patch
> > improves the situation much. last_used_idx and vq->packed.used_wrap_counter
> > can still get out of sync.
> 
> Yes, You are absolutely correct, thanks for pointing out this issue, I
> didn't take that into consideration,
> how about disabling interrupts before this code below:
> 
> > vq->last_used_idx += vq->packed.desc_state[id].num;
> > if (unlikely(vq->last_used_idx >= vq->packed.vring.num)) {
> >  vq->last_used_idx -= vq->packed.vring.num;
> >  vq->packed.used_wrap_counter ^= 1;
> > }
> 
> it seems to be fine to just turn off the interrupts of the current vring.
> 
> BR

That would make datapath significantly slower.

> 
> Michael S. Tsirkin  于2022年6月10日周五 22:50写道:
> >
> > On Fri, Jun 10, 2022 at 06:33:14PM +0800, huangjie.albert wrote:
> > > ksoftirqd may consume the packet and it will call:
> > > virtnet_poll
> > >   -->virtnet_receive
> > >   -->virtqueue_get_buf_ctx
> > >   -->virtqueue_get_buf_ctx_packed
> > > and in virtqueue_get_buf_ctx_packed:
> > >
> > > vq->last_used_idx += vq->packed.desc_state[id].num;
> > > if (unlikely(vq->last_used_idx >= vq->packed.vring.num)) {
> > >  vq->last_used_idx -= vq->packed.vring.num;
> > >  vq->packed.used_wrap_counter ^= 1;
> > > }
> > >
> > > if at the same time, there comes a vring interrupt,in vring_interrupt:
> > > we will call:
> > > vring_interrupt
> > >   -->more_used
> > >   -->more_used_packed
> > >   -->is_used_desc_packed
> > > in is_used_desc_packed, the last_used_idx maybe >= vq->packed.vring.num.
> > > so this could case a memory out of bounds bug.
> > >
> > > this patch is to fix this.
> > >
> > > Signed-off-by: huangjie.albert 
> >
> >
> > This pattern was always iffy, but I don't think the patch
> > improves the situation much. last_used_idx and vq->packed.used_wrap_counter
> > can still get out of sync.
> >
> > Maybe refactor code to keep everything in vq->last_used_idx?
> >
> > Jason what is your take?
> >
> >
> >
> >
> >
> > > ---
> > >  drivers/virtio/virtio_ring.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 13a7348cedff..d2abbb3a8187 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -1397,6 +1397,9 @@ static inline bool is_used_desc_packed(const struct 
> > > vring_virtqueue *vq,
> > >   bool avail, used;
> > >   u16 flags;
> > >
> > > + if (idx >= vq->packed.vring.num)
> > > + return false;
> > > +
> > >   flags = le16_to_cpu(vq->packed.vring.desc[idx].flags);
> > >   avail = !!(flags & (1 << VRING_PACKED_DESC_F_AVAIL));
> > >   used = !!(flags & (1 << VRING_PACKED_DESC_F_USED));
> > > --
> > > 2.27.0
> >

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

Re: [PATCH RFC v1 4/7] swiotlb: to implement io_tlb_high_mem

2022-06-10 Thread Dongli Zhang
Hi Christoph,

On 6/8/22 10:05 PM, Christoph Hellwig wrote:
> All this really needs to be hidden under the hood.
> 

Since this patch file has 200+ lines, would you please help clarify what does
'this' indicate?

The idea of this patch:

1. Convert the functions to initialize swiotlb into callee. The callee accepts
'true' or 'false' as arguments to indicate whether it is for default or new
swiotlb buffer, e.g., swiotlb_init_remap() into __swiotlb_init_remap().

2. At the caller side to decide if we are going to call the callee to create the
extra buffer.

Do you mean the callee if still *too high level* and all the
decision/allocation/initialization should be down at *lower level functions*?

E.g., if I re-work the "struct io_tlb_mem" to make the 64-bit buffer as the 2nd
array of io_tlb_mem->slots[32_or_64][index], the user will only notice it is the
default 'io_tlb_default_mem', but will not be able to know if it is allocated
from 32-bit or 64-bit buffer.

Thank you very much for the feedback.

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


Re: [PATCH] virtio_ring : fix vring_packed_desc memory out of bounds bug

2022-06-10 Thread Michael S. Tsirkin
On Fri, Jun 10, 2022 at 06:33:14PM +0800, huangjie.albert wrote:
> ksoftirqd may consume the packet and it will call:
> virtnet_poll
>   -->virtnet_receive
>   -->virtqueue_get_buf_ctx
>   -->virtqueue_get_buf_ctx_packed
> and in virtqueue_get_buf_ctx_packed:
> 
> vq->last_used_idx += vq->packed.desc_state[id].num;
> if (unlikely(vq->last_used_idx >= vq->packed.vring.num)) {
>  vq->last_used_idx -= vq->packed.vring.num;
>  vq->packed.used_wrap_counter ^= 1;
> }
> 
> if at the same time, there comes a vring interrupt,in vring_interrupt:
> we will call:
> vring_interrupt
>   -->more_used
>   -->more_used_packed
>   -->is_used_desc_packed
> in is_used_desc_packed, the last_used_idx maybe >= vq->packed.vring.num.
> so this could case a memory out of bounds bug.
> 
> this patch is to fix this.
> 
> Signed-off-by: huangjie.albert 


This pattern was always iffy, but I don't think the patch
improves the situation much. last_used_idx and vq->packed.used_wrap_counter 
can still get out of sync.

Maybe refactor code to keep everything in vq->last_used_idx?

Jason what is your take?





> ---
>  drivers/virtio/virtio_ring.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 13a7348cedff..d2abbb3a8187 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1397,6 +1397,9 @@ static inline bool is_used_desc_packed(const struct 
> vring_virtqueue *vq,
>   bool avail, used;
>   u16 flags;
>  
> + if (idx >= vq->packed.vring.num)
> + return false;
> +
>   flags = le16_to_cpu(vq->packed.vring.desc[idx].flags);
>   avail = !!(flags & (1 << VRING_PACKED_DESC_F_AVAIL));
>   used = !!(flags & (1 << VRING_PACKED_DESC_F_USED));
> -- 
> 2.27.0

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

Re: [PATCH v2 03/19] fs: Add aops->migrate_folio

2022-06-10 Thread David Hildenbrand
On 09.06.22 16:35, Matthew Wilcox wrote:
> On Thu, Jun 09, 2022 at 02:50:20PM +0200, David Hildenbrand wrote:
>> On 08.06.22 17:02, Matthew Wilcox (Oracle) wrote:
>>> diff --git a/Documentation/filesystems/locking.rst 
>>> b/Documentation/filesystems/locking.rst
>>> index c0fe711f14d3..3d28b23676bd 100644
>>> --- a/Documentation/filesystems/locking.rst
>>> +++ b/Documentation/filesystems/locking.rst
>>> @@ -253,7 +253,8 @@ prototypes::
>>> void (*free_folio)(struct folio *);
>>> int (*direct_IO)(struct kiocb *, struct iov_iter *iter);
>>> bool (*isolate_page) (struct page *, isolate_mode_t);
>>> -   int (*migratepage)(struct address_space *, struct page *, struct page 
>>> *);
>>> +   int (*migrate_folio)(struct address_space *, struct folio *dst,
>>> +   struct folio *src, enum migrate_mode);
>>> void (*putback_page) (struct page *);
>>
>> isolate_page/putback_page are leftovers from the previous patch, no?
> 
> Argh, right, I completely forgot I needed to update the documentation in
> that patch.
> 
>>> +++ b/Documentation/vm/page_migration.rst
>>> @@ -181,22 +181,23 @@ which are function pointers of struct 
>>> address_space_operations.
>>> Once page is successfully isolated, VM uses page.lru fields so driver
>>> shouldn't expect to preserve values in those fields.
>>>  
>>> -2. ``int (*migratepage) (struct address_space *mapping,``
>>> -|  ``struct page *newpage, struct page *oldpage, enum migrate_mode);``
>>> -
>>> -   After isolation, VM calls migratepage() of driver with the isolated 
>>> page.
>>> -   The function of migratepage() is to move the contents of the old page 
>>> to the
>>> -   new page
>>> -   and set up fields of struct page newpage. Keep in mind that you should
>>> -   indicate to the VM the oldpage is no longer movable via 
>>> __ClearPageMovable()
>>> -   under page_lock if you migrated the oldpage successfully and returned
>>> -   MIGRATEPAGE_SUCCESS. If driver cannot migrate the page at the moment, 
>>> driver
>>> -   can return -EAGAIN. On -EAGAIN, VM will retry page migration in a short 
>>> time
>>> -   because VM interprets -EAGAIN as "temporary migration failure". On 
>>> returning
>>> -   any error except -EAGAIN, VM will give up the page migration without
>>> -   retrying.
>>> -
>>> -   Driver shouldn't touch the page.lru field while in the migratepage() 
>>> function.
>>> +2. ``int (*migrate_folio) (struct address_space *mapping,``
>>> +|  ``struct folio *dst, struct folio *src, enum migrate_mode);``
>>> +
>>> +   After isolation, VM calls the driver's migrate_folio() with the
>>> +   isolated folio.  The purpose of migrate_folio() is to move the contents
>>> +   of the source folio to the destination folio and set up the fields
>>> +   of destination folio.  Keep in mind that you should indicate to the
>>> +   VM the source folio is no longer movable via __ClearPageMovable()
>>> +   under folio if you migrated the source successfully and returned
>>> +   MIGRATEPAGE_SUCCESS.  If driver cannot migrate the folio at the
>>> +   moment, driver can return -EAGAIN. On -EAGAIN, VM will retry folio
>>> +   migration in a short time because VM interprets -EAGAIN as "temporary
>>> +   migration failure".  On returning any error except -EAGAIN, VM will
>>> +   give up the folio migration without retrying.
>>> +
>>> +   Driver shouldn't touch the folio.lru field while in the migrate_folio()
>>> +   function.
>>>  
>>>  3. ``void (*putback_page)(struct page *);``
>>
>> Hmm, here it's a bit more complicated now, because we essentially have
>> two paths: LRU+migrate_folio or !LRU+movable_ops
>> (isolate/migrate/putback page)
> 
> Oh ... actually, this is just documenting the driver side of things.
> I don't really like how it's written.  Here, have some rewritten
> documentation (which is now part of the previous patch):
> 

LGTM, thanks.


-- 
Thanks,

David / dhildenb

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


[PATCH v1] drivers/virtio: Clarify CONFIG_VIRTIO_MEM for unsupported architectures

2022-06-10 Thread David Hildenbrand
Let's make it clearer that simply unlocking CONFIG_VIRTIO_MEM on an
architecture is most probably not sufficient to have it working as
expected.

Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
Cc: Gavin Shan 
Signed-off-by: David Hildenbrand 
---
 drivers/virtio/Kconfig | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index b5adf6abd241..f86b6a988b63 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -115,9 +115,11 @@ config VIRTIO_MEM
 This driver provides access to virtio-mem paravirtualized memory
 devices, allowing to hotplug and hotunplug memory.
 
-This driver was only tested under x86-64 and arm64, but should
-theoretically work on all architectures that support memory hotplug
-and hotremove.
+This driver currently only supports x86-64 and arm64. Although it
+should compile on other architectures that implement memory
+hot(un)plug, architecture-specific and/or common
+code changes may be required for virtio-mem, kdump and kexec to work as
+expected.
 
 If unsure, say M.
 
-- 
2.35.3

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