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

[PATCH v2 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 (5):
  tpm: fix potential buffer overruns caused by bit glitches on the bus
  tpm: st33zp24: fix potential buffer overruns caused by bit glitches on
    the bus
  tpm_i2c_infineon: fix potential buffer overruns caused by bit glitches
    on the bus
  tpm_i2c_nuvoton: fix potential buffer overruns caused by bit glitches
    on the bus
  tpm_tis: fix potential buffer overruns caused by bit glitches on the
    bus

 drivers/char/tpm/st33zp24/st33zp24.c | 4 ++--
 drivers/char/tpm/tpm-interface.c     | 4 ++++
 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, 19 insertions(+), 8 deletions(-)

-- 
2.12.3

[PATCH v2 2/5] tpm: st33zp24: 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 ++--
 1 file changed, 2 insertions(+), 2 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;
 	}
-- 
2.12.3

[PATCH v2 3/5] tpm_i2c_infineon: 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/tpm_i2c_infineon.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

[PATCH v2 4/5] tpm_i2c_nuvoton: 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/tpm_i2c_nuvoton.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

[PATCH v2 5/5] tpm_tis: 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/tpm_tis_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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 v2 0/2] Fix kernel buffer overruns caused by bit flips

[Jarkko Sakkinen]

On Thu, Feb 08, 2018 at 12:26:16PM -0800, 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 (5):
>   tpm: fix potential buffer overruns caused by bit glitches on the bus
>   tpm: st33zp24: fix potential buffer overruns caused by bit glitches on
>     the bus
>   tpm_i2c_infineon: fix potential buffer overruns caused by bit glitches
>     on the bus
>   tpm_i2c_nuvoton: fix potential buffer overruns caused by bit glitches
>     on the bus
>   tpm_tis: fix potential buffer overruns caused by bit glitches on the
>     bus
> 
>  drivers/char/tpm/st33zp24/st33zp24.c | 4 ++--
>  drivers/char/tpm/tpm-interface.c     | 4 ++++
>  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, 19 insertions(+), 8 deletions(-)
> 
> -- 
>o 2.12.3

Reviewed-by: Jarkko Sakkinen <jarkko.sakkkinen@linux.intel.com>

I cannot test all of these but I'll put these to linux-next anyway. The
changes are obvious and small scoped so if no one shouts they'll be part
of the next PR.

I've tested the changes that affect tpm2-cmd.c, tpm-interface.c and
tpm_tis_core.c. For HW specific changes tested-by's would be much
appreciated but I don't think they will break anything.

Because these are quite critical fixes I wonder if I could do one more
PR to 4.16?

/Jarkko

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

[James Bottomley]

On Fri, 2018-02-09 at 19:13 +0200, Jarkko Sakkinen wrote:
> On Thu, Feb 08, 2018 at 12:26:16PM -0800, 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 (5):
> >   tpm: fix potential buffer overruns caused by bit glitches on the
> > bus
> >   tpm: st33zp24: fix potential buffer overruns caused by bit
> > glitches on
> >     the bus
> >   tpm_i2c_infineon: fix potential buffer overruns caused by bit
> > glitches
> >     on the bus
> >   tpm_i2c_nuvoton: fix potential buffer overruns caused by bit
> > glitches
> >     on the bus
> >   tpm_tis: fix potential buffer overruns caused by bit glitches on
> > the
> >     bus
> > 
> >  drivers/char/tpm/st33zp24/st33zp24.c | 4 ++--
> >  drivers/char/tpm/tpm-interface.c     | 4 ++++
> >  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, 19 insertions(+), 8 deletions(-)
> > 
> > -- 
> > o 2.12.3
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkkinen@linux.intel.com>
> 
> I cannot test all of these but I'll put these to linux-next anyway.
> The changes are obvious and small scoped so if no one shouts they'll
> be part of the next PR.
> 
> I've tested the changes that affect tpm2-cmd.c, tpm-interface.c and
> tpm_tis_core.c. For HW specific changes tested-by's would be much
> appreciated but I don't think they will break anything.
> 
> Because these are quite critical fixes I wonder if I could do one
> more PR to 4.16?

They're all cc'd to stable, so they'd make the stable updates to 4.16
regardless of when they're pulled.  Since the merge window will close
on Sunday and you have to go via James' tree, I'd say it would cause a
lot of stress to try to make 4.16 but it's your call.

