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

Stefan Berger
2023-05-31T17:05:22Z

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