Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Thu, 08 Jun 2023 16:14:56 +0300
Message-Id: <CT7AOKF4OGHA.2S5VUEAG76GYB@suppilovahvero>
Subject: Re: [PATCH] tpm: factor out the user space mm from
 tpm_vtpm_set_locality()
From:   "Jarkko Sakkinen" <jarkko@kernel.org>
To:     "Stefan Berger" <stefanb@linux.ibm.com>,
        <linux-integrity@vger.kernel.org>
Cc:     "Jason Gunthorpe" <jgg@nvidia.com>,
        "Alejandro Cabrera" <alejandro.cabreraaldaya@tuni.fi>,
        "Jarkko Sakkinen" <jarkko.sakkinen@tuni.fi>,
        <stable@vger.kernel.org>,
        "Stefan Berger" <stefanb@linux.vnet.ibm.com>,
        <linux-kernel@vger.kernel.org>
References: <20230530205001.1302975-1-jarkko@kernel.org>
 <8f15feb5-7c6e-5a16-d9b4-008b7b45b01a@linux.ibm.com>
 <324df0fa5ad1f0508c5f62c25dd1f8d297d78813.camel@kernel.org>
 <0438f5e3-ca42-343b-e79e-5f7976ec8a62@linux.ibm.com>
In-Reply-To: <0438f5e3-ca42-343b-e79e-5f7976ec8a62@linux.ibm.com>
Precedence: bulk

On Wed May 31, 2023 at 8:01 PM EEST, Stefan Berger wrote:
>
>
> On 5/31/23 12:32, Jarkko Sakkinen wrote:
> > On Wed, 2023-05-31 at 11:20 -0400, Stefan Berger wrote:
> >>
> >> On 5/30/23 16:50, Jarkko Sakkinen wrote:
> >>> From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi>
> >>>
> >>> vtpm_proxy_fops_set_locality() causes kernel buffers to be passed to
> >>> copy_from_user() and copy_to_user().
> >>
> >> And what is the problem with that? Is it not working?
> > It is API contract and also clearly documented in the kernel documentat=
ion.
>
> First, vtpm_proxy_fops_set_locality() does not exist
>
> This may  be the function that is simulating a client sending a SET_LOCAL=
ITY command:
>
> static int vtpm_proxy_request_locality(struct tpm_chip *chip, int localit=
y)
> {
> 	struct tpm_buf buf;
> 	int rc;
> 	const struct tpm_header *header;
> 	struct proxy_dev *proxy_dev =3D dev_get_drvdata(&chip->dev);
>
> 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 		rc =3D tpm_buf_init(&buf, TPM2_ST_SESSIONS,
> 				  TPM2_CC_SET_LOCALITY);
> 	else
> 		rc =3D tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND,
> 				  TPM_ORD_SET_LOCALITY);
> 	if (rc)
> 		return rc;
> 	tpm_buf_append_u8(&buf, locality);
>
> 	proxy_dev->state |=3D STATE_DRIVER_COMMAND;
>
> 	rc =3D tpm_transmit_cmd(chip, &buf, 0, "attempting to set locality");
>
> 	proxy_dev->state &=3D ~STATE_DRIVER_COMMAND;
>
> 	if (rc < 0) {
> 		locality =3D rc;
> 		goto out;
> 	}
>
> 	header =3D (const struct tpm_header *)buf.data;
> 	rc =3D be32_to_cpu(header->return_code);
> 	if (rc)
> 		locality =3D -1;
>
> out:
> 	tpm_buf_destroy(&buf);
>
> 	return locality;
> }
>
> There is nothing wrong with the buffer being passed into the tpm_transmit=
_cmd function, which then causes the 'server side' to pick up the command (=
=3D swtpm picks up the command):
>
> /**
>   * 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)
> {
> 	struct proxy_dev *proxy_dev =3D filp->private_data;
> 	size_t len;
> 	int sig, rc;
>
> 	sig =3D wait_event_interruptible(proxy_dev->wq,
> 		proxy_dev->req_len !=3D 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 =3D 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=3D%zd, req_len=3D%zd\n",
> 			 count, len);
> 		return -EIO;
> 	}
>
> 	rc =3D copy_to_user(buf, proxy_dev->buffer, len);
> 	memset(proxy_dev->buffer, 0, len);
> 	proxy_dev->req_len =3D 0;
>
> 	if (!rc)
> 		proxy_dev->state |=3D STATE_WAIT_RESPONSE_FLAG;
>
> 	mutex_unlock(&proxy_dev->buf_lock);
>
> 	if (rc)
> 		return -EFAULT;
>
> 	return len;
> }
>
> This is swtpm picking up this command with its user buffer.
>
>    So, I am not sure at this point what is wrong.
>
>     Stefan

The answer was below but in short it is that you have a function that
expects __user * and you don't pass user tagged memory.

Even tho it is a bug, I think cc to stable is not necessary given that
it is not known to blow up anything. The main problem is that we have
code that does not work according to the expectations.

BR, Jarkko