[PATCH] tpm: fix a race between poll and write in tpm-dev-common

[PATCH] tpm: fix a race between poll and write in tpm-dev-common

[Tadeusz Struk]

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);
 	return mask;
 }
 

Re: [PATCH] tpm: fix a race between poll and write in tpm-dev-common

[James Bottomley]

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

Re: [PATCH] tpm: fix a race between poll and write in tpm-dev-common

[Tadeusz Struk]

On 3/18/19 4:19 PM, James Bottomley wrote:
>> @@ -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?

Right, it needs to be guarded by the mutex and also the condition
should only check priv->response_length, because we only care
about if there is data to read. The response_read flag only
prevents double writes, without reading in the middle (or a timeout)
which clean it. I will send a v2 soon.

Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: fix a race between poll and write in tpm-dev-common

[Jarkko Sakkinen]

On Mon, Mar 18, 2019 at 04:19:27PM -0700, James Bottomley wrote:
> 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?

I see now what you mean.

Tadeusz: before you send a new patch put this comment to that place
as a reminder:

/* Checking only response_length is correct because write() always zeros
 * it and poll() should succeed after the first partial read.
 */

/Jarkko

Re: [PATCH] tpm: fix a race between poll and write in tpm-dev-common

[Jarkko Sakkinen]

On Mon, Mar 18, 2019 at 03:18:16PM -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>

Also, please add Cc: stable@vger.kernel.org as the first tag.

/Jarkko