[PATCH RESEND 0/2] Avoid sending invalid data to the TPM

[PATCH RESEND 0/2] Avoid sending invalid data to the TPM

[Alexander Steffen]

When trying to send invalid commands (with mismatching commandSize values)
to the TPM we discovered some cases in which data is sent to the TPM that
should not be sent there. Similar problems were fixed years ago, but this
one slipped through it seems.

Alexander Steffen (2):
  tpm-dev-common: Reject too short writes
  tpm-interface: Fix checks of buffer size

 drivers/char/tpm/tpm-dev-common.c |  2 +-
 drivers/char/tpm/tpm-interface.c  | 16 ++++++++--------
 drivers/char/tpm/tpm.h            |  3 ++-
 3 files changed, 11 insertions(+), 10 deletions(-)

-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH RESEND 1/2] tpm-dev-common: Reject too short writes

[Alexander Steffen]

tpm_common_write() in tpm-dev-common.c discards the information how much
data has actually been written to the buffer. Instead, all other code has
to rely on the commandSize field in the TPM command header to figure out
how many valid bytes are supposed to be in the buffer.

But there is nothing that enforces the value in the header to match the
actual buffer contents. So by claiming a larger size in the header than
has been written, stale buffer contents are sent to the TPM. With this
commit, this problem is detected and rejected accordingly.

This should have been fixed with CVE-2011-1161 long ago, but apparently
a correct version of that patch never made it into the kernel.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm-dev-common.c | 2 +-
 drivers/char/tpm/tpm-interface.c  | 9 ++++++---
 drivers/char/tpm/tpm.h            | 3 ++-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 610638a..c39b581 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -119,7 +119,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 		return -EPIPE;
 	}
 	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
-				sizeof(priv->data_buffer), 0);
+				sizeof(priv->data_buffer), in_size, 0);
 
 	tpm_put_ops(priv->chip);
 	if (out_size < 0) {
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1d6729b..78fb41d 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -375,6 +375,7 @@ static bool tpm_validate_command(struct tpm_chip *chip,
  * @chip: TPM chip to use
  * @buf: TPM command buffer
  * @bufsiz: length of the TPM command buffer
+ * @bufvalid: number of valid bytes in the TPM command buffer
  * @flags: tpm transmit flags - bitmap
  *
  * Return:
@@ -382,7 +383,8 @@ static bool tpm_validate_command(struct tpm_chip *chip,
  *     A negative number for system errors (errno).
  */
 ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
-		     u8 *buf, size_t bufsiz, unsigned int flags)
+		     u8 *buf, size_t bufsiz, size_t bufvalid,
+		     unsigned int flags)
 {
 	struct tpm_output_header *header = (void *)buf;
 	int rc;
@@ -401,7 +403,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
 	if (count == 0)
 		return -ENODATA;
-	if (count > bufsiz) {
+	if (count > bufsiz || count > bufvalid) {
 		dev_err(&chip->dev,
 			"invalid count value %x %zx\n", count, bufsiz);
 		return -E2BIG;
@@ -522,7 +524,8 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
 	int err;
 	ssize_t len;
 
-	len = tpm_transmit(chip, space, (u8 *)buf, bufsiz, flags);
+	len = tpm_transmit(chip, space, (u8 *)buf, bufsiz,
+			   be32_to_cpu(header->length), flags);
 	if (len <  0)
 		return len;
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2d5466a..a020eca 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -511,7 +511,8 @@ enum tpm_transmit_flags {
 };
 
 ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
-		     u8 *buf, size_t bufsiz, unsigned int flags);
+		     u8 *buf, size_t bufsiz, size_t bufvalid,
+		     unsigned int flags);
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
 			 const void *buf, size_t bufsiz,
 			 size_t min_rsp_body_length, unsigned int flags,
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH RESEND 2/2] tpm-interface: Fix checks of buffer size

[Alexander Steffen]

bufsiz contains the length of the buffer, whereas bufvalid contains the
number of valid bytes within the buffer. Therefore, bufvalid should be used
as a limit when reading from the buffer and bufsize when writing to it.

Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm-interface.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 78fb41d..f481cd1 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -393,19 +393,16 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	unsigned long stop;
 	bool need_locality;
 
-	if (!tpm_validate_command(chip, space, buf, bufsiz))
+	if (!tpm_validate_command(chip, space, buf, bufvalid))
 		return -EINVAL;
 
-	if (bufsiz > TPM_BUFSIZE)
-		bufsiz = TPM_BUFSIZE;
-
 	count = be32_to_cpu(*((__be32 *) (buf + 2)));
 	ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
 	if (count == 0)
 		return -ENODATA;
-	if (count > bufsiz || count > bufvalid) {
+	if (count > bufvalid) {
 		dev_err(&chip->dev,
-			"invalid count value %x %zx\n", count, bufsiz);
+			"invalid count value %x %zx\n", count, bufvalid);
 		return -E2BIG;
 	}
 
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH RESEND 0/2] Avoid sending invalid data to the TPM

