| 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
| ||
|
Message-ID: <33183CC9F5247A488A2544077AF19020DA14EC02@DGGEMA505-MBX.china.huawei.com>
Date: Thu, 1 Dec 2016 02:27:19 +0000
From: "Gonglei (Arei)" <arei.gonglei@...wei.com>
To: Stefan Hajnoczi <stefanha@...hat.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
Hi Stefan,
>
> On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> b/drivers/crypto/virtio/virtio_crypto_algs.c
> > new file mode 100644
> > index 0000000..08b077f
> > --- /dev/null
> > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > @@ -0,0 +1,518 @@
> > + /* Algorithms supported by virtio crypto device
> > + *
> > + * Authors: Gonglei <arei.gonglei@...wei.com>
> > + *
> > + * Copyright 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/scatterlist.h>
> > +#include <crypto/algapi.h>
> > +#include <linux/err.h>
> > +#include <crypto/scatterwalk.h>
> > +#include <linux/atomic.h>
> > +
> > +#include <uapi/linux/virtio_crypto.h>
> > +#include "virtio_crypto_common.h"
> > +
> > +static DEFINE_MUTEX(algs_lock);
>
> Did you run checkpatch.pl? I think it encourages you to document what
> the lock protects.
>
Sure. Basically I run checkpatch.py each time. :)
# ./scripts/checkpatch.pl 0001-crypto-add-virtio-crypto-driver.patch
total: 0 errors, 0 warnings, 1873 lines checked
0001-crypto-add-virtio-crypto-driver.patch has no obvious style problems and is ready for submission.
> > +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.
Regards,
-Gonglei
Powered by blists - more mailing lists