Hi Jarkko,
On 3/11/19 6:09 AM, Jarkko Sakkinen wrote:
> Tadeusz, why on earth the code does not lock buffer_mutex?? Just noticed
> when I checked the function in question. It is an awful bug alone.
Because the tpm_common_poll() just reads the flags and doesn't modify
them, so the logic was that if the response_length is not 0 or
response_read is flase then the first poll should return EPOLLIN
and if not, the application should call the poll again and only call read
if the EPOLLIN is set in the mask. It looks like the tpm_timeout_work()
kicks in and messes things out.
Looking at it again I think the response_read flag is redundant
and only the response_length should be used.
>
> Maybe I'm missing something but that is the root cause for this mess
> i.e. race condition with tpm_common_write().
>
> I'm sorry for not noticing that during the review process So
> obviously wrong...
Mantas, could you please give this below a quick try and see if it helps.
I have tested this quickly, but couldn't reproduce the original problem.
If it will help I will send a proper patch.
Thanks,
--- 8< ---
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 5eecad233ea1..496b965c865f 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -63,7 +63,6 @@ static void tpm_timeout_work(struct work_struct *work)
timeout_work);
mutex_lock(&priv->buffer_mutex);
- priv->response_read = true;
priv->response_length = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
@@ -75,7 +74,6 @@ void tpm_common_open(struct file *file, struct tpm_chip *chip,
{
priv->chip = chip;
priv->space = space;
- priv->response_read = true;
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
@@ -95,8 +93,6 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
mutex_lock(&priv->buffer_mutex);
if (priv->response_length) {
- priv->response_read = true;
-
ret_size = min_t(ssize_t, size, priv->response_length);
if (!ret_size) {
priv->response_length = 0;
@@ -140,8 +136,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
* tpm_read or a user_read_timer timeout. This also prevents split
* buffered writes from blocking here.
*/
- if ((!priv->response_read && priv->response_length) ||
- priv->command_enqueued) {
+ if (priv->response_length || priv->command_enqueued) {
ret = -EBUSY;
goto out;
}
@@ -167,7 +162,6 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
}
priv->response_length = 0;
- priv->response_read = false;
*off = 0;
/*
@@ -202,13 +196,15 @@ __poll_t tpm_common_poll(struct file *file, poll_table *wait)
struct file_priv *priv = file->private_data;
__poll_t mask = 0;
+ mutex_lock(&priv->buffer_mutex);
poll_wait(file, &priv->async_wait, wait);
- if (!priv->response_read || priv->response_length)
+ if (priv->response_length)
mask = EPOLLIN | EPOLLRDNORM;
else
mask = EPOLLOUT | EPOLLWRNORM;
+ mutex_unlock(&priv->buffer_mutex);
return mask;
}
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index 1089fc0bb290..403ae3efc6e0 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -15,7 +15,6 @@ struct file_priv {
struct work_struct async_work;
wait_queue_head_t async_wait;
size_t response_length;
- bool response_read;
bool command_enqueued;
u8 data_buffer[TPM_BUFSIZE];
