KEYS: trusted: Fix tpm2_load_cmd() boundary check

[PATCH] KEYS: trusted: Fix tpm2_load_cmd() boundary check

Posted by Jarkko Sakkinen 3 weeks, 3 days ago

tpm2_load_cmd() does boundary checks against the ASN.1 size i.e.,
payload->blob_len. Address this by passing the decoded blob size to
tpm2_load_cmd(), and use it for the boundary checks.

Cc: stable@vger.kernel.org # v5.13+
Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
Reported-by: co+6a581c4284f721d4@bugs.sh
Closes: https://bugs.sh/b/6a581c4284f721d4/
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 67225dd562a9..01f18bb37047 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -99,7 +99,7 @@ struct tpm2_key_context {
 
 static int tpm2_key_decode(struct trusted_key_payload *payload,
 			   struct trusted_key_options *options,
-			   u8 **buf)
+			   u8 **buf, unsigned int *blob_len)
 {
 	int ret;
 	struct tpm2_key_context ctx;
@@ -120,6 +120,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
 		return -ENOMEM;
 
 	*buf = blob;
+	*blob_len = ctx.priv_len + ctx.pub_len;
 	options->keyhandle = ctx.parent;
 
 	memcpy(blob, ctx.priv, ctx.priv_len);
@@ -384,10 +385,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 	int rc;
 	u32 attrs;
 
-	rc = tpm2_key_decode(payload, options, &blob);
+	rc = tpm2_key_decode(payload, options, &blob, &blob_len);
 	if (rc) {
 		/* old form */
 		blob = payload->blob;
+		blob_len = payload->blob_len;
 		payload->old_format = 1;
 	} else {
 		/* Bind for cleanup: */
@@ -399,17 +401,17 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		return -EINVAL;
 
 	/* must be big enough for at least the two be16 size counts */
-	if (payload->blob_len < 4)
+	if (blob_len < 4)
 		return -EINVAL;
 
 	private_len = get_unaligned_be16(blob);
 
 	/* must be big enough for following public_len */
-	if (private_len + 2 + 2 > (payload->blob_len))
+	if (private_len + 2 + 2 > blob_len)
 		return -E2BIG;
 
 	public_len = get_unaligned_be16(blob + 2 + private_len);
-	if (private_len + 2 + public_len + 2 > payload->blob_len)
+	if (private_len + 2 + public_len + 2 > blob_len)
 		return -E2BIG;
 
 	pub = blob + 2 + private_len + 2;
-- 
2.47.3

Re: [PATCH] KEYS: trusted: Fix tpm2_load_cmd() boundary check

Posted by Srish Srinivasan 3 weeks, 2 days ago

On 9/2/26 2:28 AM, Jarkko Sakkinen wrote:
> tpm2_load_cmd() does boundary checks against the ASN.1 size i.e.,
> payload->blob_len. Address this by passing the decoded blob size to
> tpm2_load_cmd(), and use it for the boundary checks.
>
> Cc: stable@vger.kernel.org # v5.13+
> Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
> Reported-by: co+6a581c4284f721d4@bugs.sh
> Closes: https://bugs.sh/b/6a581c4284f721d4/
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>


Tested-by: Srish Srinivasan <ssrish@linux.ibm.com>


> ---
>   security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 67225dd562a9..01f18bb37047 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -99,7 +99,7 @@ struct tpm2_key_context {
>   
>   static int tpm2_key_decode(struct trusted_key_payload *payload,
>   			   struct trusted_key_options *options,
> -			   u8 **buf)
> +			   u8 **buf, unsigned int *blob_len)
>   {
>   	int ret;
>   	struct tpm2_key_context ctx;
> @@ -120,6 +120,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
>   		return -ENOMEM;
>   
>   	*buf = blob;
> +	*blob_len = ctx.priv_len + ctx.pub_len;
>   	options->keyhandle = ctx.parent;
>   
>   	memcpy(blob, ctx.priv, ctx.priv_len);
> @@ -384,10 +385,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>   	int rc;
>   	u32 attrs;
>   
> -	rc = tpm2_key_decode(payload, options, &blob);
> +	rc = tpm2_key_decode(payload, options, &blob, &blob_len);
>   	if (rc) {
>   		/* old form */
>   		blob = payload->blob;
> +		blob_len = payload->blob_len;
>   		payload->old_format = 1;
>   	} else {
>   		/* Bind for cleanup: */
> @@ -399,17 +401,17 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>   		return -EINVAL;
>   
>   	/* must be big enough for at least the two be16 size counts */
> -	if (payload->blob_len < 4)
> +	if (blob_len < 4)
>   		return -EINVAL;
>   
>   	private_len = get_unaligned_be16(blob);
>   
>   	/* must be big enough for following public_len */
> -	if (private_len + 2 + 2 > (payload->blob_len))
> +	if (private_len + 2 + 2 > blob_len)
>   		return -E2BIG;
>   
>   	public_len = get_unaligned_be16(blob + 2 + private_len);
> -	if (private_len + 2 + public_len + 2 > payload->blob_len)
> +	if (private_len + 2 + public_len + 2 > blob_len)
>   		return -E2BIG;
>   
>   	pub = blob + 2 + private_len + 2;

Re: [PATCH] KEYS: trusted: Fix tpm2_load_cmd() boundary check

Posted by Stefano Garzarella 3 weeks, 3 days ago

On Tue, Sep 01, 2026 at 11:58:06PM +0300, Jarkko Sakkinen wrote:
>tpm2_load_cmd() does boundary checks against the ASN.1 size i.e.,
>payload->blob_len. Address this by passing the decoded blob size to
>tpm2_load_cmd(), and use it for the boundary checks.
>
>Cc: stable@vger.kernel.org # v5.13+
>Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
>Reported-by: co+6a581c4284f721d4@bugs.sh
>Closes: https://bugs.sh/b/6a581c4284f721d4/
>Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>---
> security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
>diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
>index 67225dd562a9..01f18bb37047 100644
>--- a/security/keys/trusted-keys/trusted_tpm2.c
>+++ b/security/keys/trusted-keys/trusted_tpm2.c
>@@ -99,7 +99,7 @@ struct tpm2_key_context {
>
> static int tpm2_key_decode(struct trusted_key_payload *payload,
> 			   struct trusted_key_options *options,
>-			   u8 **buf)
>+			   u8 **buf, unsigned int *blob_len)
> {
> 	int ret;
> 	struct tpm2_key_context ctx;
>@@ -120,6 +120,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,

	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);

Pre-existing, but is `+ 4` here useless?

I'm not asking to fix here, just noticed while reviewing.
Maybe we can set *blob_len earlier and use it also in the kmalloc().
Not a strong opinion, that said this LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

	if (!blob)
> 		return -ENOMEM;
>
> 	*buf = blob;
>+	*blob_len = ctx.priv_len + ctx.pub_len;
> 	options->keyhandle = ctx.parent;
>
> 	memcpy(blob, ctx.priv, ctx.priv_len);
>@@ -384,10 +385,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> 	int rc;
> 	u32 attrs;
>
>-	rc = tpm2_key_decode(payload, options, &blob);
>+	rc = tpm2_key_decode(payload, options, &blob, &blob_len);
> 	if (rc) {
> 		/* old form */
> 		blob = payload->blob;
>+		blob_len = payload->blob_len;
> 		payload->old_format = 1;
> 	} else {
> 		/* Bind for cleanup: */
>@@ -399,17 +401,17 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> 		return -EINVAL;
>
> 	/* must be big enough for at least the two be16 size counts */
>-	if (payload->blob_len < 4)
>+	if (blob_len < 4)
> 		return -EINVAL;
>
> 	private_len = get_unaligned_be16(blob);
>
> 	/* must be big enough for following public_len */
>-	if (private_len + 2 + 2 > (payload->blob_len))
>+	if (private_len + 2 + 2 > blob_len)
> 		return -E2BIG;
>
> 	public_len = get_unaligned_be16(blob + 2 + private_len);
>-	if (private_len + 2 + public_len + 2 > payload->blob_len)
>+	if (private_len + 2 + public_len + 2 > blob_len)
> 		return -E2BIG;
>
> 	pub = blob + 2 + private_len + 2;
>-- 
>2.47.3
>

Re: [PATCH] KEYS: trusted: Fix tpm2_load_cmd() boundary check

Posted by Jarkko Sakkinen 2 weeks, 2 days ago

On Wed, Sep 02, 2026 at 11:24:16AM +0200, Stefano Garzarella wrote:
> On Tue, Sep 01, 2026 at 11:58:06PM +0300, Jarkko Sakkinen wrote:
> > tpm2_load_cmd() does boundary checks against the ASN.1 size i.e.,
> > payload->blob_len. Address this by passing the decoded blob size to
> > tpm2_load_cmd(), and use it for the boundary checks.
> > 
> > Cc: stable@vger.kernel.org # v5.13+
> > Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
> > Reported-by: co+6a581c4284f721d4@bugs.sh
> > Closes: https://bugs.sh/b/6a581c4284f721d4/
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> > index 67225dd562a9..01f18bb37047 100644
> > --- a/security/keys/trusted-keys/trusted_tpm2.c
> > +++ b/security/keys/trusted-keys/trusted_tpm2.c
> > @@ -99,7 +99,7 @@ struct tpm2_key_context {
> > 
> > static int tpm2_key_decode(struct trusted_key_payload *payload,
> > 			   struct trusted_key_options *options,
> > -			   u8 **buf)
> > +			   u8 **buf, unsigned int *blob_len)
> > {
> > 	int ret;
> > 	struct tpm2_key_context ctx;
> > @@ -120,6 +120,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
> 
> 	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
> 
> Pre-existing, but is `+ 4` here useless?

I checked this through in detail.

It should not be like that given that callbacks tpm2_key_{pub,priv}
provide the length of TPM2B_PUBLIC and TPM2B_PRIVATE.

Thus, it is a bug introduced by f2219745250f ("security: keys: trusted:
use ASN.1 TPM2 key format for the blobs")

BR, Jarkko

Re: [PATCH] KEYS: trusted: Fix tpm2_load_cmd() boundary check

Posted by Stefano Garzarella 2 weeks, 2 days ago

On Wed, Sep 09, 2026 at 11:59:46PM +0300, Jarkko Sakkinen wrote:
>On Wed, Sep 02, 2026 at 11:24:16AM +0200, Stefano Garzarella wrote:
>> On Tue, Sep 01, 2026 at 11:58:06PM +0300, Jarkko Sakkinen wrote:
>> > tpm2_load_cmd() does boundary checks against the ASN.1 size i.e.,
>> > payload->blob_len. Address this by passing the decoded blob size to
>> > tpm2_load_cmd(), and use it for the boundary checks.
>> >
>> > Cc: stable@vger.kernel.org # v5.13+
>> > Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
>> > Reported-by: co+6a581c4284f721d4@bugs.sh
>> > Closes: https://bugs.sh/b/6a581c4284f721d4/
>> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>> > ---
>> > security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++-----
>> > 1 file changed, 7 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
>> > index 67225dd562a9..01f18bb37047 100644
>> > --- a/security/keys/trusted-keys/trusted_tpm2.c
>> > +++ b/security/keys/trusted-keys/trusted_tpm2.c
>> > @@ -99,7 +99,7 @@ struct tpm2_key_context {
>> >
>> > static int tpm2_key_decode(struct trusted_key_payload *payload,
>> > 			   struct trusted_key_options *options,
>> > -			   u8 **buf)
>> > +			   u8 **buf, unsigned int *blob_len)
>> > {
>> > 	int ret;
>> > 	struct tpm2_key_context ctx;
>> > @@ -120,6 +120,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
>>
>> 	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
>>
>> Pre-existing, but is `+ 4` here useless?
>
>I checked this through in detail.
>
>It should not be like that given that callbacks tpm2_key_{pub,priv}
>provide the length of TPM2B_PUBLIC and TPM2B_PRIVATE.
>
>Thus, it is a bug introduced by f2219745250f ("security: keys: trusted:
>use ASN.1 TPM2 key format for the blobs")

Yep, agree. Do you want me to send a fix?

Thanks,
Stefano

Re: [PATCH] KEYS: trusted: Fix tpm2_load_cmd() boundary check

Posted by Jarkko Sakkinen 1 week, 1 day ago

On Thu, Sep 10, 2026 at 09:52:38AM +0200, Stefano Garzarella wrote:
> On Wed, Sep 09, 2026 at 11:59:46PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Sep 02, 2026 at 11:24:16AM +0200, Stefano Garzarella wrote:
> > > On Tue, Sep 01, 2026 at 11:58:06PM +0300, Jarkko Sakkinen wrote:
> > > > tpm2_load_cmd() does boundary checks against the ASN.1 size i.e.,
> > > > payload->blob_len. Address this by passing the decoded blob size to
> > > > tpm2_load_cmd(), and use it for the boundary checks.
> > > >
> > > > Cc: stable@vger.kernel.org # v5.13+
> > > > Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
> > > > Reported-by: co+6a581c4284f721d4@bugs.sh
> > > > Closes: https://bugs.sh/b/6a581c4284f721d4/
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > ---
> > > > security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++-----
> > > > 1 file changed, 7 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> > > > index 67225dd562a9..01f18bb37047 100644
> > > > --- a/security/keys/trusted-keys/trusted_tpm2.c
> > > > +++ b/security/keys/trusted-keys/trusted_tpm2.c
> > > > @@ -99,7 +99,7 @@ struct tpm2_key_context {
> > > >
> > > > static int tpm2_key_decode(struct trusted_key_payload *payload,
> > > > 			   struct trusted_key_options *options,
> > > > -			   u8 **buf)
> > > > +			   u8 **buf, unsigned int *blob_len)
> > > > {
> > > > 	int ret;
> > > > 	struct tpm2_key_context ctx;
> > > > @@ -120,6 +120,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
> > > 
> > > 	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
> > > 
> > > Pre-existing, but is `+ 4` here useless?
> > 
> > I checked this through in detail.
> > 
> > It should not be like that given that callbacks tpm2_key_{pub,priv}
> > provide the length of TPM2B_PUBLIC and TPM2B_PRIVATE.
> > 
> > Thus, it is a bug introduced by f2219745250f ("security: keys: trusted:
> > use ASN.1 TPM2 key format for the blobs")
> 
> Yep, agree. Do you want me to send a fix?

Sure, go ahead.

> Thanks,
> Stefano

BR, Jarkko