Re: [PATCH] tpm: check size of response before accessing data

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Mon, Jan 09, 2017 at 07:15:29PM -0500, Stefan Berger wrote:
> On 01/09/2017 05:59 PM, Jarkko Sakkinen wrote:
> > On Mon, Jan 09, 2017 at 01:09:31PM -0500, Stefan Berger wrote:
> > > On 01/09/2017 11:05 AM, Jarkko Sakkinen wrote:
> > > > On Thu, Jan 05, 2017 at 07:11:24AM -0500, Stefan Berger wrote:
> > > > > Check the size of the response before accesing data in
> > > > > the response packet. This is to avoid accessing data beyond
> > > > > the end of the response.
> > > > > 
> > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > How on earth this could happen if we request only one property?
> > > My test program vtpmctrl ( https://github.com/stefanberger/linux-vtpm-tests
> > > ) didn't feed the kernel a proper response to a TPM command and that's why
> > > this code blew up. We do have a very basic check in the driver and otherwise
> > > assume that the TPM is a trusted device responding with an expected
> > > response.
> > Hmm.... I guess I could add this check but I'll have to probably
> > do a similar check at least in one other place in this patch set
> > where I grab the metadata for commands.
> > 
> > I guess similar issues will arise as the virtual TPMs get more
> > common. For now I think a good guideline is
> > 
> > 1. For new code check that validation for message size is in place.
> 
> Before accessing data in the response, make sure we don't access beyond the
> number of bytes returned.
> 
> > 2. Fix the old code as you bump into issus.
> 
> It doesn't look too bad. I would rebase my current patch on your master tree
> and submit a few small other ones with it. Agrred?

Hmm. Are you talking about stuff you are adding to the tpm2-space.c.
For me it is less trouble to add checks myself than applying 3rd party
patches while preparing the next patch set version. This is only
overhead for me and I will anyway would squash these checks to my
own commits.

> 
>    Stefan
> 

/Jarkko
> > 
> > /Jarkko
> > 
>