Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650823 > unrolled thread

Re: [PATCH RFC] tpm: migrate pubek_show to struct tpm_buf

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC] tpm: migrate pubek_show to struct tpm_buf Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-05-25 23:20 +0200
    Re: [PATCH RFC] tpm: migrate pubek_show to struct tpm_buf Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-05-26 00:30 +0200
      Re: [PATCH RFC] tpm: migrate pubek_show to struct tpm_buf Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-05-26 00:50 +0200
        Re: [PATCH RFC] tpm: migrate pubek_show to struct tpm_buf Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-05-30 07:00 +0200

#1650823 — Re: [PATCH RFC] tpm: migrate pubek_show to struct tpm_buf

On Thu, May 25, 2017 at 02:11:04PM -0700, Jarkko Sakkinen wrote:
>  	struct tpm_chip *chip = to_tpm_chip(dev);
> +	char anti_replay[20];
>  
> -	tpm_cmd.header.in = tpm_readpubek_header;
> -	err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
> +	rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK);
> +	if (rc)
> +		return rc;
> +
> +	/* The checksum is ignored so it doesn't matter what the contents are.
> +	 */
> +	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));

It does matter, we do not want to leak random kernel memory incase it
has something sensitive. Zero anti_replay.

> +
> -	/*
> -	   ignore header 10 bytes
> -	   algorithm 32 bits (1 == RSA )
> -	   encscheme 16 bits
> -	   sigscheme 16 bits
> -	   parameters (RSA 12->bytes: keybit, #primes, expbit)
> -	   keylenbytes 32 bits
> -	   256 byte modulus
> -	   ignore checksum 20 bytes
> -	 */

Not sure we should delete the comment, tpm buf does not make the parse
any clearer.

Jason

[toc] | [next] | [standalone]

#1650937

On Thu, May 25, 2017 at 03:16:13PM -0600, Jason Gunthorpe wrote:
> On Thu, May 25, 2017 at 02:11:04PM -0700, Jarkko Sakkinen wrote:
> >  	struct tpm_chip *chip = to_tpm_chip(dev);
> > +	char anti_replay[20];
> >  
> > -	tpm_cmd.header.in = tpm_readpubek_header;
> > -	err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
> > +	rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/* The checksum is ignored so it doesn't matter what the contents are.
> > +	 */
> > +	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
> 
> It does matter, we do not want to leak random kernel memory incase it
> has something sensitive. Zero anti_replay.

If there was a leak it has existed before this change as tpm_cmd was
also allocated from stack. And there is not leak because the checksum is
not printed.

> > +
> > -	/*
> > -	   ignore header 10 bytes
> > -	   algorithm 32 bits (1 == RSA )
> > -	   encscheme 16 bits
> > -	   sigscheme 16 bits
> > -	   parameters (RSA 12->bytes: keybit, #primes, expbit)
> > -	   keylenbytes 32 bits
> > -	   256 byte modulus
> > -	   ignore checksum 20 bytes
> > -	 */
> 
> Not sure we should delete the comment, tpm buf does not make the parse
> any clearer.

I think better idea would be to move struct tpm_readpubek_params_out
declaration here and use it to refer different fields. Previously this
has been a complete mess. The structure has been declared but it has not
been used for anything. I wonder what is the history here...

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1650953

On Thu, May 25, 2017 at 03:28:01PM -0700, Jarkko Sakkinen wrote:
> On Thu, May 25, 2017 at 03:16:13PM -0600, Jason Gunthorpe wrote:
> > On Thu, May 25, 2017 at 02:11:04PM -0700, Jarkko Sakkinen wrote:
> > >  	struct tpm_chip *chip = to_tpm_chip(dev);
> > > +	char anti_replay[20];
> > >  
> > > -	tpm_cmd.header.in = tpm_readpubek_header;
> > > -	err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
> > > +	rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	/* The checksum is ignored so it doesn't matter what the contents are.
> > > +	 */
> > > +	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
> > 
> > It does matter, we do not want to leak random kernel memory incase it
> > has something sensitive. Zero anti_replay.
> 
> If there was a leak it has existed before this change as tpm_cmd was
> also allocated from stack. And there is not leak because the checksum is
> not printed.

It leaks stack memory to the TPM which is not OK.

> I think better idea would be to move struct tpm_readpubek_params_out
> declaration here and use it to refer different fields. Previously
> this

That would be better..

Jason

[toc] | [prev] | [next] | [standalone]

#1652812

On Thu, May 25, 2017 at 04:40:50PM -0600, Jason Gunthorpe wrote:
> On Thu, May 25, 2017 at 03:28:01PM -0700, Jarkko Sakkinen wrote:
> > On Thu, May 25, 2017 at 03:16:13PM -0600, Jason Gunthorpe wrote:
> > > On Thu, May 25, 2017 at 02:11:04PM -0700, Jarkko Sakkinen wrote:
> > > >  	struct tpm_chip *chip = to_tpm_chip(dev);
> > > > +	char anti_replay[20];
> > > >  
> > > > -	tpm_cmd.header.in = tpm_readpubek_header;
> > > > -	err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
> > > > +	rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK);
> > > > +	if (rc)
> > > > +		return rc;
> > > > +
> > > > +	/* The checksum is ignored so it doesn't matter what the contents are.
> > > > +	 */
> > > > +	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
> > > 
> > > It does matter, we do not want to leak random kernel memory incase it
> > > has something sensitive. Zero anti_replay.
> > 
> > If there was a leak it has existed before this change as tpm_cmd was
> > also allocated from stack. And there is not leak because the checksum is
> > not printed.
> 
> It leaks stack memory to the TPM which is not OK.

Right, of course, vtpm_tpm_proxy.

/Jarkko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web