[PATCH RFC v2] tpm: tpm_vtpm_proxy: do not reference kernel memory as user memory

* [PATCH RFC v2] tpm: tpm_vtpm_proxy: do not reference kernel memory as user memory
@ 2023-05-30  2:01 Jarkko Sakkinen
  2023-05-30 11:44 ` kernel test robot
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2023-05-30  2:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Jason Gunthorpe, Alejandro Cabrera, Jarkko Sakkinen, stable,
	Jarkko Sakkinen, Stefan Berger, linux-kernel

From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi>

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.
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().

Cc: stable@vger.kernel.org
Fixes: be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi>
---
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;
+
+	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;
 
 	if (count > sizeof(proxy_dev->buffer)) {
 		dev_err(&chip->dev,
@@ -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;
 
 	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;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread