tpm: tpm_vtpm_proxy: do not reference kernel memory as user memory

From: Jarkko Sakkinen <[email protected]>

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 [email protected]

Fixes be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.")

Signed-off-by Jarkko Sakkinen <[email protected]>

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

drivers/char/tpm/tpm_vtpm_proxy.c +48 -57

index 30e953988cab..8f43a82e5590 100644
@@ -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;

drivers/char/tpm/tpm_vtpm_proxy.c +5 -23

+
+	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'.
   *

drivers/char/tpm/tpm_vtpm_proxy.c +0 -0

  	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;

drivers/char/tpm/tpm_vtpm_proxy.c +0 -3

  	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");

Sent: 30 May 2023 18:46

Quoted 2 lines · Jarkko Sakkinen

From: Jarkko Sakkinen <[email protected]>

drivers/char/tpm/tpm_vtpm_proxy.c static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev); · line 79

Quoted 4 lines

drivers/char/tpm/tpm_vtpm_proxy.c +3 -1

-	rc = copy_to_user(buf, proxy_dev->buffer, len);
+	if (buf)
+		rc = copy_to_user(buf, proxy_dev->buffer, len);
+

Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read().

drivers/char/tpm/tpm_vtpm_proxy.c static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev); · line 79

Quoted 7 lines · Jarkko Sakkinen

From: Jarkko Sakkinen <[email protected]>

drivers/char/tpm/tpm_vtpm_proxy.c +3 -1

-	rc = copy_to_user(buf, proxy_dev->buffer, len);
+	if (buf)
+		rc = copy_to_user(buf, proxy_dev->buffer, len);
+

Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read().

https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/tlb.c#L1279 simple_read_from_buffer will pass the pointer to the user buffer along and it ('to') ends up in copy_to_user(to, ...);

Same here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L41

Quoted 13 lines · Stefan Berger

drivers/char/tpm/tpm_vtpm_proxy.c static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev); · line 79

Quoted 7 lines · Jarkko Sakkinen

From: Jarkko Sakkinen <[email protected]>

drivers/char/tpm/tpm_vtpm_proxy.c +3 -1

-	rc = copy_to_user(buf, proxy_dev->buffer, len);
+	if (buf)
+		rc = copy_to_user(buf, proxy_dev->buffer, len);
+

Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read().

Quoted 4 lines

https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/tlb.c#L1279 simple_read_from_buffer will pass the pointer to the user buffer along and it ('to') ends up in copy_to_user(to, ...);

Same here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L41

It is good to mention here that IMA uses __user tagged pointers correctly, and it does not really compare to the vtpm driver code by any possible means. So let's not add illegit comparison points.

BR, Jarkko