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