Re: [PATCH RFC v2] tpm: tpm_vtpm_proxy: do not reference kernel memory as user memory

[Stefan Berger <stefanb@linux.ibm.com>]

On 5/29/23 22:01, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@tuni.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@vger.kernel.org
> Fixes: be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@tuni.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