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

[Stefan Berger <stefanb@linux.ibm.com>]

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