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