| Message ID | 1517592278.3137.19.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.com> > --- > 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. I'm also wondering where is the cover letter. > > 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? > 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? /Jarkko
> Shouldn't this be rather a check whether num_bytes is surpassed and > return an error if that happens and maybe a klog message? Jarkko, I had originally suggested the use of min_t() in an attempt to make this code symmetric with the behaviour seen in tpm2_get_random() in tpm2-cmd.c. But I see the value in your suggestion. Jeremy -----Original Message----- From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-owner@vger.kernel.org] On Behalf Of Jarkko Sakkinen Sent: Thursday, February 08, 2018 8:34 AM To: James Bottomley <James.Bottomley@HansenPartnership.com> Cc: linux-integrity@vger.kernel.org; Jeremy Boone <Jeremy.Boone@nccgroup.trust> Subject: EXTERNAL: Re: [PATCH 1/2] tpm: fix potential buffer overruns caused by bit glitches on the bus 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.com> > --- > 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. I'm also wondering where is the cover letter. > > 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? > 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? /Jarkko
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 >
On Thu, Feb 08, 2018 at 09:07:32AM -0800, James Bottomley wrote: > > 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. It's just that if there's lot of stuff going on those messages I'll prioritize. Patches will get reviewed in all cases but there might be more latency :-) /Jarkko
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); 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; + } rlength = be32_to_cpu(((struct tpm2_cmd *)&buf) ->header.out.length);