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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1552951167.2785.22.camel@HansenPartnership.com>
Date:   Mon, 18 Mar 2019 16:19:27 -0700
From:   James Bottomley <James.Bottomley@...senPartnership.com>
To:     Tadeusz Struk <tadeusz.struk@...el.com>,
        jarkko.sakkinen@...ux.intel.com
Cc:     grawity@...il.com, linux-integrity@...r.kernel.org,
        linux-kernel@...r.kernel.org
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 <grawity@...il.com>
> Signed-off-by: Tadeusz Struk <tadeusz.struk@...el.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