Re: [PATCH 1/2] tpm: fix potential buffer overruns caused by bit glitches on the bus

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

On Thu, 2018-02-08 at 15:34 +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 02, 2018 at 06:24:38PM +0100, James Bottomley wrote:
> > 
> > From: Jeremy Boone <jeremy.boone@nccgroup.trust>
> > 
> > Discrete TPMs are often connected over slow serial buses which, on
> > some platforms, can have glitches causing bit flips.  If a bit does
> > flip it could cause an overrun if it's in one of the size
> > parameters,
> > so sanity check that we're not overrunning the provided buffer when
> > doing a memcpy().
> > 
> > Signed-off-by: Jeremy Boone <jeremy.boone@nccgroup.trust>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > om>
> > ---
> >  drivers/char/tpm/tpm-interface.c | 1 +
> >  drivers/char/tpm/tpm2-cmd.c      | 4 ++++
> >  2 files changed, 5 insertions(+)
> 
> Please add me to to-field in the future.

Will do.  I tend to assume everyone uses my workflow which means I junk
the additional cc to me since I'll read it on the list anyway.

>  I'm also wondering where is the cover letter.

https://marc.info/?l=linux-integrity&m=151759223601566

> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 1d6729be4cd6..e99f4f71c74f 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -1228,6 +1228,7 @@ int tpm_get_random(u32 chip_num, u8 *out,
> > size_t max)
> >  			break;
> >  
> >  		recd =
> > be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len);
> > +		recd = min_t(u32, recd, num_bytes);
> 
> Shouldn't this be rather a check whether num_bytes is surpassed and
> return an error if that happens and maybe a klog message?

I can do that.  The aim of the patch series is to make sure we don't
overrun buffers and the min achieves that.  A message might help, but
there are still many forms of corruption we could get that won't be
detected without using HMACs.

> > 
> >  		rlength = be32_to_cpu(tpm_cmd.header.out.length);
> >  		if (rlength < offsetof(struct tpm_getrandom_out,
> > rng_data) +
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-
> > cmd.c
> > index f40d20671a78..f6be08483ae6 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -683,6 +683,10 @@ static int tpm2_unseal_cmd(struct tpm_chip
> > *chip,
> >  	if (!rc) {
> >  		data_len = be16_to_cpup(
> >  			(__be16 *) &buf.data[TPM_HEADER_SIZE +
> > 4]);
> > +		if (data_len < MIN_KEY_SIZE ||  data_len >
> > MAX_KEY_SIZE + 1) {
> > +			rc = -EFAULT;
> > +			goto out;
> > +		}
> 
> This change looks good to me but I'm thinking if this commit should
> split into two?

You mean one for each driver?  I can do that.

James

> /Jarkko
>