From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 99BAEC43381 for ; Fri, 22 Feb 2019 05:51:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 54FE720818 for ; Fri, 22 Feb 2019 05:51:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725792AbfBVFvX (ORCPT ); Fri, 22 Feb 2019 00:51:23 -0500 Received: from mail-qt1-f193.google.com ([209.85.160.193]:40114 "EHLO mail-qt1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725848AbfBVFvX (ORCPT ); Fri, 22 Feb 2019 00:51:23 -0500 Received: by mail-qt1-f193.google.com with SMTP id j36so1286977qta.7 for ; Thu, 21 Feb 2019 21:51:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=qwQHzrqGdSNLFfd0VTUf34iTZvzGUtAT6kFxKaWnqD8=; b=JfFE6rhxq3BbCiBZ9nqqJP2Q/gm5ErNp34d0cXhFreCFjGiiUOkMDux4czYzqedlCn 4VTMHFwz4Y0ALYzTUv7n8jL+x5TFBuMRK7jMGF9vHd2N/P8onPwJUx6QLFIX/UDYaiUy LUZGfmujarVkwge/y2hwBbhAnZZMqCXZVLDUWggjM/IgxI9NIUluzADTxcppwIcUBqFy M6GVz39lB8UQgpMDrbO1Cp9ywN/FWyu0DVwHWHf5h70KLB0Z2XSty2mzHWR53CzevZZc 0rkjPDIOMPnDBfdNUXss8PwnVOHGfGqVXci3iJOHgVs/5C75P06Gx5U14ShAczEnaAlb q8JQ== X-Gm-Message-State: AHQUAuYbD/mxw55OzYMooCcSI0OGCEh/QR7OMGHggs8FrW/QV4UeKgDY JrxxI9ugIjKOeED/cQ+KCFblZw== X-Google-Smtp-Source: AHgI3IbR2uri3aXKVTQsFtgAGYCovc8wIvxltnqsjy23qJpNtmENbV20tSTpkmF/ohVATqF2KqgDbA== X-Received: by 2002:a0c:b89e:: with SMTP id y30mr1874813qvf.52.1550814681519; Thu, 21 Feb 2019 21:51:21 -0800 (PST) Received: from redhat.com (pool-173-76-246-42.bstnma.fios.verizon.net. [173.76.246.42]) by smtp.gmail.com with ESMTPSA id p127sm285977qke.97.2019.02.21.21.51.19 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 21 Feb 2019 21:51:20 -0800 (PST) Date: Fri, 22 Feb 2019 00:51:18 -0500 From: "Michael S. Tsirkin" To: David Tolnay Cc: Peter Huewe , Jarkko Sakkinen , Jason Gunthorpe , linux-integrity@vger.kernel.org, Jason Wang , virtualization@lists.linux-foundation.org, dgreid@chromium.org, apronin@chromium.org Subject: Re: [PATCH] tpm: Add driver for TPM over virtio Message-ID: <20190222003207-mutt-send-email-mst@kernel.org> References: <388c5b80-21a7-1e91-a11f-3a1c1432368b@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <388c5b80-21a7-1e91-a11f-3a1c1432368b@gmail.com> Sender: linux-integrity-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-integrity@vger.kernel.org On Thu, Feb 21, 2019 at 06:14:02PM -0800, David Tolnay wrote: > Add a config TCG_VIRTIO_VTPM which enables a driver providing the guest > kernel side of TPM over virtio. > > Use case: TPM support is needed for performing trusted work from within > a virtual machine launched by Chrome OS. > > Tested inside crosvm, the Chrome OS virtual machine monitor. Crosvm's > implementation of the virtio TPM device can be found in these two source > files: > > - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/devices/src/virtio/tpm.rs > - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/tpm2/src/lib.rs > > and is currently backed by the libtpm2 TPM simulator: > > - https://chromium.googlesource.com/chromiumos/third_party/tpm2/ > > Reviewed-on: https://chromium-review.googlesource.com/1387655 > Reviewed-by: Andrey Pronin > Tested-by: David Tolnay > Signed-off-by: David Tolnay > --- > UNRESOLVED: > The driver assumes virtio device number VIRTIO_ID_TPM=31. If there is > interest in accepting a virtio TPM driver into the kernel, the Chrome OS > team will coordinate with the OASIS virtio technical committee to secure > an agreed upon device number and include it in a later patchset. I am not a tpm expert but I don't see why not. > drivers/char/tpm/Kconfig | 9 + > drivers/char/tpm/Makefile | 1 + > drivers/char/tpm/tpm_virtio.c | 460 ++++++++++++++++++++++++++++++++++ Maintainer entry? > 3 files changed, 470 insertions(+) > create mode 100644 drivers/char/tpm/tpm_virtio.c > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig > index 536e55d3919f..8997060e248e 100644 > --- a/drivers/char/tpm/Kconfig > +++ b/drivers/char/tpm/Kconfig > @@ -164,6 +164,15 @@ config TCG_VTPM_PROXY > /dev/vtpmX and a server-side file descriptor on which the vTPM > can receive commands. > > +config TCG_VIRTIO_VTPM > + tristate "Virtio vTPM" > + depends on TCG_TPM > + help > + This driver provides the guest kernel side of TPM over Virtio. If > + you are building Linux to run inside of a hypervisor that supports > + TPM over Virtio, say Yes and the virtualized TPM will be > + accessible from the guest. > + > > source "drivers/char/tpm/st33zp24/Kconfig" > endif # TCG_TPM > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile > index a01c4cab902a..4f5d1071257a 100644 > --- a/drivers/char/tpm/Makefile > +++ b/drivers/char/tpm/Makefile > @@ -33,3 +33,4 @@ obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/ > obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o > obj-$(CONFIG_TCG_CRB) += tpm_crb.o > obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o > +obj-$(CONFIG_TCG_VIRTIO_VTPM) += tpm_virtio.o > diff --git a/drivers/char/tpm/tpm_virtio.c b/drivers/char/tpm/tpm_virtio.c > new file mode 100644 > index 000000000000..f3239d983f18 > --- /dev/null > +++ b/drivers/char/tpm/tpm_virtio.c > @@ -0,0 +1,460 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2019 Google Inc. > + * > + * Author: David Tolnay > + * > + * --- > + * > + * Device driver for TPM over virtio. > + * > + * This driver employs a single virtio queue to handle both send and recv. TPM > + * commands are sent over virtio to the hypervisor during a TPM send operation > + * and responses are received over the same queue during a recv operation. > + * > + * The driver contains a single buffer that is the only buffer we ever place on > + * the virtio queue. Commands are copied from the caller's command buffer into > + * the driver's buffer before handing off to virtio, and responses are received > + * into the driver's buffer then copied into the caller's response buffer. This > + * allows us to be resilient to timeouts. When a send or recv operation times > + * out, the caller is free to destroy their buffer; we don't want the hypervisor > + * continuing to perform reads or writes against that destroyed buffer. > + * > + * This driver does not support concurrent send and recv operations. Mutual > + * exclusion is upheld by the tpm_mutex lock held in tpm-interface.c around the > + * calls to chip->ops->send and chip->ops->recv. > + * > + * The intended hypervisor-side implementation is as follows. > + * > + * while true: > + * await next virtio buffer. > + * expect first descriptor in chain to be guest-to-host. > + * read tpm command from that buffer. > + * synchronously perform TPM work determined by the command. > + * expect second descriptor in chain to be host-to-guest. > + * write TPM response into that buffer. > + * place buffer on virtio used queue indicating how many bytes written. That's fine I think except generally it should be legal for guest to split each buffer to several segments. > + */ > + > +#include > + > +#include "tpm.h" > + > +/* > + * Timeout duration when waiting on the hypervisor to complete its end of the > + * TPM operation. This timeout is relatively high because certain TPM operations > + * can take dozens of seconds. > + */ > +#define TPM_VIRTIO_TIMEOUT (120 * HZ) Should we read this from device? E.g. a busy hypervisor might take longer to respond ... > + > +struct vtpm_device { > + /* > + * Data structure for integration with the common code of the TPM driver > + * in tpm-chip.c. > + */ > + struct tpm_chip *chip; > + > + /* > + * Virtio queue for sending TPM commands out of the virtual machine and > + * receiving TPM responses back from the hypervisor. > + */ > + struct virtqueue *vq; > + > + /* > + * Completion that is notified when a virtio operation has been > + * fulfilled by the hypervisor. > + */ > + struct completion complete; > + > + /* > + * Whether driver currently holds ownership of the virtqueue buffer. > + * When false, the hypervisor is in the process of reading or writing > + * the buffer and the driver must not touch it. > + */ > + bool driver_has_buffer; > + > + /* > + * Whether during the most recent TPM operation, a virtqueue_kick failed > + * or a wait timed out. > + * > + * The next send or recv operation will attempt a kick upon seeing this > + * status. That should clear up the queue in the case that the > + * hypervisor became temporarily nonresponsive, such as by resource > + * exhaustion on the host. The extra kick enables recovery from kicks > + * going unnoticed by the hypervisor as well as recovery from virtio > + * callbacks going unnoticed by the guest kernel. Well not necessarily. E.g. virtqueue_kick does not kick if hypervisor didn't enable notifications (e.g. with event idx they get disabled automatically). So it won't recover if hypervisor can discard kicks. I think this comment needs clarification. > + */ > + bool needs_kick; > + > + /* Number of bytes available to read from the virtqueue buffer. */ > + unsigned int readable; > + > + /* > + * Buffer in which all virtio transfers take place. Buffer size is the > + * maximum legal TPM command or response message size. > + */ > + u8 virtqueue_buffer[TPM_BUFSIZE]; > +}; > + > +/* > + * Wait for ownership of the virtqueue buffer. > + * > + * The why-string should begin with "waiting to..." or "waiting for..." with no > + * trailing newline. It will appear in log output. > + * > + * Returns zero for success, otherwise negative error. > + */ > +static int vtpm_wait_for_buffer(struct vtpm_device *dev, const char *why) > +{ > + int ret; > + bool did_kick; > + struct tpm_chip *chip = dev->chip; > + unsigned long deadline = jiffies + TPM_VIRTIO_TIMEOUT; > + > + /* Kick queue if needed. */ > + if (dev->needs_kick) { > + did_kick = virtqueue_kick(dev->vq); > + if (!did_kick) { > + dev_notice(&chip->dev, "kick failed; will retry\n"); > + return -EBUSY; > + } > + dev->needs_kick = false; > + } > + > + while (!dev->driver_has_buffer) { > + unsigned long now = jiffies; > + > + /* Check timeout, otherwise `deadline - now` may underflow. */ > + if time_after_eq(now, deadline) { > + dev_warn(&chip->dev, "timed out %s\n", why); > + dev->needs_kick = true; > + return -ETIMEDOUT; > + } > + > + /* > + * Wait to be signaled by virtio callback. > + * > + * Positive ret is jiffies remaining until timeout when the > + * completion occurred, which means successful completion. Zero > + * ret is timeout. Negative ret is error. > + */ > + ret = wait_for_completion_killable_timeout( > + &dev->complete, deadline - now); > + > + /* Log if completion did not occur. */ > + if (ret == -ERESTARTSYS) { > + /* Not a warning if it was simply interrupted. */ > + dev_dbg(&chip->dev, "interrupted %s\n", why); > + } else if (ret == 0) { > + dev_warn(&chip->dev, "timed out %s\n", why); > + ret = -ETIMEDOUT; Should we check NEEDS_RESET bit and try to reset the device? Or is that too drastic? > + } else if (ret < 0) { > + dev_warn(&chip->dev, "failed while %s: error %d\n", > + why, -ret); > + } > + > + /* > + * Return error if completion did not occur. Schedule kick to be > + * retried at the start of the next send/recv to help unblock > + * the queue. > + */ > + if (ret < 0) { > + dev->needs_kick = true; > + return ret; > + } > + > + /* Completion occurred. Expect response buffer back. */ > + if (virtqueue_get_buf(dev->vq, &dev->readable)) { > + dev->driver_has_buffer = true; > + > + if (dev->readable > TPM_BUFSIZE) { > + dev_crit(&chip->dev, > + "hypervisor bug: response exceeds max size, %u > %u\n", > + dev->readable, > + (unsigned int) TPM_BUFSIZE); > + dev->readable = TPM_BUFSIZE; > + return -EPROTO; > + } > + } > + } > + > + return 0; > +} > + > +static int vtpm_op_send(struct tpm_chip *chip, u8 *caller_buf, size_t len) > +{ > + int ret; > + bool did_kick; > + struct scatterlist sg_outbuf, sg_inbuf; > + struct scatterlist *sgs[2] = { &sg_outbuf, &sg_inbuf }; > + struct vtpm_device *dev = dev_get_drvdata(&chip->dev); > + u8 *virtqueue_buf = dev->virtqueue_buffer; > + > + dev_dbg(&chip->dev, __func__ " %zu bytes\n", len); > + > + if (len > TPM_BUFSIZE) { > + dev_err(&chip->dev, > + "command is too long, %zu > %zu\n", > + len, (size_t) TPM_BUFSIZE); > + return -EINVAL; > + } > + > + /* > + * Wait until hypervisor relinquishes ownership of the virtqueue buffer. > + * > + * This may block if the previous recv operation timed out in the guest > + * kernel but is still being processed by the hypervisor. Also may block > + * if send operations are performed back-to-back, such as if something > + * in the caller failed in between a send and recv. > + * > + * During normal operation absent of any errors or timeouts, this does > + * not block. > + */ > + ret = vtpm_wait_for_buffer(dev, "waiting to begin send"); > + if (ret) > + return ret; > + > + /* Driver owns virtqueue buffer and may now write into it. */ > + memcpy(virtqueue_buf, caller_buf, len); > + > + /* > + * Enqueue the virtqueue buffer once as outgoing virtio data (written by > + * the virtual machine and read by the hypervisor) and again as incoming > + * data (written by the hypervisor and read by the virtual machine). > + * This step moves ownership of the virtqueue buffer from driver to > + * hypervisor. > + * > + * Note that we don't know here how big of a buffer the caller will use > + * with their later call to recv. We allow the hypervisor to write up to > + * the TPM max message size. If the caller ends up using a smaller > + * buffer with recv that is too small to hold the entire response, the > + * recv will return an error. This conceivably breaks TPM > + * implementations that want to produce a different verbosity of > + * response depending on the receiver's buffer size. > + */ > + sg_init_one(&sg_outbuf, virtqueue_buf, len); > + sg_init_one(&sg_inbuf, virtqueue_buf, TPM_BUFSIZE); > + ret = virtqueue_add_sgs(dev->vq, sgs, 1, 1, virtqueue_buf, GFP_KERNEL); > + if (ret) { > + dev_err(&chip->dev, "failed virtqueue_add_sgs\n"); > + return ret; > + } > + > + /* Kick the other end of the virtqueue after having added a buffer. */ > + did_kick = virtqueue_kick(dev->vq); > + if (!did_kick) { > + dev->needs_kick = true; > + dev_notice(&chip->dev, "kick failed; will retry\n"); > + > + /* > + * We return 0 anyway because what the caller doesn't know can't > + * hurt them. They can call recv and it will retry the kick. If > + * that works, everything is fine. > + * > + * If the retry in recv fails too, they will get -EBUSY from > + * recv. > + */ > + } > + > + /* > + * Hypervisor is now processing the TPM command asynchronously. It will > + * read the command from the output buffer and write the response into > + * the input buffer (which are the same buffer). When done, it will send > + * back the buffers over virtio and the driver's virtio callback will > + * complete dev->complete so that we know the response is ready to be > + * read. > + * > + * It is important to have copied data out of the caller's buffer into > + * the driver's virtqueue buffer because the caller is free to destroy > + * their buffer when this call returns. We can't avoid copying by > + * waiting here for the hypervisor to finish reading, because if that > + * wait times out, we return and the caller may destroy their buffer > + * while the hypervisor is continuing to read from it. > + */ > + dev->driver_has_buffer = false; > + return 0; > +} > + > +static int vtpm_op_recv(struct tpm_chip *chip, u8 *caller_buf, size_t len) > +{ > + int ret; > + struct vtpm_device *dev = dev_get_drvdata(&chip->dev); > + u8 *virtqueue_buf = dev->virtqueue_buffer; > + > + dev_dbg(&chip->dev, __func__ "\n"); > + > + /* > + * Wait until the virtqueue buffer is owned by the driver. > + * > + * This will usually block while the hypervisor finishes processing the > + * most recent TPM command. > + */ > + ret = vtpm_wait_for_buffer(dev, "waiting for TPM response"); > + if (ret) > + return ret; > + > + dev_dbg(&chip->dev, "received %u bytes\n", dev->readable); > + > + if (dev->readable > len) { > + dev_notice(&chip->dev, > + "TPM response is bigger than receiver's buffer: %u > %zu\n", > + dev->readable, len); > + return -EINVAL; > + } > + > + /* Copy response over to the caller. */ > + memcpy(caller_buf, virtqueue_buf, dev->readable); > + > + return dev->readable; > +} > + > +static void vtpm_op_cancel(struct tpm_chip *chip) > +{ > + /* > + * Cancel is not called in this driver's use of tpm-interface.c. It may > + * be triggered through tpm-sysfs but that can be implemented as needed. > + * Be aware that tpm-sysfs performs cancellation without holding the > + * tpm_mutex that protects our send and recv operations, so a future > + * implementation will need to consider thread safety of concurrent > + * send/recv and cancel. > + */ > + dev_notice(&chip->dev, "cancellation is not implemented\n"); > +} > + > +static u8 vtpm_op_status(struct tpm_chip *chip) > +{ > + /* > + * Status is for TPM drivers that want tpm-interface.c to poll for > + * completion before calling recv. Usually this is when the hardware > + * needs to be polled i.e. there is no other way for recv to block on > + * the TPM command completion. > + * > + * Polling goes until `(status & complete_mask) == complete_val`. This > + * driver defines both complete_mask and complete_val as 0 and blocks on > + * our own completion object in recv instead. > + */ > + return 0; > +} > + > +static const struct tpm_class_ops vtpm_ops = { > + .flags = TPM_OPS_AUTO_STARTUP, > + .send = vtpm_op_send, > + .recv = vtpm_op_recv, > + .cancel = vtpm_op_cancel, > + .status = vtpm_op_status, > + .req_complete_mask = 0, > + .req_complete_val = 0, > +}; > + > +static void vtpm_virtio_complete(struct virtqueue *vq) > +{ > + struct virtio_device *vdev = vq->vdev; > + struct vtpm_device *dev = vdev->priv; > + > + complete(&dev->complete); > +} > + > +static int vtpm_probe(struct virtio_device *vdev) > +{ > + int err; > + struct vtpm_device *dev; > + struct virtqueue *vq; > + struct tpm_chip *chip; > + > + dev_dbg(&vdev->dev, __func__ "\n"); > + > + dev = kzalloc(sizeof(struct vtpm_device), GFP_KERNEL); > + if (!dev) { > + err = -ENOMEM; > + dev_err(&vdev->dev, "failed kzalloc\n"); > + goto err_dev_alloc; > + } > + vdev->priv = dev; > + > + vq = virtio_find_single_vq(vdev, vtpm_virtio_complete, "vtpm"); > + if (IS_ERR(vq)) { > + err = PTR_ERR(vq); > + dev_err(&vdev->dev, "failed virtio_find_single_vq\n"); > + goto err_virtio_find; > + } > + dev->vq = vq; > + > + chip = tpm_chip_alloc(&vdev->dev, &vtpm_ops); > + if (IS_ERR(chip)) { > + err = PTR_ERR(chip); > + dev_err(&vdev->dev, "failed tpm_chip_alloc\n"); > + goto err_chip_alloc; > + } > + dev_set_drvdata(&chip->dev, dev); > + chip->flags |= TPM_CHIP_FLAG_TPM2; > + dev->chip = chip; > + > + init_completion(&dev->complete); > + dev->driver_has_buffer = true; > + dev->needs_kick = false; > + dev->readable = 0; > + > + /* > + * Required in order to enable vq use in probe function for auto > + * startup. > + */ > + virtio_device_ready(vdev); > + > + err = tpm_chip_register(dev->chip); > + if (err) { > + dev_err(&vdev->dev, "failed tpm_chip_register\n"); > + goto err_chip_register; > + } > + > + return 0; > + > +err_chip_register: > + put_device(&dev->chip->dev); > +err_chip_alloc: > + vdev->config->del_vqs(vdev); > +err_virtio_find: > + kfree(dev); > +err_dev_alloc: > + return err; > +} > + > +static void vtpm_remove(struct virtio_device *vdev) > +{ > + struct vtpm_device *dev = vdev->priv; > + > + /* Undo tpm_chip_register. */ > + tpm_chip_unregister(dev->chip); > + > + /* Undo tpm_chip_alloc. */ > + put_device(&dev->chip->dev); > + > + vdev->config->reset(vdev); > + vdev->config->del_vqs(vdev); > + > + kfree(dev); > +} > + > +#define VIRTIO_ID_TPM 31 > + > +static struct virtio_device_id id_table[] = { > + { > + .device = VIRTIO_ID_TPM, > + .vendor = VIRTIO_DEV_ANY_ID, > + }, > + {}, > +}; Let's write static struct virtio_device_id id_table[] = { { VIRTIO_ID_TPM, VIRTIO_DEV_ANY_ID }, { 0 }, }; for consistency with other virtio devices. > + > +static struct virtio_driver vtpm_driver = { > + .driver.name = KBUILD_MODNAME, > + .driver.owner = THIS_MODULE, > + .id_table = id_table, > + .probe = vtpm_probe, > + .remove = vtpm_remove, Freeze/restore? > +}; > + > +module_virtio_driver(vtpm_driver); > + > +MODULE_AUTHOR("David Tolnay (dtolnay@gmail.com)"); > +MODULE_DESCRIPTION("Virtio vTPM Driver"); > +MODULE_VERSION("1.0"); > +MODULE_LICENSE("GPL"); > -- > 2.20.1