[Jarkko Sakkinen]

On Thu, Aug 24, 2017 at 10:35:43AM +0200, Alexander Steffen wrote:
> When trying to send invalid commands (with mismatching commandSize values)
> to the TPM we discovered some cases in which data is sent to the TPM that
> should not be sent there. Similar problems were fixed years ago, but this
> one slipped through it seems.
> 
> Alexander Steffen (2):
>   tpm-dev-common: Reject too short writes
>   tpm-interface: Fix checks of buffer size
> 
>  drivers/char/tpm/tpm-dev-common.c |  2 +-
>  drivers/char/tpm/tpm-interface.c  | 16 ++++++++--------
>  drivers/char/tpm/tpm.h            |  3 ++-
>  3 files changed, 11 insertions(+), 10 deletions(-)
> 
> -- 
> 2.7.4
> 

Have you checked that these do no break /dev/tpmrm0?

I have some cheap unit tests here to smoke it:

https://github.com/jsakkine-intel/tpm2-scripts

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH RESEND 1/2] tpm-dev-common: Reject too short writes

[Jarkko Sakkinen]

On Thu, Aug 24, 2017 at 10:35:44AM +0200, Alexander Steffen wrote:
> tpm_common_write() in tpm-dev-common.c discards the information how much
> data has actually been written to the buffer. Instead, all other code has
> to rely on the commandSize field in the TPM command header to figure out
> how many valid bytes are supposed to be in the buffer.
> 
> But there is nothing that enforces the value in the header to match the
> actual buffer contents. So by claiming a larger size in the header than
> has been written, stale buffer contents are sent to the TPM. With this
> commit, this problem is detected and rejected accordingly.
> 
> This should have been fixed with CVE-2011-1161 long ago, but apparently
> a correct version of that patch never made it into the kernel.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
>  drivers/char/tpm/tpm-dev-common.c | 2 +-
>  drivers/char/tpm/tpm-interface.c  | 9 ++++++---
>  drivers/char/tpm/tpm.h            | 3 ++-
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 610638a..c39b581 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -119,7 +119,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>  		return -EPIPE;
>  	}
>  	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
> -				sizeof(priv->data_buffer), 0);
> +				sizeof(priv->data_buffer), in_size, 0);

Why you couldn't just

unsigned int bufsiz;

/* ... */

bufsiz = sizeof(priv->data_buffer);
if (in_size < bufsiz)
	bufsiz = in_size;

out_size = tpm_transmit(priv->chip, space, priv->data_buffer, bufsiz, 0);

/Jarkko

Re: [PATCH RESEND 1/2] tpm-dev-common: Reject too short writes

[Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w]

> > diff --git a/drivers/char/tpm/tpm-dev-common.c
> > b/drivers/char/tpm/tpm-dev-common.c
> > index 610638a..c39b581 100644
> > --- a/drivers/char/tpm/tpm-dev-common.c
> > +++ b/drivers/char/tpm/tpm-dev-common.c
> > @@ -119,7 +119,7 @@ ssize_t tpm_common_write(struct file *file, const
> char __user *buf,
> >  		return -EPIPE;
> >  	}
> >  	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
> > -				sizeof(priv->data_buffer), 0);
> > +				sizeof(priv->data_buffer), in_size, 0);
> 
> Why you couldn't just
> 
> unsigned int bufsiz;
> 
> /* ... */
> 
> bufsiz = sizeof(priv->data_buffer);
> if (in_size < bufsiz)
> 	bufsiz = in_size;
> 
> out_size = tpm_transmit(priv->chip, space, priv->data_buffer, bufsiz, 0);

