Re: [PATCH] TPM: Fixup pcrs sysfs file

[Jason Gunthorpe]

On Thu, Sep 03, 2009 at 04:52:19PM -0700, Andrew Morton wrote:

> > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> > index a6b52d6..8ba0187 100644
> > +++ b/drivers/char/tpm/tpm.c
> > @@ -696,8 +696,8 @@ int __tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
> >  
> >  	cmd.header.in = pcrread_header;
> >  	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
> > -	BUILD_BUG_ON(cmd.header.in.length > READ_PCR_RESULT_SIZE);
> > -	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
> > +	BUILD_BUG_ON(sizeof(cmd) < READ_PCR_RESULT_SIZE);
> > +	rc = transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,
> >  			  "attempting to read a pcr value");
> >  
> >  	if (rc == 0)
> 
> That sounds like a fairly serious bug, and this looks like a 2.6.31
> patch.

To be fair, I'm not sure the pcrs sysfile provides anything terribly
usefull.. None of the sysfs files in this driver seem to follow the
standard one-value-one-file convention either. But, if it is going to
be included it may as well work properly...

> Jan's build_bug_on-fix-it-and-a-couple-of-bogus-uses-of-it.patch (in
> -mm) simply removes the bogus BUILD_BUG_ON(). I think we might as well
> do that within the context of your patch.

> So I end up with the below, which I propose for 2.6.31:

OK. That is fair. The tpm_cmd_params union contains a tpm_pcrread_out
which should 'by design' ensure there is enough space.

Jan's removal of the 2nd BUILD_BUG_ON is also good.

But I notice tpm_pcr_extend also has a mis-use of the transmit_cmd
idiom. This one functions ok because the in/out RPC message size
happen to be the same. But lets fix it too?

Thanks,
Jason