Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1552951167.2785.22.camel@HansenPartnership.com>
Subject: Re: [PATCH] tpm: fix a race between poll and write in tpm-dev-common
From:   James Bottomley <James.Bottomley@HansenPartnership.com>
To:     Tadeusz Struk <tadeusz.struk@intel.com>,
        jarkko.sakkinen@linux.intel.com
Cc:     grawity@gmail.com, linux-integrity@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Mon, 18 Mar 2019 16:19:27 -0700
In-Reply-To: <155294749695.20367.14472779462229450620.stgit@tstruk-mobl1.jf.intel.com>
References: <155294749695.20367.14472779462229450620.stgit@tstruk-mobl1.jf.intel.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 2019-03-18 at 15:18 -0700, Tadeusz Struk wrote:
> Since the poll returns EPOLLIN base on the state of two
> variables, the response_read being false and the
> response_length > 0 the poll needs to take the buffer_mutex
> after it is woken up.
> 
> Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
> Reported-by: Mantas Mikulėnas <grawity@gmail.com>
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
> ---
>  drivers/char/tpm/tpm-dev-common.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c
> b/drivers/char/tpm/tpm-dev-common.c
> index 5eecad233ea1..61e458d6f652 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -203,12 +203,14 @@ __poll_t tpm_common_poll(struct file *file,
> poll_table *wait)
>  	__poll_t mask = 0;
>  
>  	poll_wait(file, &priv->async_wait, wait);
> +	mutex_lock(&priv->buffer_mutex);
>  
>  	if (!priv->response_read || priv->response_length)
>  		mask = EPOLLIN | EPOLLRDNORM;
>  	else
>  		mask = EPOLLOUT | EPOLLWRNORM;
>  
> +	mutex_unlock(&priv->buffer_mutex);

This doesn't do anything to address the theory that the queued work
hasn't run before the poll wakes up, does it?  If you have an
alternative theory, could you explain it?

Thanks,

James