[PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jarkko Sakkinen]

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);
 	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)) {
+		kfree(kernel_buf);
 		return -EFAULT;
 	}
 
-	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

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jarkko Sakkinen]

On Tue, 2023-05-30 at 23:50 +0300, Jarkko Sakkinen wrote:
> -	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;

Oops, should really be:

diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 83496742cc19..6da878952a0d 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -337,8 +337,7 @@ static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
 
 	if ((chip->flags & TPM_CHIP_FLAG_TPM2) && ord == TPM2_CC_SET_LOCALITY)
 		return -EFAULT;
-
-	if (ord == TPM_ORD_SET_LOCALITY)
+	else if (ord == TPM_ORD_SET_LOCALITY) /* TPM 1.2 */
 		return -EFAULT;
 
 	mutex_lock(&proxy_dev->buf_lock);


BR, Jarkko

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Stefan Berger]

On 5/30/23 16:50, 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().

And what is the problem with that? Is it not working?
> 
> Factor out the crippled code away with help of an internal API for
> managing struct proxy_dev instances.

What is crippled code?

    Stefan

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jarkko Sakkinen]

On Wed, 2023-05-31 at 11:20 -0400, Stefan Berger wrote:
> 
> On 5/30/23 16:50, 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().
> 
> And what is the problem with that? Is it not working?

It is API contract and also clearly documented in the kernel documentation.

This should be obvious even if you have've consulted that documentation because
both functions have 'user' suffix, and also the pointer is __user tagged.

To make things worse it is architecture specific. I'm worried that it will
break in one of the 23 microarchitectures. Have you actually ever checked it
does not?

I'm not also an expert of how all the possible CPUs in the world empower
Linux to further restrict the move between different memory spaces. I'm
quite sure that this does conflict neither with SMAP or SMEP on x86
(because I know x86 pretty well), but who knows what they add in the
future to the microarchitecture.

> > Factor out the crippled code away with help of an internal API for
> > managing struct proxy_dev instances.
> 
> What is crippled code?

Code that behaves badly, i.e. does not meat the expectations. Illegit use of
in-kernel functions easily fits to the definition of crippled code.

Bad API behavior put aside, it is very inefficient implementation because it
unnecessarily recurses tpm_transmit(), which makes extending the driver to
any direction so much involved process, but we don't really need this as a
rationale.

This needs to be fixed in a way or another. That is dictated by the API
cotract so for that I do not really even need feedback because it is
force majeure. I'm cool with alternatives or suggestions to the current
fact, so please focus on that instead of asking question that kernel
documentation provides you already all the answers.

BR, Jarkko

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jarkko Sakkinen]

On Wed, 2023-05-31 at 19:32 +0300, Jarkko Sakkinen wrote:
> On Wed, 2023-05-31 at 11:20 -0400, Stefan Berger wrote:
> > 
> > On 5/30/23 16:50, 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().
> > 
> > And what is the problem with that? Is it not working?
> 
> It is API contract and also clearly documented in the kernel documentation.
> 
> This should be obvious even if you have've consulted that documentation because
> both functions have 'user' suffix, and also the pointer is __user tagged.
> 
> To make things worse it is architecture specific. I'm worried that it will
> break in one of the 23 microarchitectures. Have you actually ever checked it
> does not?
> 
> I'm not also an expert of how all the possible CPUs in the world empower
> Linux to further restrict the move between different memory spaces. I'm
> quite sure that this does conflict neither with SMAP or SMEP on x86
> (because I know x86 pretty well), but who knows what they add in the
> future to the microarchitecture.
> 
> > > Factor out the crippled code away with help of an internal API for
> > > managing struct proxy_dev instances.
> > 
> > What is crippled code?
> 
> Code that behaves badly, i.e. does not meat the expectations. Illegit use of
> in-kernel functions easily fits to the definition of crippled code.
> 
> Bad API behavior put aside, it is very inefficient implementation because it
> unnecessarily recurses tpm_transmit(), which makes extending the driver to
> any direction so much involved process, but we don't really need this as a
> rationale.
> 
> This needs to be fixed in a way or another. That is dictated by the API
> cotract so for that I do not really even need feedback because it is
> force majeure. I'm cool with alternatives or suggestions to the current
> fact, so please focus on that instead of asking question that kernel
> documentation provides you already all the answers.

I have to add that it has not been too critical because afaik tpm_vtpm_proxy
is for the most part for development use, and less of so production. This is
just to say that overally we've been happy with the driver in its use cases
but any snippet of code always has an expiration date.

