Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jerry Snitselaar]

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
>