Because the code needs to know both how large the buffer is (in order to avoid buffer overflows when writing to it) and how much of the data in the buffer is valid (in order not to send random junk to the TPM). This is made more explicit in PATCH 2/2.

Your example fails as soon as the response is longer than the command.

Alexander
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH RESEND 1/2] tpm-dev-common: Reject too short writes

[Jarkko Sakkinen]

On Mon, Aug 28, 2017 at 05:11:00PM +0000, Alexander.Steffen@infineon.com wrote:
> > > diff --git a/drivers/char/tpm/tpm-dev-common.c
> > > b/drivers/char/tpm/tpm-dev-common.c
> > > index 610638a..c39b581 100644
> > > --- a/drivers/char/tpm/tpm-dev-common.c
> > > +++ b/drivers/char/tpm/tpm-dev-common.c
> > > @@ -119,7 +119,7 @@ ssize_t tpm_common_write(struct file *file, const
> > char __user *buf,
> > >  		return -EPIPE;
> > >  	}
> > >  	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
> > > -				sizeof(priv->data_buffer), 0);
> > > +				sizeof(priv->data_buffer), in_size, 0);
> > 
> > Why you couldn't just
> > 
> > unsigned int bufsiz;
> > 
> > /* ... */
> > 
> > bufsiz = sizeof(priv->data_buffer);
> > if (in_size < bufsiz)
> > 	bufsiz = in_size;
> > 
> > out_size = tpm_transmit(priv->chip, space, priv->data_buffer, bufsiz, 0);
> 
> Because the code needs to know both how large the buffer is (in order to avoid buffer overflows when writing to it) and how much of the data in the buffer is valid (in order not to send random junk to the TPM). This is made more explicit in PATCH 2/2.
> 
> Your example fails as soon as the response is longer than the command.
> 
> Alexander

Got you.

Do the comparison for count tpm-dev-common.c as it is the only call site
where this is needed instead of scrabbling with the parameters. In other
call sites this is unnecessary at this point.

This will also make backporting a factor more sleek.

/Jarkko

Re: [PATCH RESEND 0/2] Avoid sending invalid data to the TPM

[Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w]

> On Thu, Aug 24, 2017 at 10:35:43AM +0200, Alexander Steffen wrote:
> > When trying to send invalid commands (with mismatching commandSize
> > values) to the TPM we discovered some cases in which data is sent to
> > the TPM that should not be sent there. Similar problems were fixed
> > years ago, but this one slipped through it seems.
> >
> > Alexander Steffen (2):
> >   tpm-dev-common: Reject too short writes
> >   tpm-interface: Fix checks of buffer size
> >
> >  drivers/char/tpm/tpm-dev-common.c |  2 +-
> > drivers/char/tpm/tpm-interface.c  | 16 ++++++++--------
> >  drivers/char/tpm/tpm.h            |  3 ++-
> >  3 files changed, 11 insertions(+), 10 deletions(-)
> >
> > --
> > 2.7.4
> >
> 
> Have you checked that these do no break /dev/tpmrm0?
> 
> I have some cheap unit tests here to smoke it:
> 
> https://github.com/jsakkine-intel/tpm2-scripts
> 
> /Jarkko

I've now included your tests in my automation, so that they will run for all my future changes. Is it sufficient to use tpm2_smoke.py or should I also run keyctl-smoke.sh? Not having used keyctl before, I'm not sure whether it has any dependencies/side effects.

There was one thing I needed to fix to make the tests run with my TPMs and I've sent you a pull request via GitHub for it.

Alexander
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

RE: [PATCH RESEND 1/2] tpm-dev-common: Reject too short writes

[Alexander.Steffen]

