Received-SPF: pass (google.com: domain of linux-crypto+bounces-4316-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Wed, 22 May 2024 01:42:14 +0300
Message-Id: <D1FOOI70OW9N.Y2ATVMAK2QR@kernel.org>
Cc: "James Bottomley" <James.Bottomley@HansenPartnership.com>, "Herbert Xu"
 <herbert@gondor.apana.org.au>, <linux-integrity@vger.kernel.org>,
 <keyrings@vger.kernel.org>, <Andreas.Fuchs@infineon.com>, "James Prestwood"
 <prestwoj@gmail.com>, "David Woodhouse" <dwmw2@infradead.org>, "Eric
 Biggers" <ebiggers@kernel.org>, "David S. Miller" <davem@davemloft.net>,
 "open list:CRYPTO API" <linux-crypto@vger.kernel.org>, "open list"
 <linux-kernel@vger.kernel.org>, "Peter Huewe" <peterhuewe@gmx.de>, "Jason
 Gunthorpe" <jgg@ziepe.ca>, "Mimi Zohar" <zohar@linux.ibm.com>, "Paul Moore"
 <paul@paul-moore.com>, "James Morris" <jmorris@namei.org>, "Serge E.
 Hallyn" <serge@hallyn.com>, "open list:SECURITY SUBSYSTEM"
 <linux-security-module@vger.kernel.org>
Subject: Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM
 driver
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "David Howells" <dhowells@redhat.com>
References: <D1FMVEJWGLEW.14QGHPAYPHQG1@kernel.org>
 <20240521031645.17008-1-jarkko@kernel.org>
 <20240521031645.17008-5-jarkko@kernel.org>
 <cc3d952f8295b52b052fbffe009b796ffb45707a.camel@HansenPartnership.com>
 <336755.1716327854@warthog.procyon.org.uk>
In-Reply-To: <336755.1716327854@warthog.procyon.org.uk>

On Wed May 22, 2024 at 12:44 AM EEST, David Howells wrote:
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> > On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote:
> > ...
> > You don't save a single byte of memory with any constant that dictates
> > the size requirements for multiple modules in two disjoint subsystems.
>
> I think James is just suggesting you replace your limit argument with a
> constant not that you always allocate that amount of memory.  What the li=
mit
> should be, OTOH, is up for discussion, but PAGE_SIZE seems not unreasonab=
le.

When the decoder for ASN.1 was part of trusted keys, the check used to
be:

	if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
		return -EINVAL;

And MAX_BLOB_SIZE is only 512 bytes, which does not fit event 2048 bit
RSA key but that 512 bytes cap seems to be just fine for trusted keys.

So the new check is:

	if (blob_len > max_key_len)
		return -E2BIG;

1. Too big value is not invalid value, thus -E2BIG. It is has also
   shown to be practically useful while testing this key type.
2. tpm2_key_rsa needs up to 8192 bytes for a blob to fit 4096-bit
   RSA key.=20

Just saying but there is also primary null key allocated by the driver.
And neither driver uses MAX_BLOB_SiZE. It uses value 8x MAX_BLOB_SIZE
i.e. 4096 bytes so not really following the idea suggested.

Finaly, there is three completely separate algorithms:

- KEYEDHASH (trusted_keys)
- RSA (tpm2_key_rsa)
- ECDSA (driver)=C2=A7
=09
With all this put together it is just common sense to have parametrized
cap value, and it would have no logic at all to treat them unified way.

For tpm2_key_rsa I will define for clarity:

#define TPM2_KEY_RSA_MAX_SIZE 8192

For tpm2_key_ecdsa you would define

#define TPM2_KEY_ECDSA_MAX_SIZE 4096

So yeah, this is how I will proceed because it is really the only
senseful way to proceed.

>
> David

BR, Jarkko