Re: [PATCH RFC v2] tpm: tpm_vtpm_proxy: do not reference kernel memory as user memory
From: Stefan Berger
Date: Tue May 30 2023 - 13:37:15 EST
On 5/29/23 22:01, Jarkko Sakkinen wrote:
From: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxx>
The driver has two issues (in priority order) in the locality change:
1. The driver uses __user pointer and copy_to_user() and
copy_from_user() with a kernel address during the locality change.
This can lead to ... bad things ?
2. For invalid locality change request from user space, the driver
sets errno to EFAULT, while for invalid input data EINVAL should
be used.
Address this by:
1. Introduce __vtpm_proxy_read_unlocked(), __vtpm_proxy_write_unlocked()
and __vtpm_proxy_read_write_locked().
2. Make locality change atomic by calling __vtpm_proxy_read_write_locked(),
instead of tpm_transmit_cmd().
2. sounds like it should be addressable by changing -EFAULT to -EINVAL but there's more to it?
Cc: stable@xxxxxxxxxxxxxxx
Fixes: be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxx>
---
v2:
* Acquiring and releasing mutex in-between should not be an issue
because they are executed with the chip locked.
---
drivers/char/tpm/tpm_vtpm_proxy.c | 162 ++++++++++++++----------------
1 file changed, 73 insertions(+), 89 deletions(-)
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 30e953988cab..8f43a82e5590 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,106 +57,112 @@ static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev);
* Functions related to 'server side'
*/
-/**
- * vtpm_proxy_fops_read - Read TPM commands on 'server side'
- *
- * @filp: file pointer
- * @buf: read buffer
- * @count: number of bytes to read
- * @off: offset
- *
- * Return:
- * Number of bytes read or negative error code
- */
-static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf,
- size_t count, loff_t *off)
+static ssize_t __vtpm_proxy_read_unlocked(struct proxy_dev *proxy_dev, char __user *buf,
+ size_t count)
{
- struct proxy_dev *proxy_dev = filp->private_data;
size_t len;
- int sig, 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);
+ int rc;
- if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
- mutex_unlock(&proxy_dev->buf_lock);
+ if (!(proxy_dev->state & STATE_OPENED_FLAG))
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);
+ if (buf)
+ rc = copy_to_user(buf, proxy_dev->buffer, len);
+
memset(proxy_dev->buffer, 0, len);
proxy_dev->req_len = 0;
if (!rc)
proxy_dev->state |= STATE_WAIT_RESPONSE_FLAG;
- mutex_unlock(&proxy_dev->buf_lock);
-
if (rc)
return -EFAULT;
return len;
}
-/**
- * vtpm_proxy_fops_write - Write TPM responses on 'server side'
- *
- * @filp: file pointer
- * @buf: write buffer
- * @count: number of bytes to write
- * @off: offset
- *
- * Return:
- * Number of bytes read or negative error value
- */
-static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf,
- size_t count, loff_t *off)
+static ssize_t __vtpm_proxy_write_unlocked(struct proxy_dev *proxy_dev, const char __user *buf,
+ size_t count)
{
- struct proxy_dev *proxy_dev = filp->private_data;
-
- mutex_lock(&proxy_dev->buf_lock);
-
- if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
- mutex_unlock(&proxy_dev->buf_lock);
+ if (!(proxy_dev->state & STATE_OPENED_FLAG))
return -EPIPE;
- }
if (count > sizeof(proxy_dev->buffer) ||
- !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG)) {
- mutex_unlock(&proxy_dev->buf_lock);
+ !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG))
return -EIO;
- }
proxy_dev->state &= ~STATE_WAIT_RESPONSE_FLAG;
proxy_dev->req_len = 0;
- if (copy_from_user(proxy_dev->buffer, buf, count)) {
- mutex_unlock(&proxy_dev->buf_lock);
+ if (buf && copy_from_user(proxy_dev->buffer, buf, count))
return -EFAULT;
- }
proxy_dev->resp_len = count;
+ return count;
+}
+static ssize_t __vtpm_proxy_read_write_unlocked(struct proxy_dev *proxy_dev, char __user *buf,
+ size_t count)
+{
+ ssize_t rc;
+
+ do {
+ rc = __vtpm_proxy_write_unlocked(proxy_dev, buf, count);
+ if (rc < 0)
+ break;
+ rc = __vtpm_proxy_read_unlocked(proxy_dev, buf, rc);
+ } while (0);
+
+ return rc;
+}
+
+/*
+ * See struct file_operations.
+ */
+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;
+ ssize_t rc;
+ int sig;
+
+ 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);
+ rc = __vtpm_proxy_read_unlocked(proxy_dev, buf, count);
mutex_unlock(&proxy_dev->buf_lock);
+ return rc;
+}
+
+/*
+ * See struct file_operations.
+ */
+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;
+ int rc;
ssize_t rc;
+
+ mutex_lock(&proxy_dev->buf_lock);
+ rc = __vtpm_proxy_write_unlocked(proxy_dev, buf, count);
+ mutex_unlock(&proxy_dev->buf_lock);
wake_up_interruptible(&proxy_dev->wq);
- return count;
+ return rc;
}
/*
@@ -295,28 +300,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 +313,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;
'u32 ordinal' like in tpm_try_transmit ?
you need this here in some form: be32_to_cpu(hdr->ordinal)
if (count > sizeof(proxy_dev->buffer)) {
dev_err(&chip->dev,(chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -338,9 +322,11 @@ 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))
- return -EFAULT;
+ if ((chip->flags & TPM_CHIP_FLAG_TPM2) && ord == TPM2_CC_SET_LOCALITY)
+ return -EINVAL;
+
+ if (ord == TPM_ORD_SET_LOCALITY)
+ return -EINVAL;
if ((chip->flags & TPM_CHIP_FLAG_TPM2)) {
if (ord == )
return -EINVAL;
} else if (ord == ) {
return -EINVAL;
}
mutex_lock(&proxy_dev->buf_lock);
@@ -409,12 +395,10 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
return rc;
tpm_buf_append_u8(&buf, locality);
- proxy_dev->state |= STATE_DRIVER_COMMAND;
-
- rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to set locality");
-
- proxy_dev->state &= ~STATE_DRIVER_COMMAND;
-
+ mutex_lock(&proxy_dev->buf_lock);
+ memcpy(proxy_dev->buffer, buf.data, tpm_buf_length(&buf));
+ rc = __vtpm_proxy_read_write_unlocked(proxy_dev, NULL, tpm_buf_length(&buf));
+ mutex_unlock(&proxy_dev->buf_lock);
if (rc < 0) {
locality = rc;
goto out;
Stefan