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]
Date: Fri, 23 Feb 2024 02:01:32 +0200
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Daniel P. Smith" <dpsmith@...rtussolutions.com>, "Jason Gunthorpe"
 <jgg@...pe.ca>, "Sasha Levin" <sashal@...nel.org>, "Lino Sanfilippo"
 <l.sanfilippo@...bus.com>, <linux-integrity@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Cc: "Ross Philipson" <ross.philipson@...cle.com>, "Kanth Ghatraju"
 <kanth.ghatraju@...cle.com>, "Peter Huewe" <peterhuewe@....de>
Subject: Re: [PATCH 1/3] tpm: protect against locality counter underflow

On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote:
> Commit 933bfc5ad213 introduced the use of a locality counter to control when a
> locality request is allowed to be sent to the TPM. In the commit, the counter
> is indiscriminately decremented. Thus creating a situation for an integer
> underflow of the counter.
>
> Signed-off-by: Daniel P. Smith <dpsmith@...rtussolutions.com>
> Signed-off-by: Ross Philipson <ross.philipson@...cle.com>
> Reported-by: Kanth Ghatraju <kanth.ghatraju@...cle.com>
> Fixes: 933bfc5ad213 ("tpm, tpm: Implement usage counter for locality")
> ---
>  drivers/char/tpm/tpm_tis_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 1b350412d8a6..4176d3bd1f04 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -180,7 +180,8 @@ static int tpm_tis_relinquish_locality(struct tpm_chip *chip, int l)
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  
>  	mutex_lock(&priv->locality_count_mutex);
> -	priv->locality_count--;
> +	if (priv->locality_count > 0)
> +		priv->locality_count--;
>  	if (priv->locality_count == 0)
>  		__tpm_tis_relinquish_locality(priv, l);
>  	mutex_unlock(&priv->locality_count_mutex);

To make this patch set better the whole story should be scenario based.

Starting from cover letter the explanation is way too rounded to guide
to the conclusion that these are actually best possible code changes to
fix the issue. 

I agree fully on that the problem should be fixed but given that the
scenarios are fuzzy deciding whether this the right things done right
is the open question.

I.e. we need the steps for destruction and how these patches change
those steps to make this right. Since the whole topic is complicated
I'd use PC Client spec as reference.

BR, Jarkko

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