Re: [PATCH 5/8] security: keys: trusted: Make sealed key properly interoperable

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Mon, 2019-12-09 at 10:09 +0000, David Woodhouse wrote:
> On Sat, 2019-12-07 at 21:11 -0800, James Bottomley wrote:
[...]
> > @@ -330,13 +332,16 @@ static int tpm2_load_cmd(struct tpm_chip
> > *chip,
> >  	unsigned int private_len;
> >  	unsigned int public_len;
> >  	unsigned int blob_len;
> > -	u8 *blob;
> > +	u8 *blob, *pub;
> >  	int rc;
> > +	u32 attrs;
> >  
> >  	rc = tpm2_key_decode(payload, options, &blob);
> > -	if (rc)
> > +	if (rc) {
> >  		/* old form */
> >  		blob = payload->blob;
> > +		payload->old_format = 1;
> > +	}
> >  
> >  	/* new format carries keyhandle but old format doesn't */
> >  	if (!options->keyhandle)
> > @@ -347,6 +352,16 @@ static int tpm2_load_cmd(struct tpm_chip
> > *chip,
> >  		return -E2BIG;
> >  
> >  	public_len = be16_to_cpup((__be16 *) &blob[2 +
> > private_len]);
> > +
> > +	pub = blob + 2 + private_len + 2;
> > +	/* key attributes are always at offset 4 */
> > +	attrs = get_unaligned_be32(pub + 4);
> 
> 
> At this point I don't believe you've checked yet that payload-
> >blob_len  is sufficient to know that these bytes exist.

Check added.

> I think you're reading 'private_len' from non-existent bytes too, if
> payload->blob_len is zero or one? Which I think was there before you
> started, but you touched it last...

Well, I started this because of bugs in the current code, so this is
just one more bug I have to fix.

James