BR, Jarkko

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Stefan Berger]

On 5/31/23 12:32, Jarkko Sakkinen wrote:
> On Wed, 2023-05-31 at 11:20 -0400, Stefan Berger wrote:
>>
>> On 5/30/23 16:50, 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().
>>
>> And what is the problem with that? Is it not working?
> It is API contract and also clearly documented in the kernel documentation.

First, vtpm_proxy_fops_set_locality() does not exist

This may  be the function that is simulating a client sending a SET_LOCALITY command:

static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
{
	struct tpm_buf buf;
	int rc;
	const struct tpm_header *header;
	struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev);

	if (chip->flags & TPM_CHIP_FLAG_TPM2)
		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
				  TPM2_CC_SET_LOCALITY);
	else
		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
				  TPM_ORD_SET_LOCALITY);
	if (rc)
		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;

	if (rc < 0) {
		locality = rc;
		goto out;
	}

	header = (const struct tpm_header *)buf.data;
	rc = be32_to_cpu(header->return_code);
	if (rc)
		locality = -1;

out:
	tpm_buf_destroy(&buf);

	return locality;
}

There is nothing wrong with the buffer being passed into the tpm_transmit_cmd function, which then causes the 'server side' to pick up the command (= swtpm picks up the command):

/**
  * 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)
{
	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);

	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;

	if (!rc)
		proxy_dev->state |= STATE_WAIT_RESPONSE_FLAG;

	mutex_unlock(&proxy_dev->buf_lock);

	if (rc)
		return -EFAULT;

	return len;
}

This is swtpm picking up this command with its user buffer.

   So, I am not sure at this point what is wrong.

    Stefan

> 
> This should be obvious even if you have've consulted that documentation because
> both functions have 'user' suffix, and also the pointer is __user tagged.
> 
> To make things worse it is architecture specific. I'm worried that it will
> break in one of the 23 microarchitectures. Have you actually ever checked it
> does not?
> 
> I'm not also an expert of how all the possible CPUs in the world empower
> Linux to further restrict the move between different memory spaces. I'm
> quite sure that this does conflict neither with SMAP or SMEP on x86
> (because I know x86 pretty well), but who knows what they add in the
> future to the microarchitecture.
> 
>>> Factor out the crippled code away with help of an internal API for
>>> managing struct proxy_dev instances.
>>
>> What is crippled code?
> 
> Code that behaves badly, i.e. does not meat the expectations. Illegit use of
> in-kernel functions easily fits to the definition of crippled code.
> 
> Bad API behavior put aside, it is very inefficient implementation because it
> unnecessarily recurses tpm_transmit(), which makes extending the driver to
> any direction so much involved process, but we don't really need this as a
> rationale.
> 
> This needs to be fixed in a way or another. That is dictated by the API
> cotract so for that I do not really even need feedback because it is
> force majeure. I'm cool with alternatives or suggestions to the current
> fact, so please focus on that instead of asking question that kernel
> documentation provides you already all the answers.
> 
> BR, Jarkko

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jarkko Sakkinen]

On Wed May 31, 2023 at 8:01 PM EEST, Stefan Berger wrote:
>
>
> On 5/31/23 12:32, Jarkko Sakkinen wrote:
> > On Wed, 2023-05-31 at 11:20 -0400, Stefan Berger wrote:
> >>
> >> On 5/30/23 16:50, 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().
> >>
> >> And what is the problem with that? Is it not working?
> > It is API contract and also clearly documented in the kernel documentation.
>
> First, vtpm_proxy_fops_set_locality() does not exist
>
> This may  be the function that is simulating a client sending a SET_LOCALITY command:
>
> static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
> {
> 	struct tpm_buf buf;
> 	int rc;
> 	const struct tpm_header *header;
> 	struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev);
>
> 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS,
> 				  TPM2_CC_SET_LOCALITY);
> 	else
> 		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
> 				  TPM_ORD_SET_LOCALITY);
> 	if (rc)
> 		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;
>
> 	if (rc < 0) {
> 		locality = rc;
> 		goto out;
> 	}
>
> 	header = (const struct tpm_header *)buf.data;
> 	rc = be32_to_cpu(header->return_code);
> 	if (rc)
> 		locality = -1;
>
> out:
> 	tpm_buf_destroy(&buf);
>
> 	return locality;
> }
>
> There is nothing wrong with the buffer being passed into the tpm_transmit_cmd function, which then causes the 'server side' to pick up the command (= swtpm picks up the command):
>
> /**
>   * 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)
> {
> 	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);
>
> 	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;
>
> 	if (!rc)
> 		proxy_dev->state |= STATE_WAIT_RESPONSE_FLAG;
>
> 	mutex_unlock(&proxy_dev->buf_lock);
>
> 	if (rc)
> 		return -EFAULT;
>
> 	return len;
> }
>
> This is swtpm picking up this command with its user buffer.
>
>    So, I am not sure at this point what is wrong.
>
>     Stefan

The answer was below but in short it is that you have a function that
expects __user * and you don't pass user tagged memory.

Even tho it is a bug, I think cc to stable is not necessary given that
it is not known to blow up anything. The main problem is that we have
code that does not work according to the expectations.

BR, Jarkko

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Stefan Berger]

On 6/8/23 09:14, Jarkko Sakkinen wrote:
> On Wed May 31, 2023 at 8:01 PM EEST, Stefan Berger wrote:
>>
>>
>
>>
>> This is swtpm picking up this command with its user buffer.
>>
>>     So, I am not sure at this point what is wrong.
>>
>>      Stefan
> 
> The answer was below but in short it is that you have a function that
> expects __user * and you don't pass user tagged memory.

There are two functions that expect user tagged memory:

static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf,
				    size_t count, loff_t *off)
static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf,
				     size_t count, loff_t *off)

the correspond to this interface:

struct file_operations {
	struct module *owner;
	loff_t (*llseek) (struct file *, loff_t, int);
	ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
	ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);

defined here:

static const struct file_operations vtpm_proxy_fops = {
	.owner = THIS_MODULE,
	.llseek = no_llseek,
	.read = vtpm_proxy_fops_read,
	.write = vtpm_proxy_fops_write,

Conversely, I see no other function interfaces in tpm_vtpm_proxy.c where the code would be missing the __user.

Neither do I see any functions where I am passing a __user tagged buffer as parameter that shouldn't have
such a tag on it or the reverse where a plain buffer is passed and it should be a __user tagged buffer.

    Stefan

> 
> Even tho it is a bug, I think cc to stable is not necessary given that
> it is not known to blow up anything. The main problem is that we have
> code that does not work according to the expectations.
> 
> BR, Jarkko
> 
> 

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jarkko Sakkinen]

On Thu Jun 8, 2023 at 6:10 PM EEST, Stefan Berger wrote:
>
>
> On 6/8/23 09:14, Jarkko Sakkinen wrote:
> > On Wed May 31, 2023 at 8:01 PM EEST, Stefan Berger wrote:
> >>
> >>
> >
> >>
> >> This is swtpm picking up this command with its user buffer.
> >>
> >>     So, I am not sure at this point what is wrong.
> >>
> >>      Stefan
> > 
> > The answer was below but in short it is that you have a function that
> > expects __user * and you don't pass user tagged memory.
>
> There are two functions that expect user tagged memory:
>
> static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf,
> 				    size_t count, loff_t *off)
> static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf,
> 				     size_t count, loff_t *off)
>
> the correspond to this interface:
>
> struct file_operations {
> 	struct module *owner;
> 	loff_t (*llseek) (struct file *, loff_t, int);
> 	ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
> 	ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
>
> defined here:
>
> static const struct file_operations vtpm_proxy_fops = {
> 	.owner = THIS_MODULE,
> 	.llseek = no_llseek,
> 	.read = vtpm_proxy_fops_read,
> 	.write = vtpm_proxy_fops_write,
>
> Conversely, I see no other function interfaces in tpm_vtpm_proxy.c where the code would be missing the __user.
>
> Neither do I see any functions where I am passing a __user tagged buffer as parameter that shouldn't have
> such a tag on it or the reverse where a plain buffer is passed and it should be a __user tagged buffer.

Uh oh, you're correct. I was looking this in the context of my user
vtpm driver changes, so that confused me, so it is actually my bad.
So blame is on my side.

I would still want to open code the whole thing because there is no
need to cycle it through tpm_transmit_cmd() but I'll do it in the
context of other changes.

It is pretty complicated sequence and makes hard to build anything
on top of existing functionality, so it needs to be simplified in
any case... But you are right: it is not a bug.

BR, Jarkko

Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jerry Snitselaar]

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)) {
> +		kfree(kernel_buf);
>  		return -EFAULT;
>  	}
>  
> -	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
>