[PATCH 0/2] Fix kernel buffer overruns caused by bit flips

[PATCH 0/2] Fix kernel buffer overruns caused by bit flips

[James Bottomley]

If a TPM is attached to a system via a serial bus on a platform that
suffers bit flips, we can get back dangerously wrong data.  This patch
series aims never to do a direct copy into a kernel buffer based on an
unchecked size value returned from the TPM.

Jeremy Boone (2):
  tpm: fix potential buffer overruns caused by bit glitches on the bus
  tpm drivers: fix potential buffer overruns caused by bit glitches on
    the bus

 drivers/char/tpm/st33zp24/st33zp24.c | 4 ++--
 drivers/char/tpm/tpm-interface.c     | 1 +
 drivers/char/tpm/tpm2-cmd.c          | 4 ++++
 drivers/char/tpm/tpm_i2c_infineon.c  | 5 +++--
 drivers/char/tpm/tpm_i2c_nuvoton.c   | 5 +++--
 drivers/char/tpm/tpm_tis_core.c      | 5 +++--
 6 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.12.3

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

[James Bottomley]

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(+)

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);
-- 
2.12.3

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

[James Bottomley]

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.  In all the
driver _recv() functions, we need to use a u32 to unmarshal the
response size, otherwise a bit flip of the 31st bit would cause the
expected variable to go negative, which would then try to read a huge
amount of data.  Also sanity check that the expected amount of data is
large enough for the TPM header.

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/st33zp24/st33zp24.c | 4 ++--
 drivers/char/tpm/tpm_i2c_infineon.c  | 5 +++--
 drivers/char/tpm/tpm_i2c_nuvoton.c   | 5 +++--
 drivers/char/tpm/tpm_tis_core.c      | 5 +++--
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c
index 4d1dc8b46877..f95b9c75175b 100644
--- a/drivers/char/tpm/st33zp24/st33zp24.c
+++ b/drivers/char/tpm/st33zp24/st33zp24.c
@@ -457,7 +457,7 @@ static int st33zp24_recv(struct tpm_chip *chip, unsigned char *buf,
 			    size_t count)
 {
 	int size = 0;
-	int expected;
+	u32 expected;
 
 	if (!chip)
 		return -EBUSY;
@@ -474,7 +474,7 @@ static int st33zp24_recv(struct tpm_chip *chip, unsigned char *buf,
 	}
 
 	expected = be32_to_cpu(*(__be32 *)(buf + 2));
-	if (expected > count) {
+	if (expected > count || expected < TPM_HEADER_SIZE) {
 		size = -EIO;
 		goto out;
 	}
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index 79d6bbb58e39..d5b44cadac56 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -473,7 +473,8 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	int size = 0;
-	int expected, status;
+	int status;
+	u32 expected;
 
 	if (count < TPM_HEADER_SIZE) {
 		size = -EIO;
@@ -488,7 +489,7 @@ static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	}
 
 	expected = be32_to_cpu(*(__be32 *)(buf + 2));
-	if ((size_t) expected > count) {
+	if (((size_t) expected > count) || (expected < TPM_HEADER_SIZE)) {
 		size = -EIO;
 		goto out;
 	}
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
index c6428771841f..17cf7af9a2c5 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -281,7 +281,8 @@ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	struct device *dev = chip->dev.parent;
 	struct i2c_client *client = to_i2c_client(dev);
 	s32 rc;
-	int expected, status, burst_count, retries, size = 0;
+	int status, burst_count, retries, size = 0;
+	u32 expected;
 
 	if (count < TPM_HEADER_SIZE) {
 		i2c_nuvoton_ready(chip);    /* return to idle */
@@ -323,7 +324,7 @@ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 		 * to machine native
 		 */
 		expected = be32_to_cpu(*(__be32 *) (buf + 2));
-		if (expected > count) {
+		if (expected > count || expected < size) {
 			dev_err(dev, "%s() expected > count\n", __func__);
 			size = -EIO;
 			continue;
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fdde971bc810..7561922bc8f8 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -202,7 +202,8 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int size = 0;
-	int expected, status;
+	int status;
+	u32 expected;
 
 	if (count < TPM_HEADER_SIZE) {
 		size = -EIO;
@@ -217,7 +218,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	}
 
 	expected = be32_to_cpu(*(__be32 *) (buf + 2));
-	if (expected > count) {
+	if (expected > count || expected < TPM_HEADER_SIZE) {
 		size = -EIO;
 		goto out;
 	}
-- 
2.12.3

Re: [PATCH 0/2] Fix kernel buffer overruns caused by bit flips

[Jason Gunthorpe]

On Fri, Feb 02, 2018 at 06:23:41PM +0100, James Bottomley wrote:
> If a TPM is attached to a system via a serial bus on a platform that
> suffers bit flips, we can get back dangerously wrong data.  This patch
> series aims never to do a direct copy into a kernel buffer based on an
> unchecked size value returned from the TPM.
> 
> Jeremy Boone (2):
>   tpm: fix potential buffer overruns caused by bit glitches on the bus
>   tpm drivers: fix potential buffer overruns caused by bit glitches on
>     the bus

Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>

Jason

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

[Jarkko Sakkinen]

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

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

[Jarkko Sakkinen]

On Fri, Feb 02, 2018 at 06:25:33PM +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.  In all the
> driver _recv() functions, we need to use a u32 to unmarshal the
> response size, otherwise a bit flip of the 31st bit would cause the
> expected variable to go negative, which would then try to read a huge
> amount of data.  Also sanity check that the expected amount of data is
> large enough for the TPM header.
> 
> 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/st33zp24/st33zp24.c | 4 ++--
>  drivers/char/tpm/tpm_i2c_infineon.c  | 5 +++--
>  drivers/char/tpm/tpm_i2c_nuvoton.c   | 5 +++--
>  drivers/char/tpm/tpm_tis_core.c      | 5 +++--
>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c
> index 4d1dc8b46877..f95b9c75175b 100644
> --- a/drivers/char/tpm/st33zp24/st33zp24.c
> +++ b/drivers/char/tpm/st33zp24/st33zp24.c
> @@ -457,7 +457,7 @@ static int st33zp24_recv(struct tpm_chip *chip, unsigned char *buf,
>  			    size_t count)
>  {
>  	int size = 0;
> -	int expected;
> +	u32 expected;
>  
>  	if (!chip)
>  		return -EBUSY;
> @@ -474,7 +474,7 @@ static int st33zp24_recv(struct tpm_chip *chip, unsigned char *buf,
>  	}
>  
>  	expected = be32_to_cpu(*(__be32 *)(buf + 2));
> -	if (expected > count) {
> +	if (expected > count || expected < TPM_HEADER_SIZE) {
>  		size = -EIO;
>  		goto out;
>  	}
> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
> index 79d6bbb58e39..d5b44cadac56 100644
> --- a/drivers/char/tpm/tpm_i2c_infineon.c
> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
> @@ -473,7 +473,8 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
>  static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  {
>  	int size = 0;
> -	int expected, status;
> +	int status;
> +	u32 expected;
>  
>  	if (count < TPM_HEADER_SIZE) {
>  		size = -EIO;
> @@ -488,7 +489,7 @@ static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  	}
>  
>  	expected = be32_to_cpu(*(__be32 *)(buf + 2));
> -	if ((size_t) expected > count) {
> +	if (((size_t) expected > count) || (expected < TPM_HEADER_SIZE)) {
>  		size = -EIO;
>  		goto out;
>  	}
> diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
> index c6428771841f..17cf7af9a2c5 100644
> --- a/drivers/char/tpm/tpm_i2c_nuvoton.c
> +++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
> @@ -281,7 +281,8 @@ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  	struct device *dev = chip->dev.parent;
>  	struct i2c_client *client = to_i2c_client(dev);
>  	s32 rc;
> -	int expected, status, burst_count, retries, size = 0;
> +	int status, burst_count, retries, size = 0;
> +	u32 expected;
>  
>  	if (count < TPM_HEADER_SIZE) {
>  		i2c_nuvoton_ready(chip);    /* return to idle */
> @@ -323,7 +324,7 @@ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  		 * to machine native
>  		 */
>  		expected = be32_to_cpu(*(__be32 *) (buf + 2));
> -		if (expected > count) {
> +		if (expected > count || expected < size) {
>  			dev_err(dev, "%s() expected > count\n", __func__);
>  			size = -EIO;
>  			continue;
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index fdde971bc810..7561922bc8f8 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -202,7 +202,8 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	int size = 0;
> -	int expected, status;
> +	int status;
> +	u32 expected;
>  
>  	if (count < TPM_HEADER_SIZE) {
>  		size = -EIO;
> @@ -217,7 +218,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  	}
>  
>  	expected = be32_to_cpu(*(__be32 *) (buf + 2));
> -	if (expected > count) {
> +	if (expected > count || expected < TPM_HEADER_SIZE) {
>  		size = -EIO;
>  		goto out;
>  	}
> -- 
> 2.12.3

LGTM but should be split into four commits.

/Jarkko

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

[Jeremy Boone]

> 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

________________________________

Jeremy Boone
Principal Security Consultant
NCC Group
51 Breithaupt Street,
Suite 100, Kitchener, N2H 5G5

Telephone: +1 226 606 8318<tel:+1 226 606 8318>
Mobile: +1 226 606 8318<tel:+1 226 606 8318>
Website: www.nccgroup.trust<http://www.nccgroup.trust>
Twitter: @NCCGroupplc<https://twitter.com/NCCGroupplc>
        [https://www.nccgroup.trust/static/img/emaillogo/ncc-group-logo.png]  <http://www.nccgroup.trust/>
________________________________


This email is sent for and on behalf of NCC Group. NCC Group is the trading name of NCC Services Limited (Registered in England CRN: 2802141). The ultimate holding company is NCC Group plc (Registered in England CRN: 4627044). This email may be confidential and/or legally privileged.

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

[James Bottomley]

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
> 

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

[Jarkko Sakkinen]

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