linux-kernel - Re: [PATCH v4 1/1] crypto: add virtio-crypto driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161201115810.GC10441@stefanha-x1.localdomain>
Date:   Thu, 1 Dec 2016 11:58:10 +0000
From:   Stefan Hajnoczi <stefanha@...hat.com>
To:     "Gonglei (Arei)" <arei.gonglei@...wei.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "qemu-devel@...gnu.org" <qemu-devel@...gnu.org>,
        "virtio-dev@...ts.oasis-open.org" <virtio-dev@...ts.oasis-open.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        Luonengjun <luonengjun@...wei.com>,
        "mst@...hat.com" <mst@...hat.com>,
        "Huangweidong (C)" <weidong.huang@...wei.com>,
        "Wubin (H)" <wu.wubin@...wei.com>,
        "xin.zeng@...el.com" <xin.zeng@...el.com>,
        Claudio Fontana <Claudio.Fontana@...wei.com>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "pasic@...ux.vnet.ibm.com" <pasic@...ux.vnet.ibm.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Zhoujian (jay, Euler)" <jianjay.zhou@...wei.com>,
        "Hanweidong (Randy)" <hanweidong@...wei.com>,
        "arei.gonglei@...mail.com" <arei.gonglei@...mail.com>,
        "cornelia.huck@...ibm.com" <cornelia.huck@...ibm.com>,
        "Xuquan (Quan Xu)" <xuquan8@...wei.com>,
        longpeng <longpeng2@...wei.com>,
        "salvatore.benedetto@...el.com" <salvatore.benedetto@...el.com>
Subject: Re: [PATCH v4 1/1] crypto: add virtio-crypto driver

On Thu, Dec 01, 2016 at 02:27:19AM +0000, Gonglei (Arei) wrote:
> > On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > > +static int virtio_crypto_alg_ablkcipher_init_session(
> > > +		struct virtio_crypto_ablkcipher_ctx *ctx,
> > > +		uint32_t alg, const uint8_t *key,
> > > +		unsigned int keylen,
> > > +		int encrypt)
> > > +{
> > > +	struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> > > +	unsigned int tmp;
> > > +	struct virtio_crypto *vcrypto = ctx->vcrypto;
> > > +	int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> > VIRTIO_CRYPTO_OP_DECRYPT;
> > > +	int err;
> > > +	unsigned int num_out = 0, num_in = 0;
> > > +
> > > +	/*
> > > +	 * Avoid to do DMA from the stack, switch to using
> > > +	 * dynamically-allocated for the key
> > > +	 */
> > > +	uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> > > +
> > > +	if (!cipher_key)
> > > +		return -ENOMEM;
> > > +
> > > +	memcpy(cipher_key, key, keylen);
> > 
> > Are there any rules on handling key material in the kernel?  This buffer
> > is just kfreed later.  Do you need to zero it out before freeing it?
> > 
> Good questions. For kernel crypto core, each cipher request should be freed
> by skcipher_request_free(): zeroize and free request data structure.
> 
> I need to use kzfree() for key as well. I'll also check other stuffs. Thanks. 
> 
> > > +
> > > +	spin_lock(&vcrypto->ctrl_lock);
> > 
> > The QAT accelerator driver doesn't spin while talking to the device in
> > virtio_crypto_alg_ablkcipher_init_session().  I didn't find any other
> > driver examples in the kernel tree, but this function seems like a
> > weakness in the virtio-crypto device.
> > 
> The control queues of virtio-net and virtio-console are also be locked
> Please see:
>  __send_control_msg() in virtio_console.c and virtio-net's control queue
> protected by rtnl lock.
> 
> I didn't want to protect session creations but the virtqueue's operations
> like what other virtio devices do.
> 
> > While QEMU is servicing the create session command this vcpu is blocked.
> > The QEMU global mutex is held so no other vcpu can enter QEMU and the
> > QMP monitor is also blocked.
> > 
> > This is a scalability and performance problem.  Can you look at how QAT
> > avoids this synchronous session setup?
> 
> For QAT driver, the session creation is synchronous as well because it's a
> plain software operation which can be completed ASAP.

I'm mentioning the vmexit and wait for request completion in a spinlock
because the same type of issue has been a performance bottleneck with
virtio guest driver in the past.

If there is a way to avoid spinning then that would be preferred.  It's
basically a known anti-pattern for virtio guest drivers.

Could you initialize the session on the host side when the first
asynchronous data is submitted for encryption/decryption instead of
during init_session()?

Stefan

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)