2019-03-18 22:19:24

by Tadeusz Struk

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

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 <[email protected]>
Signed-off-by: Tadeusz Struk <[email protected]>
---
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;
}

2019-03-18 23:21:36

by James Bottomley

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

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 <[email protected]>
> Signed-off-by: Tadeusz Struk <[email protected]>
> ---
> 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

2019-03-19 19:10:27

by Tadeusz Struk

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

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

2019-03-21 13:35:47

by Jarkko Sakkinen

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

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 <[email protected]>
> > Signed-off-by: Tadeusz Struk <[email protected]>
> > ---
> > 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

2019-03-21 13:36:23

by Jarkko Sakkinen

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

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 <[email protected]>
> Signed-off-by: Tadeusz Struk <[email protected]>

Also, please add Cc: [email protected] as the first tag.

/Jarkko