On Tue, May 30, 2023 at 11:50:01PM +0300, 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(). > > 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@xxxxxxxxxx/ > Cc: stable@xxxxxxxxxxxxxxx # v4.14+ > Fixes: be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.") > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxx> > --- > 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 >