> On Mon, Aug 28, 2017 at 05:11:00PM +0000,
> Alexander.Steffen@infineon.com wrote:
> > > > diff --git a/drivers/char/tpm/tpm-dev-common.c
> > > > b/drivers/char/tpm/tpm-dev-common.c
> > > > index 610638a..c39b581 100644
> > > > --- a/drivers/char/tpm/tpm-dev-common.c
> > > > +++ b/drivers/char/tpm/tpm-dev-common.c
> > > > @@ -119,7 +119,7 @@ ssize_t tpm_common_write(struct file *file,
> > > > const
> > > char __user *buf,
> > > >  		return -EPIPE;
> > > >  	}
> > > >  	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
> > > > -				sizeof(priv->data_buffer), 0);
> > > > +				sizeof(priv->data_buffer), in_size, 0);
> > >
> > > Why you couldn't just
> > >
> > > unsigned int bufsiz;
> > >
> > > /* ... */
> > >
> > > bufsiz = sizeof(priv->data_buffer);
> > > if (in_size < bufsiz)
> > > 	bufsiz = in_size;
> > >
> > > out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
> > > bufsiz, 0);
> >
> > Because the code needs to know both how large the buffer is (in order to
> avoid buffer overflows when writing to it) and how much of the data in the
> buffer is valid (in order not to send random junk to the TPM). This is made
> more explicit in PATCH 2/2.
> >
> > Your example fails as soon as the response is longer than the command.
> >
> > Alexander
> 
> Got you.
> 
> Do the comparison for count tpm-dev-common.c as it is the only call site
> where this is needed instead of scrabbling with the parameters. In other call
> sites this is unnecessary at this point.
> 
> This will also make backporting a factor more sleek.

I am not entirely happy with that approach, since it leads to worse code, splitting the buffer size validation logic over multiple places, but I can see how it makes backporting easier thanks to a smaller diff. A new patch is on its way.

Maybe at some point those parts of the code and the internal interfaces should be thoroughly cleaned up (i.e. rewritten).

Alexander

Re: [PATCH RESEND 1/2] tpm-dev-common: Reject too short writes

[Jarkko Sakkinen]

On Mon, Sep 04, 2017 at 05:35:00PM +0000, Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org wrote:
> > On Mon, Aug 28, 2017 at 05:11:00PM +0000,
> > Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org wrote:
> > > > > diff --git a/drivers/char/tpm/tpm-dev-common.c
> > > > > b/drivers/char/tpm/tpm-dev-common.c
> > > > > index 610638a..c39b581 100644
> > > > > --- a/drivers/char/tpm/tpm-dev-common.c
> > > > > +++ b/drivers/char/tpm/tpm-dev-common.c
> > > > > @@ -119,7 +119,7 @@ ssize_t tpm_common_write(struct file *file,
> > > > > const
> > > > char __user *buf,
> > > > >  		return -EPIPE;
> > > > >  	}
> > > > >  	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
> > > > > -				sizeof(priv->data_buffer), 0);
> > > > > +				sizeof(priv->data_buffer), in_size, 0);
> > > >
> > > > Why you couldn't just
> > > >
> > > > unsigned int bufsiz;
> > > >
> > > > /* ... */
> > > >
> > > > bufsiz = sizeof(priv->data_buffer);
> > > > if (in_size < bufsiz)
> > > > 	bufsiz = in_size;
> > > >
> > > > out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
> > > > bufsiz, 0);
> > >
> > > Because the code needs to know both how large the buffer is (in order to
> > avoid buffer overflows when writing to it) and how much of the data in the
> > buffer is valid (in order not to send random junk to the TPM). This is made
> > more explicit in PATCH 2/2.
> > >
> > > Your example fails as soon as the response is longer than the command.
> > >
> > > Alexander
> > 
> > Got you.
> > 
> > Do the comparison for count tpm-dev-common.c as it is the only call site
> > where this is needed instead of scrabbling with the parameters. In other call
> > sites this is unnecessary at this point.
> > 
> > This will also make backporting a factor more sleek.
> 
> I am not entirely happy with that approach, since it leads to worse
> code, splitting the buffer size validation logic over multiple places,
> but I can see how it makes backporting easier thanks to a smaller
> diff. A new patch is on its way.

Well I do not see it as part of the common buffer validation logic if it
is required only when you access from user space.

It's not only about smaller diff but also unnecessary change of
semantics in the places where it is no absolutely needed.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot