| Message ID | 155294749695.20367.14472779462229450620.stgit@tstruk-mobl1.jf.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: fix a race between poll and write in tpm-dev-common | expand |
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
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,
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
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
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; }
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(+)