| 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: <9c498e15-9de1-ce52-61a8-414a4cf7195f@linux.ibm.com>
Date: Tue, 30 May 2023 13:36:54 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Jarkko Sakkinen <jarkko@...nel.org>,
linux-integrity@...r.kernel.org
Cc: 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 RFC v2] tpm: tpm_vtpm_proxy: do not reference kernel
memory as user memory
On 5/29/23 22:01, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@...i.fi>
>
> The driver has two issues (in priority order) in the locality change:
>
> 1. The driver uses __user pointer and copy_to_user() and
> copy_from_user() with a kernel address during the locality change.
This can lead to ... bad things ?
> 2. For invalid locality change request from user space, the driver
> sets errno to EFAULT, while for invalid input data EINVAL should
> be used.
>
> Address this by:
>
> 1. Introduce __vtpm_proxy_read_unlocked(), __vtpm_proxy_write_unlocked()
> and __vtpm_proxy_read_write_locked().
> 2. Make locality change atomic by calling __vtpm_proxy_read_write_locked(),
> instead of tpm_transmit_cmd().
2. sounds like it should be addressable by changing -EFAULT to -EINVAL but there's more to it?
>
> Cc: stable@...r.kernel.org
> Fixes: be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...i.fi>
> ---
> v2:
> * Acquiring and releasing mutex in-between should not be an issue
> because they are executed with the chip locked.
> ---
> drivers/char/tpm/tpm_vtpm_proxy.c | 162 ++++++++++++++----------------
> 1 file changed, 73 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 30e953988cab..8f43a82e5590 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,106 +57,112 @@ static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev);
> * Functions related to 'server side'
> */
>
> -/**
> - * 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)
> +static ssize_t __vtpm_proxy_read_unlocked(struct proxy_dev *proxy_dev, char __user *buf,
> + size_t count)
> {
> - 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);
> + int rc;
>
> - if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
> - mutex_unlock(&proxy_dev->buf_lock);
> + if (!(proxy_dev->state & STATE_OPENED_FLAG))
> 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);
> + if (buf)
> + 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;
> }
>
> -/**
> - * vtpm_proxy_fops_write - Write TPM responses on 'server side'
> - *
> - * @filp: file pointer
> - * @buf: write buffer
> - * @count: number of bytes to write
> - * @off: offset
> - *
> - * Return:
> - * Number of bytes read or negative error value
> - */
> -static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf,
> - size_t count, loff_t *off)
> +static ssize_t __vtpm_proxy_write_unlocked(struct proxy_dev *proxy_dev, const char __user *buf,
> + size_t count)
> {
> - struct proxy_dev *proxy_dev = filp->private_data;
> -
> - mutex_lock(&proxy_dev->buf_lock);
> -
> - if (!(proxy_dev->state & STATE_OPENED_FLAG)) {
> - mutex_unlock(&proxy_dev->buf_lock);
> + if (!(proxy_dev->state & STATE_OPENED_FLAG))
> return -EPIPE;
> - }
>
> if (count > sizeof(proxy_dev->buffer) ||
> - !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG)) {
> - mutex_unlock(&proxy_dev->buf_lock);
> + !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG))
> return -EIO;
> - }
>
> proxy_dev->state &= ~STATE_WAIT_RESPONSE_FLAG;
>
> proxy_dev->req_len = 0;
>
> - if (copy_from_user(proxy_dev->buffer, buf, count)) {
> - mutex_unlock(&proxy_dev->buf_lock);
> + if (buf && copy_from_user(proxy_dev->buffer, buf, count))
> return -EFAULT;
> - }
>
> proxy_dev->resp_len = count;
> + return count;
> +}
>
> +static ssize_t __vtpm_proxy_read_write_unlocked(struct proxy_dev *proxy_dev, char __user *buf,
> + size_t count)
> +{
> + ssize_t rc;
> +
> + do {
> + rc = __vtpm_proxy_write_unlocked(proxy_dev, buf, count);
> + if (rc < 0)
> + break;
> + rc = __vtpm_proxy_read_unlocked(proxy_dev, buf, rc);
> + } while (0);
> +
> + return rc;
> +}
> +
> +/*
> + * See struct file_operations.
> + */
> +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;
> + ssize_t rc;
> + int sig;
> +
> + 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);
> + rc = __vtpm_proxy_read_unlocked(proxy_dev, buf, count);
> mutex_unlock(&proxy_dev->buf_lock);
>
> + return rc;
> +}
> +
> +/*
> + * See struct file_operations.
> + */
> +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;
> + int rc;
ssize_t rc;
> +
> + mutex_lock(&proxy_dev->buf_lock);
> + rc = __vtpm_proxy_write_unlocked(proxy_dev, buf, count);
> + mutex_unlock(&proxy_dev->buf_lock);
> wake_up_interruptible(&proxy_dev->wq);
>
> - return count;
> + return rc;
> }
>
> /*
> @@ -295,28 +300,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 +313,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;
'u32 ordinal' like in tpm_try_transmit ?
you need this here in some form: be32_to_cpu(hdr->ordinal)
>
> if (count > sizeof(proxy_dev->buffer)) {
> dev_err(&chip->dev,(chip->flags & TPM_CHIP_FLAG_TPM2)
> @@ -338,9 +322,11 @@ 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))
> - return -EFAULT;
> + if ((chip->flags & TPM_CHIP_FLAG_TPM2) && ord == TPM2_CC_SET_LOCALITY)
> + return -EINVAL;
> +
> + if (ord == TPM_ORD_SET_LOCALITY)
> + return -EINVAL;
if ((chip->flags & TPM_CHIP_FLAG_TPM2)) {
if (ord == )
return -EINVAL;
} else if (ord == ) {
return -EINVAL;
}
>
> mutex_lock(&proxy_dev->buf_lock);
>
> @@ -409,12 +395,10 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
> 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;
> -
> + mutex_lock(&proxy_dev->buf_lock);
> + memcpy(proxy_dev->buffer, buf.data, tpm_buf_length(&buf));
> + rc = __vtpm_proxy_read_write_unlocked(proxy_dev, NULL, tpm_buf_length(&buf));
> + mutex_unlock(&proxy_dev->buf_lock);
> if (rc < 0) {
> locality = rc;
> goto out;
Stefan
Powered by blists - more mailing lists