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: <01edff76-2a66-4543-b1bf-4dc33d46c741@linux.ibm.com>
Date: Thu, 24 Oct 2024 08:59:08 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, linux-integrity@...r.kernel.org,
        Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
        James Bottomley <James.Bottomley@...senPartnership.com>,
        Ard Biesheuvel <ardb@...nel.org>
Cc: David Howells <dhowells@...hat.com>, Mimi Zohar <zohar@...ux.ibm.com>,
        Roberto Sassu <roberto.sassu@...wei.com>, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH v7 4/5] tpm: Allocate chip->auth in
 tpm2_start_auth_session()



On 10/24/24 7:28 AM, Jarkko Sakkinen wrote:
> On Wed Oct 23, 2024 at 10:15 PM EEST, Stefan Berger wrote:
>>
>>
>> On 10/21/24 1:39 AM, Jarkko Sakkinen wrote:
>>> Move allocation of chip->auth to tpm2_start_auth_session() so that the
>>> field can be used as flag to tell whether auth session is active or not.
>>>
>>> Cc: stable@...r.kernel.org # v6.10+
>>> Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
>>> Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
>>> ---
>>> v5:
>>> - No changes.
>>> v4:
>>> - Change to bug.
>>> v3:
>>> - No changes.
>>> v2:
>>> - A new patch.
>>> ---
>>>    drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-------------
>>>    1 file changed, 25 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
>>> index 78c650ce4c9f..6e52785de9fd 100644
>>> --- a/drivers/char/tpm/tpm2-sessions.c
>>> +++ b/drivers/char/tpm/tpm2-sessions.c
>>> @@ -484,7 +484,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8 *pt_v,
>>>    	sha256_final(&sctx, out);
>>>    }
>>>    
>>> -static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
>>> +static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
>>> +				struct tpm2_auth *auth)
>>>    {
>>>    	struct crypto_kpp *kpp;
>>>    	struct kpp_request *req;
>>> @@ -543,7 +544,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
>>>    	sg_set_buf(&s[0], chip->null_ec_key_x, EC_PT_SZ);
>>>    	sg_set_buf(&s[1], chip->null_ec_key_y, EC_PT_SZ);
>>>    	kpp_request_set_input(req, s, EC_PT_SZ*2);
>>> -	sg_init_one(d, chip->auth->salt, EC_PT_SZ);
>>> +	sg_init_one(d, auth->salt, EC_PT_SZ);
>>>    	kpp_request_set_output(req, d, EC_PT_SZ);
>>>    	crypto_kpp_compute_shared_secret(req);
>>>    	kpp_request_free(req);
>>> @@ -554,8 +555,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
>>>    	 * This works because KDFe fully consumes the secret before it
>>>    	 * writes the salt
>>>    	 */
>>> -	tpm2_KDFe(chip->auth->salt, "SECRET", x, chip->null_ec_key_x,
>>> -		  chip->auth->salt);
>>> +	tpm2_KDFe(auth->salt, "SECRET", x, chip->null_ec_key_x, auth->salt);
>>>    
>>>     out:
>>>    	crypto_free_kpp(kpp);
>>> @@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
>>>    			/* manually close the session if it wasn't consumed */
>>>    			tpm2_flush_context(chip, auth->handle);
>>>    		memzero_explicit(auth, sizeof(*auth));
>>> +		kfree(auth);
>>> +		chip->auth = NULL;
>>>    	} else {
>>>    		/* reset for next use  */
>>>    		auth->session = TPM_HEADER_SIZE;
>>> @@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
>>>    
>>>    	tpm2_flush_context(chip, auth->handle);
>>>    	memzero_explicit(auth, sizeof(*auth));
>>> +	kfree(auth);
>>> +	chip->auth = NULL;
>>>    }
>>>    EXPORT_SYMBOL(tpm2_end_auth_session);
>>>    
>>> @@ -970,25 +974,29 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
>>>     */
>>>    int tpm2_start_auth_session(struct tpm_chip *chip)
>>>    {
>>> +	struct tpm2_auth *auth;
>>>    	struct tpm_buf buf;
>>> -	struct tpm2_auth *auth = chip->auth;
>>> -	int rc;
>>>    	u32 null_key;
>>> +	int rc;
>>>    
>>> -	if (!auth) {
>>> -		dev_warn_once(&chip->dev, "auth session is not active\n");
>>> +	if (chip->auth) {
>>> +		dev_warn_once(&chip->dev, "auth session is active\n");
>>>    		return 0;
>>>    	}
>>>    
>>> +	auth = kzalloc(sizeof(*auth), GFP_KERNEL);
>>> +	if (!auth)
>>> +		return -ENOMEM;
>>> +
>>>    	rc = tpm2_load_null(chip, &null_key);
>>>    	if (rc)
>>> -		goto out;
>>> +		goto err;
>>>    
>>>    	auth->session = TPM_HEADER_SIZE;
>>>    
>>>    	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
>>>    	if (rc)
>>> -		goto out;
>>> +		goto err;
>>>    
>>>    	/* salt key handle */
>>>    	tpm_buf_append_u32(&buf, null_key);
>>> @@ -1000,7 +1008,7 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
>>>    	tpm_buf_append(&buf, auth->our_nonce, sizeof(auth->our_nonce));
>>>    
>>>    	/* append encrypted salt and squirrel away unencrypted in auth */
>>> -	tpm_buf_append_salt(&buf, chip);
>>> +	tpm_buf_append_salt(&buf, chip, auth);
>>>    	/* session type (HMAC, audit or policy) */
>>>    	tpm_buf_append_u8(&buf, TPM2_SE_HMAC);
>>>    
>>> @@ -1021,10 +1029,13 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
>>>    
>>>    	tpm_buf_destroy(&buf);
>>>    
>>> -	if (rc)
>>> -		goto out;
>>> +	if (rc == TPM2_RC_SUCCESS) {
>>> +		chip->auth = auth;
>>> +		return 0;
>>> +	}
>>>    
>>> - out:
>>> +err:
>>
>> like in many other cases before kfree(auth):
>> memzero_explicit(auth, sizeof(*auth));
>>
>> With this:
>>
>> Reviewed-by: Stefan Berger <stefanb@...ux.ibm.com>
> 
> Thanks, or should we use kfree_sensitive()?
> 
> It has some additional functionality, which is missed now:
> 
> https://elixir.bootlin.com/linux/v6.11.5/source/mm/slab_common.c#L1339
> 
> I.e. kasan_unpoison().

And change the other ones that use memzero_explicit()?

> 
> BR, Jarkko
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