On Tue, May 30, 2023 at 11:50:01PM +0300, Jarkko Sakkinen wrote:
From: Jarkko Sakkinen jarkko.sakkinen@tuni.fi
vtpm_proxy_fops_set_locality() causes kernel buffers to be passed to copy_from_user() and copy_to_user().
Factor out the crippled code away with help of an internal API for managing struct proxy_dev instances.
Link: https://lore.kernel.org/all/20230530020133.235765-1-jarkko@kernel.org/ Cc: stable@vger.kernel.org # v4.14+ Fixes: be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.") Signed-off-by: Jarkko Sakkinen jarkko.sakkinen@tuni.fi
I've tested this on RISC-V QEMU and have not noticed issues so far, and thus dropped the RFC tag. I've been looking into the code a lot lately as I'm building a boot time support for it, which will allow to e.g. test IMA without a physical TPM. drivers/char/tpm/tpm_vtpm_proxy.c | 168 ++++++++++++++++-------------- 1 file changed, 87 insertions(+), 81 deletions(-)
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c index 30e953988cab..83496742cc19 100644 --- a/drivers/char/tpm/tpm_vtpm_proxy.c +++ b/drivers/char/tpm/tpm_vtpm_proxy.c @@ -38,7 +38,6 @@ struct proxy_dev { #define STATE_OPENED_FLAG BIT(0) #define STATE_WAIT_RESPONSE_FLAG BIT(1) /* waiting for emulator response */ #define STATE_REGISTERED_FLAG BIT(2) -#define STATE_DRIVER_COMMAND BIT(3) /* sending a driver specific command */ size_t req_len; /* length of queued TPM request */ size_t resp_len; /* length of queued TPM response */ @@ -58,6 +57,53 @@ static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev);
- Functions related to 'server side'
*/ +static ssize_t __vtpm_proxy_copy_from(struct proxy_dev *proxy_dev, u8 *buf, size_t count) +{
- size_t len = proxy_dev->req_len;
- if (!(proxy_dev->state & STATE_OPENED_FLAG))
return -EPIPE;
- if (count < len || len > sizeof(proxy_dev->buffer)) {
pr_debug("Invalid size in recv: count=%zd, req_len=%zd\n", count, len);
return -EIO;
- }
- memcpy(buf, proxy_dev->buffer, len);
- memset(proxy_dev->buffer, 0, len);
- proxy_dev->req_len = 0;
- proxy_dev->state |= STATE_WAIT_RESPONSE_FLAG;
- return len;
+}
+static ssize_t __vtpm_proxy_copy_to(struct proxy_dev *proxy_dev, const u8 *buf, size_t count) +{
- if (!(proxy_dev->state & STATE_OPENED_FLAG))
return -EPIPE;
- if (count > sizeof(proxy_dev->buffer) || !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG))
return -EIO;
- proxy_dev->state &= ~STATE_WAIT_RESPONSE_FLAG;
- proxy_dev->req_len = 0;
- proxy_dev->resp_len = count;
- memcpy(proxy_dev->buffer, buf, count);
- return count;
+}
+static int vtpm_proxy_wait_for(struct proxy_dev *proxy_dev) +{
- if (wait_event_interruptible(
proxy_dev->wq,
proxy_dev->req_len != 0 || !(proxy_dev->state & STATE_OPENED_FLAG)))
return -EINTR;
- return 0;
+}
/**
- vtpm_proxy_fops_read - Read TPM commands on 'server side'
@@ -73,44 +119,26 @@ static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf, size_t count, loff_t *off) { struct proxy_dev *proxy_dev = filp->private_data;
- size_t len;
- int sig, rc;
- u8 *kernel_buf;
- ssize_t rc;
- sig = wait_event_interruptible(proxy_dev->wq,
proxy_dev->req_len != 0 ||
!(proxy_dev->state & STATE_OPENED_FLAG));
- if (sig)
return -EINTR;
- mutex_lock(&proxy_dev->buf_lock);
- if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
mutex_unlock(&proxy_dev->buf_lock);
return -EPIPE;
- }
- len = proxy_dev->req_len;
- if (count < len || len > sizeof(proxy_dev->buffer)) {
mutex_unlock(&proxy_dev->buf_lock);
pr_debug("Invalid size in recv: count=%zd, req_len=%zd\n",
count, len);
return -EIO;
- }
- rc = copy_to_user(buf, proxy_dev->buffer, len);
- memset(proxy_dev->buffer, 0, len);
- proxy_dev->req_len = 0;
- rc = vtpm_proxy_wait_for(proxy_dev);
- if (rc)
return rc;
- if (!rc)
proxy_dev->state |= STATE_WAIT_RESPONSE_FLAG;
- kernel_buf = kzalloc(sizeof(proxy_dev->buffer), GFP_KERNEL);
- if (!kernel_buf)
return -ENOMEM;
- mutex_lock(&proxy_dev->buf_lock);
- rc = __vtpm_proxy_copy_from(proxy_dev, buf, count);
Is this meant to be kernel_buf instead of buf here? To me it reads like it copies to buf from proxy_dev->buffer, and then below it copies over buf with the kzalloc'd kernel_buf.
mutex_unlock(&proxy_dev->buf_lock);
- if (rc)
return -EFAULT;
- if (!rc && copy_to_user(buf, kernel_buf, count))
rc = -EFAULT;
- return len;
- kfree(kernel_buf);
- return rc;
} /** @@ -128,36 +156,26 @@ static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf, size_t count, loff_t *off) { struct proxy_dev *proxy_dev = filp->private_data;
- u8 *kernel_buf;
- int rc;
- mutex_lock(&proxy_dev->buf_lock);
- if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
mutex_unlock(&proxy_dev->buf_lock);
return -EPIPE;
- }
- if (count > sizeof(proxy_dev->buffer) ||
!(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG)) {
mutex_unlock(&proxy_dev->buf_lock);
return -EIO;
- }
- proxy_dev->state &= ~STATE_WAIT_RESPONSE_FLAG;
- proxy_dev->req_len = 0;
- kernel_buf = kzalloc(sizeof(proxy_dev->buffer), GFP_KERNEL);
- if (!kernel_buf)
return -ENOMEM;
- if (copy_from_user(proxy_dev->buffer, buf, count)) {
mutex_unlock(&proxy_dev->buf_lock);
- if (copy_from_user(kernel_buf, buf, count)) {
return -EFAULT; }kfree(kernel_buf);
- proxy_dev->resp_len = count;
- mutex_lock(&proxy_dev->buf_lock);
- rc = __vtpm_proxy_copy_to(proxy_dev, kernel_buf, count); mutex_unlock(&proxy_dev->buf_lock);
wake_up_interruptible(&proxy_dev->wq);
- kfree(kernel_buf);
- return count;
- return rc;
} /* @@ -295,28 +313,6 @@ static int vtpm_proxy_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count) return len; } -static int vtpm_proxy_is_driver_command(struct tpm_chip *chip,
u8 *buf, size_t count)
-{
- struct tpm_header *hdr = (struct tpm_header *)buf;
- if (count < sizeof(struct tpm_header))
return 0;
- if (chip->flags & TPM_CHIP_FLAG_TPM2) {
switch (be32_to_cpu(hdr->ordinal)) {
case TPM2_CC_SET_LOCALITY:
return 1;
}
- } else {
switch (be32_to_cpu(hdr->ordinal)) {
case TPM_ORD_SET_LOCALITY:
return 1;
}
- }
- return 0;
-}
/*
- Called when core TPM driver forwards TPM requests to 'server side'.
@@ -330,6 +326,7 @@ static int vtpm_proxy_is_driver_command(struct tpm_chip *chip, static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count) { struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev);
- unsigned int ord = ((struct tpm_header *)buf)->ordinal;
if (count > sizeof(proxy_dev->buffer)) { dev_err(&chip->dev, @@ -338,8 +335,10 @@ static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count) return -EIO; }
- if (!(proxy_dev->state & STATE_DRIVER_COMMAND) &&
vtpm_proxy_is_driver_command(chip, buf, count))
- if ((chip->flags & TPM_CHIP_FLAG_TPM2) && ord == TPM2_CC_SET_LOCALITY)
return -EFAULT;
- if (ord == TPM_ORD_SET_LOCALITY) return -EFAULT;
mutex_lock(&proxy_dev->buf_lock); @@ -407,13 +406,20 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality) TPM_ORD_SET_LOCALITY); if (rc) return rc;
- tpm_buf_append_u8(&buf, locality);
- proxy_dev->state |= STATE_DRIVER_COMMAND;
- mutex_lock(&proxy_dev->buf_lock);
- rc = __vtpm_proxy_copy_to(proxy_dev, buf.data, tpm_buf_length(&buf));
- mutex_unlock(&proxy_dev->buf_lock);
- rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to set locality");
- rc = vtpm_proxy_wait_for(proxy_dev);
- if (rc)
return rc;
- proxy_dev->state &= ~STATE_DRIVER_COMMAND;
- mutex_lock(&proxy_dev->buf_lock);
- rc = __vtpm_proxy_copy_from(proxy_dev, buf.data, tpm_buf_length(&buf));
- mutex_unlock(&proxy_dev->buf_lock);
if (rc < 0) { locality = rc; -- 2.39.2