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

  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.
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().

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

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
  

On 5/29/23 22:01, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi>
> 

> -	rc = copy_to_user(buf, proxy_dev->buffer, len);
> +	if (buf)
> +		rc = copy_to_user(buf, proxy_dev->buffer, len);
> +

Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read().


https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/tlb.c#L1279   simple_read_from_buffer will pass the pointer to the user buffer along and it ('to') ends up in copy_to_user(to, ...);


Same here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L41
  

From: Stefan Berger
> Sent: 30 May 2023 18:46
> 
> On 5/29/23 22:01, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi>
> >
> 
> > -	rc = copy_to_user(buf, proxy_dev->buffer, len);
> > +	if (buf)
> > +		rc = copy_to_user(buf, proxy_dev->buffer, len);
> > +
> 
> Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on
> file_operations.read().

If the user passes NULL the copy_to/from_user() fails and
-EFAULT is returned.

Adding the NULL check makes the request silently succeed.
I doubt that is anywhere near right when you ignore copy_from_user().

I'm not sure what the rational/subject is about either.
copy_to/from_user() calls access_ok() and will fail on
a kernel address.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Tue, 2023-05-30 at 13:45 -0400, Stefan Berger wrote:
> 
> On 5/29/23 22:01, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi>
> > 
> 
> > -	rc = copy_to_user(buf, proxy_dev->buffer, len);
> > +	if (buf)
> > +		rc = copy_to_user(buf, proxy_dev->buffer, len);
> > +
> 
> Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read().
> 
> 
> https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/tlb.c#L1279   simple_read_from_buffer will pass the pointer to the user buffer along and it ('to') ends up in copy_to_user(to, ...);
> 
> 
> Same here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L41

It is good to mention here that IMA uses __user tagged pointers
correctly, and it does not really compare to the vtpm driver code
by any possible means. So let's not add illegit comparison points.

BR, Jarkko
  

On 5/31/23 13:17, Jarkko Sakkinen wrote:
> On Tue, 2023-05-30 at 13:45 -0400, Stefan Berger wrote:
>>
>> On 5/29/23 22:01, Jarkko Sakkinen wrote:
>>> From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi>
>>>
>>
>>> -	rc = copy_to_user(buf, proxy_dev->buffer, len);
>>> +	if (buf)
>>> +		rc = copy_to_user(buf, proxy_dev->buffer, len);
>>> +
>>
>> Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read().
>>
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/tlb.c#L1279   simple_read_from_buffer will pass the pointer to the user buffer along and it ('to') ends up in copy_to_user(to, ...);
>>
>>
>> Same here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L41
> 
> It is good to mention here that IMA uses __user tagged pointers
> correctly, and it does not really compare to the vtpm driver code
> by any possible means. So let's not add illegit comparison points.

Yes, sir. Did you read David' response?
> 
> BR, Jarkko