linux-kernel - Re: [EXTERNAL] [PATCH v2 6/6] keys: asymmetric: ASYMMETRIC_TPM2_KEY_RSA

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>] [day] [month] [year] [list]

Message-Id: <D1F5GQYP9JFC.3MXMKUM0CKGS5@kernel.org>
Date: Tue, 21 May 2024 10:38:45 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Bharat Bhushan" <bbhushan2@...vell.com>, "Herbert Xu"
 <herbert@...dor.apana.org.au>
Cc: "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
 "keyrings@...r.kernel.org" <keyrings@...r.kernel.org>,
 "Andreas.Fuchs@...ineon.com" <Andreas.Fuchs@...ineon.com>, "James
 Prestwood" <prestwoj@...il.com>, "David Woodhouse" <dwmw2@...radead.org>,
 "Eric Biggers" <ebiggers@...nel.org>, "James Bottomley"
 <James.Bottomley@...senpartnership.com>, "David S. Miller"
 <davem@...emloft.net>, "open list:CRYPTO API"
 <linux-crypto@...r.kernel.org>, "open list" <linux-kernel@...r.kernel.org>,
 "David Howells" <dhowells@...hat.com>, "James Bottomley"
 <James.Bottomley@...senPartnership.com>, "Stefan Berger"
 <stefanb@...ux.ibm.com>, "Ard Biesheuvel" <ardb@...nel.org>, "Mario
 Limonciello" <mario.limonciello@....com>
Subject: Re: [EXTERNAL] [PATCH v2 6/6] keys: asymmetric:
 ASYMMETRIC_TPM2_KEY_RSA_SUBTYPE

On Tue May 21, 2024 at 10:25 AM EEST, Bharat Bhushan wrote:
> > +	rc = crypto_akcipher_encrypt(req);
> > +	rc = crypto_wait_req(rc, &cwait);
> > +
>
> Few Minor comments, 
> Extra line here 

Yeah, makes sense.


> > +	if (!rc)
> > +		rc = req->dst_len;
> > +
> > +	akcipher_request_free(req);
> > +
> > +err_tfm:
> > +	crypto_free_akcipher(tfm);
> > +
> > +	return rc;
> > +}
> > +
> > +static int __tpm2_key_rsa_decrypt(struct tpm_chip *chip,
> > +				  struct tpm2_key_rsa *key,
> > +				  struct kernel_pkey_params *params,
> > +				  const void *in, int in_len, void *out)
> > +{
> > +	unsigned int offset = 0;
> > +	u32 key_handle = 0;
> > +	struct tpm_buf buf;
> > +	u16 decrypted_len;
> > +	u32 parent;
> > +	u8 *pos;
> > +	int ret;
> > +
> > +	ret = tpm_try_get_ops(chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tpm2_start_auth_session(chip);
> > +	if (ret)
> > +		goto err_ops;
> > +
> > +	if (key->key.parent == TPM2_RH_NULL) {
> > +		ret = tpm2_load_context(chip, chip->null_key_context,
> > &offset,
> > +					&parent);
> > +		if (ret) {
> > +			ret = -EIO;
> > +			goto err_auth;
> > +		}
> > +	} else {
> > +		parent = key->key.parent;
> > +	}
>
> Do we need braces here?


I think I added them because checkpatch complained me about not having
them. So I guess the rule is that if any branch has braces, all of them
should have...
>
> > +
> > +	ret = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
> > +	if (ret < 0)
> > +		goto err_parent;
> > +
> > +	tpm_buf_append_name(chip, &buf, parent, NULL);
> > +	tpm_buf_append_hmac_session(chip, &buf,
> > TPM2_SA_CONTINUE_SESSION |
> > +				    TPM2_SA_ENCRYPT, NULL, 0);
> > +	tpm_buf_append(&buf, key->key.blob, key->key.blob_len);
> > +	if (buf.flags & TPM_BUF_OVERFLOW) {
> > +		ret = -E2BIG;
> > +		goto err_buf;
> > +	}
> > +	tpm_buf_fill_hmac_session(chip, &buf);
> > +	ret = tpm_transmit_cmd(chip, &buf, 4, "RSA key loading");
> > +	ret = tpm_buf_check_hmac_response(chip, &buf, ret);
> > +	if (ret) {
> > +		ret = -EIO;
> > +		goto err_buf;
> > +	}
> > +	key_handle = be32_to_cpup((__be32
> > *)&buf.data[TPM_HEADER_SIZE]);
> > +
> > +	tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_RSA_DECRYPT);
> > +	tpm_buf_append_name(chip, &buf, key_handle, NULL);
> > +	tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
> > NULL, 0);
> > +	tpm_buf_append_u16(&buf, in_len);
> > +	tpm_buf_append(&buf, in, in_len);
> > +	tpm_buf_append_u16(&buf, TPM_ALG_NULL);
> > +	tpm_buf_append_u16(&buf, 0);
> > +	tpm_buf_fill_hmac_session(chip, &buf);
> > +	ret = tpm_transmit_cmd(chip, &buf, 4, "RSA key decrypting");
> > +	ret = tpm_buf_check_hmac_response(chip, &buf, ret);
> > +	if (ret) {
> > +		ret = -EIO;
> > +		goto err_blob;
> > +	}
> > +
> > +	pos = buf.data + TPM_HEADER_SIZE + 4;
> > +	decrypted_len = be16_to_cpup((__be16 *)pos);
> > +	pos += 2;
> > +
> > +	if (params->out_len < decrypted_len) {
> > +		ret = -EMSGSIZE;
> > +		goto err_blob;
> > +	}
> > +
> > +	memcpy(out, pos, decrypted_len);
> > +	ret = decrypted_len;
> > +
> > +err_blob:
> > +	tpm2_flush_context(chip, key_handle);
> > +
> > +err_buf:
> > +	tpm_buf_destroy(&buf);
> > +
> > +err_parent:
> > +	if (key->key.parent == TPM2_RH_NULL)
> > +		tpm2_flush_context(chip, parent);
> > +
> > +err_auth:
> > +	if (ret < 0)
> > +		tpm2_end_auth_session(chip);
> > +
> > +err_ops:
> > +	tpm_put_ops(chip);
> > +	return ret;
> > +}
> > +
> > +static int tpm2_key_rsa_decrypt(struct tpm_chip *chip, struct tpm2_key_rsa
> > *key,
> > +				struct kernel_pkey_params *params,
> > +				const void *in, void *out)
> > +{
> > +	const u8 *ptr;
> > +	int out_len;
> > +	u8 *work;
> > +	int ret;
> > +
> > +	work = kzalloc(params->out_len, GFP_KERNEL);
> > +	if (!work)
> > +		return -ENOMEM;
>
> Maybe use ERR_PTR() here and couple of more places

Hmm... but the function returns 'int'?

BR, Jarkko