James

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

[Jarkko Sakkinen]

On Fri, Feb 09, 2018 at 09:33:31AM -0800, James Bottomley wrote:
> On Fri, 2018-02-09 at 19:13 +0200, Jarkko Sakkinen wrote:
> > On Thu, Feb 08, 2018 at 12:26:16PM -0800, 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 (5):
> > >   tpm: fix potential buffer overruns caused by bit glitches on the
> > > bus
> > >   tpm: st33zp24: fix potential buffer overruns caused by bit
> > > glitches on
> > >     the bus
> > >   tpm_i2c_infineon: fix potential buffer overruns caused by bit
> > > glitches
> > >     on the bus
> > >   tpm_i2c_nuvoton: fix potential buffer overruns caused by bit
> > > glitches
> > >     on the bus
> > >   tpm_tis: fix potential buffer overruns caused by bit glitches on
> > > the
> > >     bus
> > > 
> > >  drivers/char/tpm/st33zp24/st33zp24.c | 4 ++--
> > >  drivers/char/tpm/tpm-interface.c     | 4 ++++
> > >  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, 19 insertions(+), 8 deletions(-)
> > > 
> > > -- 
> > > o 2.12.3
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkkinen@linux.intel.com>
> > 
> > I cannot test all of these but I'll put these to linux-next anyway.
> > The changes are obvious and small scoped so if no one shouts they'll
> > be part of the next PR.
> > 
> > I've tested the changes that affect tpm2-cmd.c, tpm-interface.c and
> > tpm_tis_core.c. For HW specific changes tested-by's would be much
> > appreciated but I don't think they will break anything.
> > 
> > Because these are quite critical fixes I wonder if I could do one
> > more PR to 4.16?
> 
> They're all cc'd to stable, so they'd make the stable updates to 4.16
> regardless of when they're pulled.  Since the merge window will close
> on Sunday and you have to go via James' tree, I'd say it would cause a
> lot of stress to try to make 4.16 but it's your call.
> 
> James

Agreed, thanks for input!

/Jarkko

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

[Jarkko Sakkinen]

On Wed, Feb 14, 2018 at 01:44:56PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 09, 2018 at 09:33:31AM -0800, James Bottomley wrote:
> > On Fri, 2018-02-09 at 19:13 +0200, Jarkko Sakkinen wrote:
> > > On Thu, Feb 08, 2018 at 12:26:16PM -0800, 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 (5):
> > > >   tpm: fix potential buffer overruns caused by bit glitches on the
> > > > bus
> > > >   tpm: st33zp24: fix potential buffer overruns caused by bit
> > > > glitches on
> > > >     the bus
> > > >   tpm_i2c_infineon: fix potential buffer overruns caused by bit
> > > > glitches
> > > >     on the bus
> > > >   tpm_i2c_nuvoton: fix potential buffer overruns caused by bit
> > > > glitches
> > > >     on the bus
> > > >   tpm_tis: fix potential buffer overruns caused by bit glitches on
> > > > the
> > > >     bus
> > > > 
> > > >  drivers/char/tpm/st33zp24/st33zp24.c | 4 ++--
> > > >  drivers/char/tpm/tpm-interface.c     | 4 ++++
> > > >  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, 19 insertions(+), 8 deletions(-)
> > > > 
> > > > -- 
> > > > o 2.12.3
> > > 
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkkinen@linux.intel.com>
> > > 
> > > I cannot test all of these but I'll put these to linux-next anyway.
> > > The changes are obvious and small scoped so if no one shouts they'll
> > > be part of the next PR.
> > > 
> > > I've tested the changes that affect tpm2-cmd.c, tpm-interface.c and
> > > tpm_tis_core.c. For HW specific changes tested-by's would be much
> > > appreciated but I don't think they will break anything.
> > > 
> > > Because these are quite critical fixes I wonder if I could do one
> > > more PR to 4.16?
> > 
> > They're all cc'd to stable, so they'd make the stable updates to 4.16
> > regardless of when they're pulled.  Since the merge window will close
> > on Sunday and you have to go via James' tree, I'd say it would cause a
> > lot of stress to try to make 4.16 but it's your call.
> > 
> > James
> 
> Agreed, thanks for input!
> 
> /Jarkko

They are now in master/next.

/Jarkko