Received-SPF: pass (google.com: domain of linux-crypto+bounces-4298-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Tue, 21 May 2024 10:38:45 +0300
Message-Id: <D1F5GQYP9JFC.3MXMKUM0CKGS5@kernel.org>
Cc: "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
 "keyrings@vger.kernel.org" <keyrings@vger.kernel.org>,
 "Andreas.Fuchs@infineon.com" <Andreas.Fuchs@infineon.com>, "James
 Prestwood" <prestwoj@gmail.com>, "David Woodhouse" <dwmw2@infradead.org>,
 "Eric Biggers" <ebiggers@kernel.org>, "James Bottomley"
 <James.Bottomley@hansenpartnership.com>, "David S. Miller"
 <davem@davemloft.net>, "open list:CRYPTO API"
 <linux-crypto@vger.kernel.org>, "open list" <linux-kernel@vger.kernel.org>,
 "David Howells" <dhowells@redhat.com>, "James Bottomley"
 <James.Bottomley@HansenPartnership.com>, "Stefan Berger"
 <stefanb@linux.ibm.com>, "Ard Biesheuvel" <ardb@kernel.org>, "Mario
 Limonciello" <mario.limonciello@amd.com>
Subject: Re: [EXTERNAL] [PATCH v2 6/6] keys: asymmetric:
 ASYMMETRIC_TPM2_KEY_RSA_SUBTYPE
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Bharat Bhushan" <bbhushan2@marvell.com>, "Herbert Xu"
 <herbert@gondor.apana.org.au>
References: <20240521031645.17008-1-jarkko@kernel.org>
 <20240521031645.17008-7-jarkko@kernel.org>
 <SN7PR18MB531494159D3996799475209DE3EA2@SN7PR18MB5314.namprd18.prod.outlook.com>
In-Reply-To: <
 <SN7PR18MB531494159D3996799475209DE3EA2@SN7PR18MB5314.namprd18.prod.outlook.com>

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

Yeah, makes sense.


> > +	if (!rc)
> > +		rc =3D 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 =3D 0;
> > +	u32 key_handle =3D 0;
> > +	struct tpm_buf buf;
> > +	u16 decrypted_len;
> > +	u32 parent;
> > +	u8 *pos;
> > +	int ret;
> > +
> > +	ret =3D tpm_try_get_ops(chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret =3D tpm2_start_auth_session(chip);
> > +	if (ret)
> > +		goto err_ops;
> > +
> > +	if (key->key.parent =3D=3D TPM2_RH_NULL) {
> > +		ret =3D tpm2_load_context(chip, chip->null_key_context,
> > &offset,
> > +					&parent);
> > +		if (ret) {
> > +			ret =3D -EIO;
> > +			goto err_auth;
> > +		}
> > +	} else {
> > +		parent =3D 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 =3D 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 =3D -E2BIG;
> > +		goto err_buf;
> > +	}
> > +	tpm_buf_fill_hmac_session(chip, &buf);
> > +	ret =3D tpm_transmit_cmd(chip, &buf, 4, "RSA key loading");
> > +	ret =3D tpm_buf_check_hmac_response(chip, &buf, ret);
> > +	if (ret) {
> > +		ret =3D -EIO;
> > +		goto err_buf;
> > +	}
> > +	key_handle =3D 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 =3D tpm_transmit_cmd(chip, &buf, 4, "RSA key decrypting");
> > +	ret =3D tpm_buf_check_hmac_response(chip, &buf, ret);
> > +	if (ret) {
> > +		ret =3D -EIO;
> > +		goto err_blob;
> > +	}
> > +
> > +	pos =3D buf.data + TPM_HEADER_SIZE + 4;
> > +	decrypted_len =3D be16_to_cpup((__be16 *)pos);
> > +	pos +=3D 2;
> > +
> > +	if (params->out_len < decrypted_len) {
> > +		ret =3D -EMSGSIZE;
> > +		goto err_blob;
> > +	}
> > +
> > +	memcpy(out, pos, decrypted_len);
> > +	ret =3D decrypted_len;
> > +
> > +err_blob:
> > +	tpm2_flush_context(chip, key_handle);
> > +
> > +err_buf:
> > +	tpm_buf_destroy(&buf);
> > +
> > +err_parent:
> > +	if (key->key.parent =3D=3D 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 =3D 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