| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4wa3nb6h6ns3msp4o6wojhxwn37t5sxlgexacm7ga7rfcrnsfl@chr5qhednjdl>
Date: Wed, 31 May 2023 08:53:11 -0700
From: Jerry Snitselaar <jsnitsel@...hat.com>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: linux-integrity@...r.kernel.org, Jason Gunthorpe <jgg@...dia.com>,
Alejandro Cabrera <alejandro.cabreraaldaya@...i.fi>,
Jarkko Sakkinen <jarkko.sakkinen@...i.fi>,
stable@...r.kernel.org, Stefan Berger <stefanb@...ux.vnet.ibm.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tpm: factor out the user space mm from
tpm_vtpm_set_locality()
On Tue, May 30, 2023 at 11:50:01PM +0300, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@...i.fi>
>
> 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@kernel.org/
> Cc: stable@...r.kernel.org # v4.14+
> Fixes: be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...i.fi>
> ---
> 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
>
Powered by blists - more mailing lists