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

On 5/29/23 22:01, Jarkko Sakkinen wrote:

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.

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@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;

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