Re: [EXTERNAL] [PATCH v2 6/6] keys: asymmetric: ASYMMETRIC_TPM2_KEY_RSA_SUBTYPE

["Jarkko Sakkinen" <jarkko@kernel.org>]

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